rzjfr commented on code in PR #40554:
URL: https://github.com/apache/airflow/pull/40554#discussion_r1669863507


##########
helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -660,6 +660,26 @@ def test_safe_to_evict_annotation(self, safe_to_evict: 
bool):
             "cluster-autoscaler.kubernetes.io/safe-to-evict": "true" if 
safe_to_evict else "false"
         }
 
+    def test_safe_to_evict_annotation_other_services(self):
+        docs = render_chart(
+            values={
+                "workers": {"safeToEvict": False},
+                "scheduler": {"safeToEvict": True},

Review Comment:
   Hi Jed,
   Thanks for reviewing the PR,
   
   The test case I was thinking of is this: changing workers' `safeToEvict` 
should not override values of other services like scheduler. 
   
   Without the change in this PR, worker's `safeToEvict` overrides services' 
own value when applied due to the order of the duplicated annotations; 
`.Values.airflowPodAnnotations` is always the second. I couldn't test the 
duplication itself because `render_chart` returns a dictionary, so what I tried 
to test was side effect of the duplicated annotation.
   
   Since pod template file was what causing the issue, I thought best place to 
add the test is there.
   
   The reason I set the  `safeToEvict` of other services explicitly to True was 
not to relying on the default, after all all I wanted to test was if worker is  
false and the others are true, others should stay as true after being rendered.
   



-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to