You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2026/04/10 20:03:51 UTC
Re: [PR] Enable git-sync toggle for Airflow components, refactor Helm chart logic [airflow]
Copilot commented on code in PR #49022:
URL: https://github.com/apache/airflow/pull/49022#discussion_r3066497351
##########
chart/templates/_helpers.yaml:
##########
@@ -343,6 +349,46 @@ If release name contains chart name it will be used as a full name.
{{- end }}
{{- end }}
+{{/* Container for waiting for Airflow migrations */}}
+{{- define "wait_for_airflow_migrations_container" -}}
+{{- $ := index . 0 -}}
+{{- $component := index . 1 -}}
+{{- $componentValues := index $.Values $component -}}
+{{- $containerSecurityContextWaitForMigrations := include "containerSecurityContext" (list $ $componentValues.waitForMigrations) -}}
+{{- if $componentValues.waitForMigrations.enabled }}
+- name: wait-for-airflow-migrations
+ resources: {{- toYaml $componentValues.resources | nindent 4 }}
+ image: {{ template "airflow_image_for_migrations" $ }}
+ imagePullPolicy: {{ $.Values.images.airflow.pullPolicy }}
+ securityContext: {{ $containerSecurityContextWaitForMigrations | nindent 4 }}
+ volumeMounts:
+ - name: logs
+ mountPath: {{ template "airflow_logs" $ }}
+ {{- if $.Values.logs.persistence.subPath }}
+ subPath: {{ $.Values.logs.persistence.subPath }}
+ {{- end }}
Review Comment:
`wait_for_airflow_migrations_container` always mounts a `logs` volume, but some components using this helper (notably apiServer and webserver) only define the `logs` volume when `logs.persistence.enabled` is true. With default `waitForMigrations.enabled: true` and default `logs.persistence.enabled: false`, this produces an invalid Pod spec (volumeMount references a missing volume). Make the `logs` volumeMount conditional on the volume being present (or ensure the volume is always defined for these components).
```suggestion
volumeMounts:
{{- if $.Values.logs.persistence.enabled }}
- name: logs
mountPath: {{ template "airflow_logs" $ }}
{{- if $.Values.logs.persistence.subPath }}
subPath: {{ $.Values.logs.persistence.subPath }}
{{- end }}
{{- end }}
```
##########
chart/templates/_helpers.yaml:
##########
@@ -343,6 +349,46 @@ If release name contains chart name it will be used as a full name.
{{- end }}
{{- end }}
+{{/* Container for waiting for Airflow migrations */}}
+{{- define "wait_for_airflow_migrations_container" -}}
+{{- $ := index . 0 -}}
+{{- $component := index . 1 -}}
+{{- $componentValues := index $.Values $component -}}
+{{- $containerSecurityContextWaitForMigrations := include "containerSecurityContext" (list $ $componentValues.waitForMigrations) -}}
+{{- if $componentValues.waitForMigrations.enabled }}
+- name: wait-for-airflow-migrations
+ resources: {{- toYaml $componentValues.resources | nindent 4 }}
+ image: {{ template "airflow_image_for_migrations" $ }}
+ imagePullPolicy: {{ $.Values.images.airflow.pullPolicy }}
+ securityContext: {{ $containerSecurityContextWaitForMigrations | nindent 4 }}
+ volumeMounts:
+ - name: logs
+ mountPath: {{ template "airflow_logs" $ }}
+ {{- if $.Values.logs.persistence.subPath }}
+ subPath: {{ $.Values.logs.persistence.subPath }}
+ {{- end }}
+ {{- include "airflow_config_mount" $ | nindent 4 }}
+ {{- include "airflow_dags_mount" (list $ $component) | nindent 4 }}
+ {{- if $.Values.volumeMounts }}
+ {{- toYaml $.Values.volumeMounts | nindent 4 }}
+ {{- end }}
+ {{- if $componentValues.extraVolumeMounts }}
+ {{- tpl (toYaml $componentValues.extraVolumeMounts) $ | nindent 4 }}
+ {{- end }}
+ {{- if or $.Values.webserver.webserverConfig $.Values.webserver.webserverConfigConfigMapName }}
Review Comment:
This helper conditionally mounts `webserver-config` based on global webserver settings, but the apiServer (and potentially other components using this helper) does not define a `webserver-config` volume. If `webserver.webserverConfig*` is set, the wait-for-migrations init container will reference a missing volume and fail to render. Gate this mount on the current component having the volume (e.g., exclude apiServer, or pass in component-specific mount options).
```suggestion
{{- if and (ne $component "apiServer") (or $.Values.webserver.webserverConfig $.Values.webserver.webserverConfigConfigMapName) }}
```
##########
helm-tests/tests/helm_tests/other/test_git_sync_scheduler.py:
##########
@@ -266,23 +273,24 @@ def test_validate_if_ssh_params_are_added_with_git_ssh_key(self):
"secret": {"secretName": "release-name-ssh-secret", "defaultMode": 288},
} in jmespath.search("spec.template.spec.volumes", docs[0])
- def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self):
- docs = render_chart(
- values={
- "dags": {
- "gitSync": {
- "enabled": True,
- "containerName": "git-sync-test",
- "sshKeySecret": "ssh-secret",
- "knownHosts": None,
- "branch": "test-branch",
- },
- "persistence": {"enabled": True},
- }
- },
- show_only=["templates/scheduler/scheduler-deployment.yaml"],
- )
- assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0])
+ # def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self):
+ # docs = render_chart(
+ # values={
+ # "dags": {
+ # "gitSync": {
+ # "enabled": True,
+ # "components": {"scheduler": True},
+ # "containerName": "git-sync-test",
+ # "sshKeySecret": "ssh-secret",
+ # "knownHosts": None,
+ # "branch": "test-branch",
+ # },
+ # "persistence": {"enabled": True},
+ # }
+ # },
+ # show_only=["templates/scheduler/scheduler-deployment.yaml"],
+ # )
+ # assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0])
Review Comment:
A previously active test is now commented out. Commented-out tests reduce coverage and can hide regressions (here, behavior around not mounting the SSH key secret when DAG persistence is enabled). Please either re-enable the test with updated expectations/values, or delete it and add a replacement that validates the intended new behavior.
```suggestion
def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self):
docs = render_chart(
values={
"airflowVersion": "2.11.0",
"dags": {
"gitSync": {
"enabled": True,
"components": {"scheduler": True},
"containerName": "git-sync-test",
"sshKeySecret": "ssh-secret",
"knownHosts": None,
"branch": "test-branch",
},
"persistence": {"enabled": True},
},
},
show_only=["templates/scheduler/scheduler-deployment.yaml"],
)
assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0])
```
##########
chart/templates/_helpers.yaml:
##########
@@ -577,17 +623,43 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{- end }}
{{- end }}
+{{- define "airflow_dags_volume" -}}
+{{- $ := index . 0 -}}
+{{- $component := index . 1 -}}
+{{- $gitSync := $.Values.dags.gitSync -}}
+{{- $enabledGitSyncForComponent := index $gitSync.components $component -}}
+{{- if $.Values.dags.persistence.enabled }}
+- name: dags
+ persistentVolumeClaim:
+ claimName: {{ template "airflow_dags_volume_claim" $ }}
+{{- else if and $gitSync.enabled $enabledGitSyncForComponent }}
+- name: dags
+ emptyDir: {{- toYaml (default (dict) $gitSync.emptyDirConfig) | nindent 4 }}
+{{- end }}
+{{- end }}
+
{{- define "airflow_dags_mount" -}}
+{{- $ := index . 0 -}}
+{{- $component := index . 1 -}}
+# TODO: It appears that localMode is not necessary anymore. If this is true, remove its usage from the definition.
+{{- $localMode := false -}}
+{{- if gt (len .) 2 -}}
+ {{- $localMode := index . 2 -}}
Review Comment:
The `localMode` argument handling is broken: `{{- $localMode := index . 2 -}}` re-declares a new scoped variable and does not update the outer `$localMode`, so passing a third parameter has no effect. Use assignment (`=`) instead of re-declaration (`:=`), or remove the unused parameter support entirely if it’s no longer needed.
```suggestion
{{- $localMode = index . 2 -}}
```
##########
chart/templates/dag-processor/dag-processor-deployment.yaml:
##########
@@ -117,32 +117,10 @@ spec:
securityContext: {{ $securityContext | nindent 8 }}
imagePullSecrets: {{ include "image_pull_secrets" . | nindent 8 }}
initContainers:
- {{- if .Values.dagProcessor.waitForMigrations.enabled }}
- - name: wait-for-airflow-migrations
- resources: {{- toYaml .Values.dagProcessor.resources | nindent 12 }}
- image: {{ template "airflow_image_for_migrations" . }}
- imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
- securityContext: {{ $containerSecurityContextWaitForMigrations | nindent 12 }}
- volumeMounts:
- {{- if .Values.volumeMounts }}
- {{- toYaml .Values.volumeMounts | nindent 12 }}
- {{- end }}
- {{- if .Values.dagProcessor.extraVolumeMounts }}
- {{- tpl (toYaml .Values.dagProcessor.extraVolumeMounts) . | nindent 12 }}
- {{- end }}
- {{- include "airflow_config_mount" . | nindent 12 }}
- args: {{- include "wait-for-migrations-command" . | indent 10 }}
- envFrom: {{- include "custom_airflow_environment_from" . | default "\n []" | indent 10 }}
- env:
- {{- include "custom_airflow_environment" . | indent 10 }}
- {{- include "standard_airflow_environment" (merge (dict "IncludeJwtSecret" false) .) | indent 10 }}
- {{- if .Values.dagProcessor.waitForMigrations.env }}
- {{- tpl (toYaml .Values.dagProcessor.waitForMigrations.env) $ | nindent 12 }}
- {{- end }}
- {{- end }}
- {{- if and .Values.dags.gitSync.enabled (not .Values.dags.persistence.enabled) }}
+ {{- if and .Values.dags.gitSync.enabled .Values.dags.gitSync.components.dagProcessor }}
{{- include "git_sync_container" (dict "Values" .Values "is_init" "true" "Template" .Template) | nindent 8 }}
{{- end }}
Review Comment:
The dag-processor now includes the git-sync init container whenever `dags.gitSync.enabled` and `components.dagProcessor` are true, even if `dags.persistence.enabled` is also true. This changes prior behavior (and conflicts with the new tests expecting no git-sync when persistence is enabled) and can lead to git-sync writing into a PVC that the user expects to manage themselves. Either restore the `not dags.persistence.enabled` guard here (consistent with other components), or update the chart behavior + tests/documentation to explicitly support git-sync with persistent DAGs for dagProcessor.
##########
chart/templates/_helpers.yaml:
##########
@@ -577,17 +623,43 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{- end }}
{{- end }}
+{{- define "airflow_dags_volume" -}}
+{{- $ := index . 0 -}}
+{{- $component := index . 1 -}}
+{{- $gitSync := $.Values.dags.gitSync -}}
+{{- $enabledGitSyncForComponent := index $gitSync.components $component -}}
+{{- if $.Values.dags.persistence.enabled }}
+- name: dags
+ persistentVolumeClaim:
+ claimName: {{ template "airflow_dags_volume_claim" $ }}
+{{- else if and $gitSync.enabled $enabledGitSyncForComponent }}
+- name: dags
+ emptyDir: {{- toYaml (default (dict) $gitSync.emptyDirConfig) | nindent 4 }}
+{{- end }}
+{{- end }}
+
{{- define "airflow_dags_mount" -}}
+{{- $ := index . 0 -}}
+{{- $component := index . 1 -}}
+# TODO: It appears that localMode is not necessary anymore. If this is true, remove its usage from the definition.
+{{- $localMode := false -}}
+{{- if gt (len .) 2 -}}
+ {{- $localMode := index . 2 -}}
+{{- end -}}
+{{- $gitSync := $.Values.dags.gitSync -}}
+{{- $enabledGitSyncForComponent := index $gitSync.components $component -}}
+{{- if or $localMode $.Values.dags.persistence.enabled (and $gitSync.enabled $enabledGitSyncForComponent) }}
- name: dags
- {{- if .Values.dags.mountPath }}
- mountPath: {{ .Values.dags.mountPath }}
+ {{- if $.Values.dags.mountPath }}
+ mountPath: {{ $.Values.dags.mountPath }}
{{- else }}
- mountPath: {{ printf "%s/dags" .Values.airflowHome }}
+ mountPath: {{ printf "%s/dags" $.Values.airflowHome }}
{{- end }}
- {{- if .Values.dags.persistence.subPath }}
- subPath: {{ .Values.dags.persistence.subPath }}
+ {{- if $.Values.dags.persistence.subPath }}
+ subPath: {{ $.Values.dags.persistence.subPath }}
{{- end }}
- readOnly: {{ .Values.dags.gitSync.enabled | ternary "True" "False" }}
+ readOnly: {{ $.Values.dags.gitSync.enabled | ternary "True" "False" }}
Review Comment:
`airflow_dags_mount` sets `readOnly` based on the global `dags.gitSync.enabled` flag, not whether git-sync is actually enabled for this component. If git-sync is enabled globally but disabled for a component (or if the mount is coming from persistence), this can incorrectly make the `dags` mount read-only and break expected behavior. Compute `readOnly` from the same per-component condition used to include git-sync (e.g., `$gitSync.enabled && $enabledGitSyncForComponent`, possibly combined with persistence rules).
```suggestion
readOnly: {{ ternary "True" "False" (and $gitSync.enabled $enabledGitSyncForComponent) }}
```
##########
helm-tests/tests/helm_tests/other/test_git_sync_worker.py:
##########
@@ -113,31 +93,33 @@ def test_resources_are_configurable(self):
)
assert jmespath.search("spec.template.spec.containers[1].resources.requests.cpu", docs[0]) == "300m"
- def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self):
- docs = render_chart(
- values={
- "dags": {
- "gitSync": {
- "enabled": True,
- "containerName": "git-sync-test",
- "sshKeySecret": "ssh-secret",
- "knownHosts": None,
- "branch": "test-branch",
- },
- "persistence": {"enabled": True},
- }
- },
- show_only=["templates/workers/worker-deployment.yaml"],
- )
-
- assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0])
+ # def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self):
+ # docs = render_chart(
+ # values={
+ # "dags": {
+ # "gitSync": {
+ # "enabled": True,
+ # "components": {"workers": True},
+ # "containerName": "git-sync-test",
+ # "sshKeySecret": "ssh-secret",
+ # "knownHosts": None,
+ # "branch": "test-branch",
+ # },
+ # "persistence": {"enabled": True},
+ # }
+ # },
+ # show_only=["templates/workers/worker-deployment.yaml"],
+ # )
+ #
+ # assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0])
Review Comment:
A previously active test is now commented out. Commented-out tests reduce coverage and can hide regressions (here, behavior around not mounting the SSH key secret when DAG persistence is enabled). Please either re-enable the test with updated expectations/values, or delete it and add a replacement that validates the intended new behavior.
```suggestion
def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self):
docs = render_chart(
values={
"dags": {
"gitSync": {
"enabled": True,
"components": {"workers": True},
"containerName": "git-sync-test",
"sshKeySecret": "ssh-secret",
"knownHosts": None,
"branch": "test-branch",
},
"persistence": {"enabled": True},
}
},
show_only=["templates/workers/worker-deployment.yaml"],
)
volumes = jmespath.search("spec.template.spec.volumes[].name", docs[0]) or []
assert "git-sync-ssh-key" not in volumes
```
##########
chart/templates/_helpers.yaml:
##########
@@ -182,11 +182,17 @@ If release name contains chart name it will be used as a full name.
{{/* Git ssh key volume */}}
{{- define "git_sync_ssh_key_volume" }}
+{{- $ := index . 0 -}}
+{{- $component := index . 1 -}}
+{{- $gitSync := $.Values.dags.gitSync -}}
+{{- $enabledGitSyncForComponent := index $gitSync.components $component -}}
+{{- if and $gitSync.enabled $enabledGitSyncForComponent (or $gitSync.sshKeySecret $gitSync.sshKey) }}
- name: git-sync-ssh-key
secret:
- secretName: {{ template "git_sync_ssh_key" . }}
+ secretName: {{ template "git_sync_ssh_key" $ }}
defaultMode: 288
Review Comment:
`git_sync_ssh_key_volume` is emitted whenever git-sync is enabled for the component and an SSH key is configured, even if this component will *not* deploy a git-sync container because `dags.persistence.enabled` is true (e.g., workers/triggerer/webserver/apiServer). This unnecessarily mounts the SSH key secret into pods (security exposure) and is likely why the persistence-related tests were commented out. Align the SSH key volume condition with the git-sync container inclusion logic for the given component (typically also requiring `not dags.persistence.enabled`, except for components where git-sync is intentionally supported with persistence).
##########
helm-tests/tests/helm_tests/other/test_git_sync_triggerer.py:
##########
@@ -23,30 +23,32 @@
class TestGitSyncTriggerer:
"""Test git sync triggerer."""
- def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self):
- docs = render_chart(
- values={
- "dags": {
- "gitSync": {
- "enabled": True,
- "containerName": "git-sync-test",
- "sshKeySecret": "ssh-secret",
- "knownHosts": None,
- "branch": "test-branch",
- },
- "persistence": {"enabled": True},
- }
- },
- show_only=["templates/triggerer/triggerer-deployment.yaml"],
- )
- assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0])
+ # def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self):
+ # docs = render_chart(
+ # values={
+ # "dags": {
+ # "gitSync": {
+ # "enabled": True,
+ # "components": {"triggerer": True},
+ # "containerName": "git-sync-test",
+ # "sshKeySecret": "ssh-secret",
+ # "knownHosts": None,
+ # "branch": "test-branch",
+ # },
+ # "persistence": {"enabled": True},
+ # }
+ # },
+ # show_only=["templates/triggerer/triggerer-deployment.yaml"],
+ # )
+ # assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0])
Review Comment:
A previously active test is now commented out. Commented-out tests reduce coverage and can hide regressions (here, behavior around not mounting the SSH key secret when DAG persistence is enabled). Please either re-enable the test with updated expectations/values, or delete it and add a replacement that validates the intended new behavior.
```suggestion
def test_validate_sshkeysecret_not_added_when_persistence_is_enabled(self):
docs = render_chart(
values={
"dags": {
"gitSync": {
"enabled": True,
"components": {"triggerer": True},
"containerName": "git-sync-test",
"sshKeySecret": "ssh-secret",
"knownHosts": None,
"branch": "test-branch",
},
"persistence": {"enabled": True},
}
},
show_only=["templates/triggerer/triggerer-deployment.yaml"],
)
assert "git-sync-ssh-key" not in jmespath.search("spec.template.spec.volumes[].name", docs[0])
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org