Copilot commented on code in PR #65030:
URL: https://github.com/apache/airflow/pull/65030#discussion_r3066465285
##########
chart/templates/NOTES.txt:
##########
@@ -765,6 +765,14 @@ DEPRECATION WARNING:
{{- end }}
+{{- if not (empty .Values.workers.labels) }}
+
+ DEPRECATION WARNING:
+ `workers.labels` has been renamed to
`workers.celery.labels`/`workers.kubernetes.labels`.
+ Please change your values as support for the old name will be dropped in a
future release.
Review Comment:
This deprecation message says `workers.labels` has been “renamed”, but the
configuration is actually being split/deprecated in favor of two separate keys
(`workers.celery.labels` and `workers.kubernetes.labels`). Rewording this to
“deprecated; use …” (and clarifying users may need to set one or both depending
on executor) would make the warning more accurate and actionable.
##########
chart/docs/customizing-labels.rst:
##########
@@ -42,7 +42,7 @@ You can also set specific labels for individual Airflow
components, which will b
If the same label key exists in both global and component-specific labels, the
component-specific value takes precedence (overrides the global value).
This allows you to customize labels for specific components while still
maintaining common global labels across all resources.
-For example, to add specific labels to different components:
+For example, to add specific labels to different components like scheduler or
api-server:
Review Comment:
The worker-specific labels example was removed, but this PR introduces new
worker label keys (`workers.celery.labels` and `workers.kubernetes.labels`) and
deprecates `workers.labels`. Without an updated example or a note explaining
the new keys, this doc section no longer shows how to apply component-specific
labels to workers, which is likely to confuse users upgrading.
##########
helm-tests/tests/helm_tests/airflow_core/test_worker.py:
##########
@@ -2316,20 +2354,37 @@ def test_should_be_disabled_on_keda_enabled(self,
workers_values):
assert len(docs) == 1
- def test_should_add_component_specific_labels(self):
+ @pytest.mark.parametrize(
+ "workers_values",
+ [
+ {"celery": {"hpa": {"enabled": True}}, "labels": {"test_label":
"test_label_value"}},
+ {
+ "celery": {
+ "hpa": {"enabled": True},
+ "labels": {"test_label": "test_label_value"},
+ }
+ },
+ {
+ "labels": {"label": "value"},
+ "celery": {
+ "hpa": {"enabled": True},
+ "labels": {"test_label": "test_label_value"},
+ },
+ },
+ ],
+ )
+ def test_should_add_component_specific_labels(self, workers_values):
docs = render_chart(
values={
"executor": "CeleryExecutor",
- "workers": {
- "celery": {"hpa": {"enabled": True}},
- "labels": {"test_label": "test_label_value"},
- },
+ "workers": workers_values,
},
show_only=["templates/workers/worker-hpa.yaml"],
)
- assert "test_label" in jmespath.search("metadata.labels", docs[0])
- assert jmespath.search("metadata.labels", docs[0])["test_label"] ==
"test_label_value"
+ labels = jmespath.search("metadata.labels", docs[0])
+ assert labels["test_label"] == "test_label_value"
+ assert labels.get("labels") is None
Review Comment:
In this test, the final assertion checks `labels.get("labels") is None`,
which doesn’t correspond to the input values (the extra key used in the
parametrization is `"label"`). As written, the assertion will almost always
pass and won’t catch regressions where the deprecated `workers.labels` value
leaks into the rendered HPA labels. Update the assertion to validate the
absence of the intended key from the deprecated map.
--
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]
