Copilot commented on code in PR #65027:
URL: https://github.com/apache/airflow/pull/65027#discussion_r3066478506
##########
helm-tests/tests/helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -1043,29 +1043,82 @@ def test_safe_to_evict_annotation_other_services(self,
workers_values):
annotations =
jmespath.search("spec.template.metadata.annotations", doc)
assert
annotations.get("cluster-autoscaler.kubernetes.io/safe-to-evict") == "true"
- def test_workers_pod_annotations(self):
+ def test_global_pod_annotations_templated(self):
docs = render_chart(
- values={"workers": {"podAnnotations": {"my_annotation":
"annotated!"}}},
+ values={"airflowPodAnnotations": {"global": "{{ .Release.Name
}}"}},
show_only=["templates/pod-template-file.yaml"],
chart_dir=self.temp_chart_dir,
)
+
annotations = jmespath.search("metadata.annotations", docs[0])
- assert "my_annotation" in annotations
- assert "annotated!" in annotations["my_annotation"]
+ assert len(annotations) == 2 # safe-to-evict + extra annotation
+ assert annotations["global"] == "release-name"
- def test_airflow_and_workers_pod_annotations(self):
+ @pytest.mark.parametrize(
+ "workers_values",
+ [
+ {"podAnnotations": {"my_annotation": "{{ .Release.Name }}"}},
+ {"kubernetes": {"podAnnotations": {"my_annotation": "{{
.Release.Name }}"}}},
+ {
+ "podAnnotations": {"test": "test"},
+ "kubernetes": {"podAnnotations": {"my_annotation": "{{
.Release.Name }}"}},
+ },
+ ],
+ )
+ def test_workers_pod_annotations_templated(self, workers_values):
+ docs = render_chart(
+ values={"workers": workers_values},
+ show_only=["templates/pod-template-file.yaml"],
+ chart_dir=self.temp_chart_dir,
+ )
+
+ annotations = jmespath.search("metadata.annotations", docs[0])
+ assert len(annotations) == 2 # safe-to-evict + extra annotation
Review Comment:
Asserting the total annotation count makes these tests brittle (they will
fail if the chart adds another default annotation). Prefer asserting only the
required keys/values are present (and, when relevant, that specific keys are
absent) rather than asserting `len(annotations)`.
##########
chart/files/pod-template-file.kubernetes-helm-yaml:
##########
@@ -29,7 +29,7 @@
{{- $containerSecurityContext := include "containerSecurityContext" (list
.Values.workers.kubernetes .Values.workers .Values) }}
{{- $containerLifecycleHooks := or
.Values.workers.kubernetes.containerLifecycleHooks
.Values.workers.containerLifecycleHooks .Values.containerLifecycleHooks }}
{{- $safeToEvict := dict "cluster-autoscaler.kubernetes.io/safe-to-evict" (or
.Values.workers.kubernetes.safeToEvict (and (not (has
.Values.workers.kubernetes.safeToEvict (list true false)))
.Values.workers.safeToEvict) | toString) }}
-{{- $podAnnotations := mergeOverwrite (deepCopy .Values.airflowPodAnnotations)
$safeToEvict .Values.workers.podAnnotations }}
+{{- $podAnnotations := mergeOverwrite (deepCopy .Values.airflowPodAnnotations)
$safeToEvict (.Values.workers.kubernetes.podAnnotations | default
.Values.workers.podAnnotations) }}
Review Comment:
Using `default` here makes an explicitly empty
`workers.kubernetes.podAnnotations: {}` fall back to the deprecated
`workers.podAnnotations`, which prevents users from intentionally clearing
pod-template-file annotations while still having legacy values set. Prefer
checking key presence (e.g., via `hasKey` on `.Values.workers.kubernetes`) to
distinguish 'unset' from 'set to empty', and only fall back to
`workers.podAnnotations` when the new key is not provided.
```suggestion
{{- $workerPodAnnotations := .Values.workers.podAnnotations }}
{{- if hasKey .Values.workers.kubernetes "podAnnotations" }}
{{- $workerPodAnnotations = .Values.workers.kubernetes.podAnnotations }}
{{- end }}
{{- $podAnnotations := mergeOverwrite (deepCopy
.Values.airflowPodAnnotations) $safeToEvict $workerPodAnnotations }}
```
##########
helm-tests/tests/helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -1043,29 +1043,82 @@ def test_safe_to_evict_annotation_other_services(self,
workers_values):
annotations =
jmespath.search("spec.template.metadata.annotations", doc)
assert
annotations.get("cluster-autoscaler.kubernetes.io/safe-to-evict") == "true"
- def test_workers_pod_annotations(self):
+ def test_global_pod_annotations_templated(self):
docs = render_chart(
- values={"workers": {"podAnnotations": {"my_annotation":
"annotated!"}}},
+ values={"airflowPodAnnotations": {"global": "{{ .Release.Name
}}"}},
show_only=["templates/pod-template-file.yaml"],
chart_dir=self.temp_chart_dir,
)
+
annotations = jmespath.search("metadata.annotations", docs[0])
- assert "my_annotation" in annotations
- assert "annotated!" in annotations["my_annotation"]
+ assert len(annotations) == 2 # safe-to-evict + extra annotation
Review Comment:
Asserting the total annotation count makes these tests brittle (they will
fail if the chart adds another default annotation). Prefer asserting only the
required keys/values are present (and, when relevant, that specific keys are
absent) rather than asserting `len(annotations)`.
##########
helm-tests/tests/helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -1043,29 +1043,82 @@ def test_safe_to_evict_annotation_other_services(self,
workers_values):
annotations =
jmespath.search("spec.template.metadata.annotations", doc)
assert
annotations.get("cluster-autoscaler.kubernetes.io/safe-to-evict") == "true"
- def test_workers_pod_annotations(self):
+ def test_global_pod_annotations_templated(self):
docs = render_chart(
- values={"workers": {"podAnnotations": {"my_annotation":
"annotated!"}}},
+ values={"airflowPodAnnotations": {"global": "{{ .Release.Name
}}"}},
show_only=["templates/pod-template-file.yaml"],
chart_dir=self.temp_chart_dir,
)
+
annotations = jmespath.search("metadata.annotations", docs[0])
- assert "my_annotation" in annotations
- assert "annotated!" in annotations["my_annotation"]
+ assert len(annotations) == 2 # safe-to-evict + extra annotation
+ assert annotations["global"] == "release-name"
- def test_airflow_and_workers_pod_annotations(self):
+ @pytest.mark.parametrize(
+ "workers_values",
+ [
+ {"podAnnotations": {"my_annotation": "{{ .Release.Name }}"}},
+ {"kubernetes": {"podAnnotations": {"my_annotation": "{{
.Release.Name }}"}}},
+ {
+ "podAnnotations": {"test": "test"},
+ "kubernetes": {"podAnnotations": {"my_annotation": "{{
.Release.Name }}"}},
+ },
+ ],
+ )
+ def test_workers_pod_annotations_templated(self, workers_values):
+ docs = render_chart(
+ values={"workers": workers_values},
+ show_only=["templates/pod-template-file.yaml"],
+ chart_dir=self.temp_chart_dir,
+ )
+
+ annotations = jmespath.search("metadata.annotations", docs[0])
+ assert len(annotations) == 2 # safe-to-evict + extra annotation
+ assert annotations["my_annotation"] == "release-name"
+
+ @pytest.mark.parametrize(
+ "workers_values",
+ [
+ {"podAnnotations": {"my_annotation": "workerPodAnnotations"}},
+ {"kubernetes": {"podAnnotations": {"my_annotation":
"workerPodAnnotations"}}},
+ ],
+ )
+ def test_workers_pod_annotations_override(self, workers_values):
# should give preference to workers.podAnnotations
Review Comment:
This comment is now misleading because the test is parametrized to cover
both `workers.podAnnotations` and `workers.kubernetes.podAnnotations`. Consider
updating it to reflect the actual intent (e.g., worker-specific annotations
override `airflowPodAnnotations`, regardless of whether they come from the
deprecated key or the new kubernetes key).
```suggestion
# Worker-specific pod annotations should override
airflowPodAnnotations,
# whether they come from workers.podAnnotations or
workers.kubernetes.podAnnotations.
```
##########
helm-tests/tests/helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -1043,29 +1043,82 @@ def test_safe_to_evict_annotation_other_services(self,
workers_values):
annotations =
jmespath.search("spec.template.metadata.annotations", doc)
assert
annotations.get("cluster-autoscaler.kubernetes.io/safe-to-evict") == "true"
- def test_workers_pod_annotations(self):
+ def test_global_pod_annotations_templated(self):
docs = render_chart(
- values={"workers": {"podAnnotations": {"my_annotation":
"annotated!"}}},
+ values={"airflowPodAnnotations": {"global": "{{ .Release.Name
}}"}},
show_only=["templates/pod-template-file.yaml"],
chart_dir=self.temp_chart_dir,
)
+
annotations = jmespath.search("metadata.annotations", docs[0])
- assert "my_annotation" in annotations
- assert "annotated!" in annotations["my_annotation"]
+ assert len(annotations) == 2 # safe-to-evict + extra annotation
+ assert annotations["global"] == "release-name"
- def test_airflow_and_workers_pod_annotations(self):
+ @pytest.mark.parametrize(
+ "workers_values",
+ [
+ {"podAnnotations": {"my_annotation": "{{ .Release.Name }}"}},
+ {"kubernetes": {"podAnnotations": {"my_annotation": "{{
.Release.Name }}"}}},
+ {
+ "podAnnotations": {"test": "test"},
+ "kubernetes": {"podAnnotations": {"my_annotation": "{{
.Release.Name }}"}},
+ },
+ ],
+ )
+ def test_workers_pod_annotations_templated(self, workers_values):
+ docs = render_chart(
+ values={"workers": workers_values},
+ show_only=["templates/pod-template-file.yaml"],
+ chart_dir=self.temp_chart_dir,
+ )
+
+ annotations = jmespath.search("metadata.annotations", docs[0])
+ assert len(annotations) == 2 # safe-to-evict + extra annotation
+ assert annotations["my_annotation"] == "release-name"
+
+ @pytest.mark.parametrize(
+ "workers_values",
+ [
+ {"podAnnotations": {"my_annotation": "workerPodAnnotations"}},
+ {"kubernetes": {"podAnnotations": {"my_annotation":
"workerPodAnnotations"}}},
+ ],
+ )
+ def test_workers_pod_annotations_override(self, workers_values):
# should give preference to workers.podAnnotations
docs = render_chart(
values={
"airflowPodAnnotations": {"my_annotation":
"airflowPodAnnotations"},
- "workers": {"podAnnotations": {"my_annotation":
"workerPodAnnotations"}},
+ "workers": workers_values,
},
show_only=["templates/pod-template-file.yaml"],
chart_dir=self.temp_chart_dir,
)
+
annotations = jmespath.search("metadata.annotations", docs[0])
- assert "my_annotation" in annotations
- assert "workerPodAnnotations" in annotations["my_annotation"]
+ assert len(annotations) == 2 # safe-to-evict + extra annotation
Review Comment:
Asserting the total annotation count makes these tests brittle (they will
fail if the chart adds another default annotation). Prefer asserting only the
required keys/values are present (and, when relevant, that specific keys are
absent) rather than asserting `len(annotations)`.
##########
helm-tests/tests/helm_tests/airflow_aux/test_pod_template_file.py:
##########
@@ -1043,29 +1043,82 @@ def test_safe_to_evict_annotation_other_services(self,
workers_values):
annotations =
jmespath.search("spec.template.metadata.annotations", doc)
assert
annotations.get("cluster-autoscaler.kubernetes.io/safe-to-evict") == "true"
- def test_workers_pod_annotations(self):
+ def test_global_pod_annotations_templated(self):
docs = render_chart(
- values={"workers": {"podAnnotations": {"my_annotation":
"annotated!"}}},
+ values={"airflowPodAnnotations": {"global": "{{ .Release.Name
}}"}},
show_only=["templates/pod-template-file.yaml"],
chart_dir=self.temp_chart_dir,
)
+
annotations = jmespath.search("metadata.annotations", docs[0])
- assert "my_annotation" in annotations
- assert "annotated!" in annotations["my_annotation"]
+ assert len(annotations) == 2 # safe-to-evict + extra annotation
+ assert annotations["global"] == "release-name"
- def test_airflow_and_workers_pod_annotations(self):
+ @pytest.mark.parametrize(
+ "workers_values",
+ [
+ {"podAnnotations": {"my_annotation": "{{ .Release.Name }}"}},
+ {"kubernetes": {"podAnnotations": {"my_annotation": "{{
.Release.Name }}"}}},
+ {
+ "podAnnotations": {"test": "test"},
+ "kubernetes": {"podAnnotations": {"my_annotation": "{{
.Release.Name }}"}},
+ },
+ ],
+ )
+ def test_workers_pod_annotations_templated(self, workers_values):
+ docs = render_chart(
+ values={"workers": workers_values},
+ show_only=["templates/pod-template-file.yaml"],
+ chart_dir=self.temp_chart_dir,
+ )
+
+ annotations = jmespath.search("metadata.annotations", docs[0])
+ assert len(annotations) == 2 # safe-to-evict + extra annotation
+ assert annotations["my_annotation"] == "release-name"
+
+ @pytest.mark.parametrize(
+ "workers_values",
+ [
+ {"podAnnotations": {"my_annotation": "workerPodAnnotations"}},
+ {"kubernetes": {"podAnnotations": {"my_annotation":
"workerPodAnnotations"}}},
+ ],
+ )
+ def test_workers_pod_annotations_override(self, workers_values):
# should give preference to workers.podAnnotations
docs = render_chart(
values={
"airflowPodAnnotations": {"my_annotation":
"airflowPodAnnotations"},
- "workers": {"podAnnotations": {"my_annotation":
"workerPodAnnotations"}},
+ "workers": workers_values,
},
show_only=["templates/pod-template-file.yaml"],
chart_dir=self.temp_chart_dir,
)
+
annotations = jmespath.search("metadata.annotations", docs[0])
- assert "my_annotation" in annotations
- assert "workerPodAnnotations" in annotations["my_annotation"]
+ assert len(annotations) == 2 # safe-to-evict + extra annotation
+ assert annotations["my_annotation"] == "workerPodAnnotations"
+
+ @pytest.mark.parametrize(
+ "workers_values",
+ [
+ {"podAnnotations": {"local": "workerPodAnnotations"}},
+ {"kubernetes": {"podAnnotations": {"local":
"workerPodAnnotations"}}},
+ ],
+ )
+ def test_pod_annotations_merge(self, workers_values):
+ docs = render_chart(
+ values={
+ "airflowPodAnnotations": {"global": "airflowPodAnnotations"},
+ "workers": workers_values,
+ },
+ show_only=["templates/pod-template-file.yaml"],
+ chart_dir=self.temp_chart_dir,
+ )
+
+ annotations = jmespath.search("metadata.annotations", docs[0])
+ assert len(annotations) == 3 # safe-to-evict + extra annotations
Review Comment:
Asserting the total annotation count makes these tests brittle (they will
fail if the chart adds another default annotation). Prefer asserting only the
required keys/values are present (and, when relevant, that specific keys are
absent) rather than asserting `len(annotations)`.
```suggestion
assert "global" in annotations
assert "local" in annotations
```
--
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]
