potiuk commented on a change in pull request #11131:
URL: https://github.com/apache/airflow/pull/11131#discussion_r495607576
##########
File path: chart/tests/git-sync-webserver_test.yaml
##########
@@ -60,7 +60,14 @@ tests:
container_name: git-sync
persistence:
enabled: true
+ - it: should have service account defined
+ set:
+ dags:
+ gitSync:
+ enabled: true
+ persistence:
+ enabled: true
Review comment:
They have no impact. But I chose the approach that I did not want to add
all the different combinations here. There is apparently missing set of unit
tests for all the different combinations of parameters for the helm charts -
for example only the git-sync option is tested for the helm chart rather than
all combinations. So rather than fixing the underlying problem (which might
take a lot of time) I wanted to add a simple test in the only webserver test I
have found - with git sync and persiatence.
I think if we want to add more tests we would need to add more comprehensive
set of tests but for now this is the simplest test I can add.
I guess more comprehensive tests should be the prerequisite for helm chart
testing. WDYT @dimbermann ? Is it worth to add more comprehensive tests or
should it be deferred ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
