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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
