potiuk commented on code in PR #65866:
URL: https://github.com/apache/airflow/pull/65866#discussion_r3143696862
##########
chart/templates/_helpers.yaml:
##########
@@ -97,28 +97,21 @@ If release name contains chart name it will be used as a
full name.
name: {{ template "airflow_metadata_secret" . }}
key: kedaConnection
{{- end }}
- {{- if and (semverCompare "<3.0.0" .Values.airflowVersion)
.Values.enableBuiltInSecretEnvVars.AIRFLOW__WEBSERVER__SECRET_KEY }}
- - name: AIRFLOW__WEBSERVER__SECRET_KEY
- valueFrom:
- secretKeyRef:
- name: {{ template "webserver_secret_key_secret" . }}
- key: webserver-secret-key
- {{- end }}
- {{- if and (semverCompare ">=3.0.0" .Values.airflowVersion)
.Values.enableBuiltInSecretEnvVars.AIRFLOW__API__SECRET_KEY }}
+ {{- if and .Values.enableBuiltInSecretEnvVars.AIRFLOW__API__SECRET_KEY }}
Review Comment:
Redundant `and` with a single operand — the version guard is gone, so this
can drop the `and`:
```suggestion
{{- if .Values.enableBuiltInSecretEnvVars.AIRFLOW__API__SECRET_KEY }}
```
##########
chart/templates/scheduler/scheduler-service.yaml:
##########
@@ -17,10 +17,10 @@
under the License.
*/}}
-################################
+############################
## Airflow Scheduler Service
-#################################
-{{- if and .Values.scheduler.enabled (or (contains "LocalExecutor"
.Values.executor) (contains "LocalKubernetesExecutor" .Values.executor)) }}
+############################
+{{- if and .Values.scheduler.enabled (or (contains "LocalExecutor"
.Values.executor)) }}
Review Comment:
The `or` wrapper around a single `contains` is no longer doing anything (it
used to also accept `LocalKubernetesExecutor`):
```suggestion
{{- if and .Values.scheduler.enabled (contains "LocalExecutor"
.Values.executor) }}
```
##########
helm-tests/tests/helm_tests/airflow_core/test_worker.py:
##########
@@ -2404,104 +2384,19 @@ def test_should_remove_replicas_field(self):
assert "replicas" not in jmespath.search("spec", docs[0])
- @pytest.mark.parametrize(
- ("executor", "expected_query"),
- [
- (
- "CeleryExecutor",
- "SELECT ceil(COUNT(*)::decimal / 16) FROM task_instance WHERE
(state='running' OR state='queued') AND queue IN ('default')",
- ),
- (
- "CeleryKubernetesExecutor",
- "SELECT ceil(COUNT(*)::decimal / 16) FROM task_instance WHERE
(state='running' OR state='queued') AND queue IN ('default') AND queue !=
'kubernetes'",
- ),
- ],
- )
- def test_keda_query_default(self, executor, expected_query):
+ def test_keda_query_default(self):
docs = render_chart(
values={
- "executor": executor,
+ "executor": "CeleryExecutor",
"workers": {
"celery": {"keda": {"enabled": True}},
},
},
show_only=["templates/workers/worker-kedaautoscaler.yaml"],
)
- assert expected_query ==
jmespath.search("spec.triggers[0].metadata.query", docs[0])
-
- @pytest.mark.parametrize(
- ("workers_values", "executor"),
- [
- # workers - test custom static query
- (
- {
- "keda": {
- "enabled": True,
- "query": "SELECT ceil(COUNT(*)::decimal / 32) FROM
task_instance",
- }
- },
- "CeleryKubernetesExecutor",
- ),
- # workers - test custom template query
- (
- {
- "keda": {
- "enabled": True,
- "query": "SELECT ceil(COUNT(*)::decimal / {{ mul
.Values.config.celery.worker_concurrency 2 }}) FROM task_instance",
- }
- },
- "CeleryKubernetesExecutor",
- ),
- # workers.celery - test custom static query
- (
- {
- "celery": {
- "keda": {
- "enabled": True,
- "query": "SELECT ceil(COUNT(*)::decimal / 32) FROM
task_instance",
- }
- }
- },
- "CeleryKubernetesExecutor",
- ),
- # workers.celery - test custom template query
- (
- {
- "celery": {
- "keda": {
- "enabled": True,
- "query": "SELECT ceil(COUNT(*)::decimal / {{ mul
.Values.config.celery.worker_concurrency 2 }}) FROM task_instance",
- }
- }
- },
- "CeleryKubernetesExecutor",
- ),
- # workers - workers.celery overwrite
- (
- {
- "keda": {"query": "SELECT ceil(COUNT(*)::decimal / 16)
FROM task_instance"},
- "celery": {
- "keda": {
- "enabled": True,
- "query": "SELECT ceil(COUNT(*)::decimal / 32) FROM
task_instance",
- }
- },
- },
- "CeleryKubernetesExecutor",
- ),
- ],
- )
- def test_keda_query_overwrite(self, workers_values, executor):
- docs = render_chart(
- values={
- "executor": executor,
- "workers": workers_values,
- },
- show_only=["templates/workers/worker-kedaautoscaler.yaml"],
- )
assert (
jmespath.search("spec.triggers[0].metadata.query", docs[0])
- == "SELECT ceil(COUNT(*)::decimal / 32) FROM task_instance"
+ == "SELECT ceil(COUNT(*)::decimal / 16) FROM task_instance WHERE
(state='running' OR state='queued') AND queue IN ('default')"
)
Review Comment:
Suggestion to restore the `test_keda_query_overwrite` coverage that was
dropped along with `CeleryKubernetesExecutor` — the override paths
(`workers.keda.query`, `workers.celery.keda.query`, and the precedence between
them) are still supported and have a deprecation notice in
`chart/templates/NOTES.txt` pointing users at the rename. Restoring with
`CeleryExecutor` keeps the cases meaningful:
```suggestion
)
@pytest.mark.parametrize(
"workers_values",
[
# workers - custom static query
{
"keda": {
"enabled": True,
"query": "SELECT ceil(COUNT(*)::decimal / 32) FROM
task_instance",
}
},
# workers - custom template query
{
"keda": {
"enabled": True,
"query": "SELECT ceil(COUNT(*)::decimal / {{ mul
.Values.config.celery.worker_concurrency 2 }}) FROM task_instance",
}
},
# workers.celery - custom static query
{
"celery": {
"keda": {
"enabled": True,
"query": "SELECT ceil(COUNT(*)::decimal / 32) FROM
task_instance",
}
}
},
# workers.celery - custom template query
{
"celery": {
"keda": {
"enabled": True,
"query": "SELECT ceil(COUNT(*)::decimal / {{ mul
.Values.config.celery.worker_concurrency 2 }}) FROM task_instance",
}
}
},
# workers.celery overrides workers when both are set
{
"keda": {"query": "SELECT ceil(COUNT(*)::decimal / 16) FROM
task_instance"},
"celery": {
"keda": {
"enabled": True,
"query": "SELECT ceil(COUNT(*)::decimal / 32) FROM
task_instance",
}
},
},
],
)
def test_keda_query_overwrite(self, workers_values):
docs = render_chart(
values={
"executor": "CeleryExecutor",
"workers": workers_values,
},
show_only=["templates/workers/worker-kedaautoscaler.yaml"],
)
assert (
jmespath.search("spec.triggers[0].metadata.query", docs[0])
== "SELECT ceil(COUNT(*)::decimal / 32) FROM task_instance"
)
```
##########
chart/newsfragments/65866.significant.rst:
##########
@@ -0,0 +1,3 @@
+Dropped support for Airflow < 3.1.0
+
+If you want to use the Chart for older versions you need to use one of the
charts from the version 1.x-line.
Review Comment:
Suggested fixup: enumerate the removed values and their replacements so
users editing `values.yaml` get a usable migration map instead of a
schema-level "additional property not allowed" error.
```suggestion
Dropped support for Airflow < 3.1.0
The chart now requires Apache Airflow 3.1.0 or above. If you need to deploy
older versions, use a chart from the 1.x line.
The following values are no longer accepted and must be replaced:
- ``webserver.*`` (entire section) → use ``apiServer.*``
- ``webserver.defaultUser.*`` → use ``createUserJob.defaultUser.*``
- ``webserver.allowPodLogReading`` → use ``apiServer.allowPodLogReading``
- ``webserverSecretKey`` → use ``apiSecretKey``
- ``webserverSecretKeySecretName`` → use ``apiSecretKeySecretName``
- ``webserverSecretAnnotations`` → use ``apiSecretAnnotations``
- ``enableBuiltInSecretEnvVars.AIRFLOW__WEBSERVER__SECRET_KEY`` → use
``enableBuiltInSecretEnvVars.AIRFLOW__API__SECRET_KEY``
The following executors are no longer supported:
- ``LocalKubernetesExecutor``
- ``CeleryKubernetesExecutor``
Other behavior changes:
- ``dagProcessor.enabled`` default is now ``true`` (was ``null``, which
previously resolved to ``true`` on Airflow 3 anyway).
- ``config.webserver.enable_proxy_fix`` and
``config.scheduler.standalone_dag_processor`` defaults removed from
``values.yaml``.
```
--
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]
