Copilot commented on code in PR #64980:
URL: https://github.com/apache/airflow/pull/64980#discussion_r3066477962
##########
chart/templates/NOTES.txt:
##########
@@ -741,6 +741,14 @@ DEPRECATION WARNING:
{{- end }}
+{{- if not (empty .Values.workers.topologySpreadConstraints) }}
+
+ DEPRECATION WARNING:
+ `workers.topologySpreadConstraints` has been renamed to
`workers.celery.topologySpreadConstraints`/`workers.kubernetes.topologySpreadConstraints`.
+ Please change your values as support for the old name will be dropped in a
future release.
Review Comment:
The deprecation warning uses a slash (`...celery...`/`...kubernetes...`)
which can read like a literal path rather than two alternatives. Prefer wording
that mirrors the PR description (e.g., '... renamed; use
`workers.celery.topologySpreadConstraints` and/or
`workers.kubernetes.topologySpreadConstraints`') to reduce ambiguity for users.
##########
chart/values.schema.json:
##########
@@ -2372,7 +2372,7 @@
}
},
"topologySpreadConstraints": {
- "description": "Specify topology spread constraints for
Airflow Celery worker pods and pods created with pod-template-file.",
+ "description": "Specify topology spread constraints for
Airflow Celery worker pods and pods created with pod-template-file (deprecated,
use ``workers.celery.topologySpreadConstraints`` and/or
``workers.kubernetes.topologySpreadConstraints`` instead).",
Review Comment:
The schema description uses double backticks (``...``), which is
reStructuredText-style and may render oddly/inconsistently in tooling that
expects plain text/JSON Schema descriptions. Consider switching to single
backticks (or no backticks) consistent with other schema descriptions in the
chart, and optionally add a JSON Schema `deprecated: true` marker for
machine-readable deprecation.
```suggestion
"description": "Specify topology spread constraints for
Airflow Celery worker pods and pods created with pod-template-file (deprecated,
use `workers.celery.topologySpreadConstraints` and/or
`workers.kubernetes.topologySpreadConstraints` instead).",
"deprecated": true,
```
##########
helm-tests/tests/helm_tests/airflow_core/test_worker.py:
##########
@@ -664,23 +664,67 @@ def test_tolerations(self):
{"key": "dynamic-pods", "operator": "Equal", "value": "true",
"effect": "NoSchedule"}
]
- def test_topology_spread_constraints(self):
- expected_topology_spread_constraints = [
+ @pytest.mark.parametrize(
+ "workers_values",
+ [
+ {
+ "topologySpreadConstraints": [
+ {
+ "maxSkew": 1,
+ "topologyKey": "foo",
+ "whenUnsatisfiable": "ScheduleAnyway",
+ "labelSelector": {"matchLabels": {"tier": "airflow"}},
+ }
+ ]
+ },
+ {
+ "celery": {
+ "topologySpreadConstraints": [
+ {
+ "maxSkew": 1,
+ "topologyKey": "foo",
+ "whenUnsatisfiable": "ScheduleAnyway",
+ "labelSelector": {"matchLabels": {"tier":
"airflow"}},
+ }
+ ]
+ }
+ },
Review Comment:
The topology spread constraint object is repeated multiple times within the
parametrization. To make future edits less error-prone, consider extracting the
repeated constraint dict/list into a shared constant (or fixture) and reusing
it across the parametrized cases.
--
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]
