[Helm]Support tpl rendering in ServiceAccount annotations, metadataConnection, and config ConfigMap names#64763
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
5e77629 to
3816392
Compare
…on, and config ConfigMap names
The scheduler ServiceAccount already tpl-renders annotations via
range/tpl, but all other ServiceAccounts (pgbouncer, webserver, workers,
triggerer, dag-processor, api-server) use raw toYaml. This makes it
impossible to use template expressions like
{{ .Values.global.appName }}-sa@project.iam.gserviceaccount.com in
annotation values for those components.
Apply the same range/tpl pattern from the scheduler to all ServiceAccount
templates for consistency.
Also tpl-render data.metadataConnection.user and
data.metadataConnection.db so wrapper charts can derive database
credentials from template expressions instead of requiring static values.
This affects the metadata connection secret and pgbouncer config.
Also tpl-render webserverConfigConfigMapName and
apiServerConfigConfigMapName so wrapper charts can use
{{ .Release.Name }}-custom-config instead of hardcoding the release name.
All changes are backward compatible: tpl on a plain string without
template expressions returns the string unchanged.
3816392 to
3e7fda5
Compare
jscheffl
left a comment
There was a problem hiding this comment.
Good for me, leavinf review to another pair of eyes prior merge.
|
Also, looking at the tests, I would recommend configuring prek for running hooks before commit and push - you can find a guide here -> https://github.com/apache/airflow/blob/main/contributing-docs/03_contributors_quick_start.rst#id9 |
… files - Remove nindent from tplDict helper, move indent to call sites - Move SA annotation tpl tests to test_annotations.py - Move metadataConnection tpl tests to test_metadata_connection_secret.py - Split Airflow 2/3 version tests into separate parametrized cases - Remove conditional logic from tests per review feedback - Fix result-backend nit (remove unnecessary parens) - Delete test_tpl_rendering.py (tests relocated) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@Miretpl Thanks for the thorough review! All feedback addressed in the latest commit:
Will also set up pre-commit hooks as suggested. Thanks! |
Miretpl
left a comment
There was a problem hiding this comment.
Looks much better! Just small nits
…x versions - Separate pgbouncer into dedicated test (no conditional logic) - Use workers.celery.serviceAccount for Celery worker tpl test - Add workers.kubernetes.serviceAccount test for KubernetesExecutor - Use airflowVersion 2.11.0 (first supported 2.11) instead of 2.11.2 - Remove explicit airflowVersion from Airflow 3 tests (default is 3.x) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… template The worker SA template reads from workers.serviceAccount (merged from workers.celery), not workers.kubernetes.serviceAccount. Removed invalid test. All 82 tests in test_annotations.py and test_metadata_connection_secret.py pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
eladkal
left a comment
There was a problem hiding this comment.
Overall LGTM
probably need also @jedcunningham eye on this one
|
@Miretpl All feedback addressed and tests verified locally (82/82 pass):
|
@shlomitubul it works in a little different way. The |
|
@shlomitubul I see that the commits were co-authored by Claude. I would really encourage you to make sure that everything that Claude did is fully understandable to be compliant with our AI-usage policy - https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions |
@Miretpl i appreciate your excellent review !, yes im aware of the project AI policy and fully understand the changeset |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR enables broader Helm tpl rendering support across the chart so wrapper charts can derive values (ServiceAccount annotations, metadata DB credentials, and config ConfigMap names) from template expressions.
Changes:
- Standardize ServiceAccount annotation rendering to use a shared
tpl-rendering helper across components. tpl-renderdata.metadataConnection.userand.dbfor the metadata connection secret and PgBouncer config.tpl-renderwebserverConfigConfigMapNameandapiServerConfigConfigMapNameoverrides.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| helm-tests/tests/helm_tests/security/test_metadata_connection_secret.py | Adds tests for tpl rendering of metadata connection user and db. |
| helm-tests/tests/helm_tests/airflow_aux/test_annotations.py | Adds tests ensuring ServiceAccount annotations support tpl rendering across components. |
| chart/templates/workers/worker-serviceaccount.yaml | Switches worker SA annotations rendering to shared tpl dict helper. |
| chart/templates/webserver/webserver-serviceaccount.yaml | Switches webserver SA annotations rendering to shared tpl dict helper. |
| chart/templates/triggerer/triggerer-serviceaccount.yaml | Switches triggerer SA annotations rendering to shared tpl dict helper. |
| chart/templates/secrets/metadata-connection-secret.yaml | Applies tpl rendering to metadata connection user and db in generated connection URIs. |
| chart/templates/scheduler/scheduler-serviceaccount.yaml | Refactors scheduler SA annotations to use shared tpl dict helper. |
| chart/templates/pgbouncer/pgbouncer-serviceaccount.yaml | Switches PgBouncer SA annotations rendering to shared tpl dict helper. |
| chart/templates/dag-processor/dag-processor-serviceaccount.yaml | Switches dag-processor SA annotations rendering to shared tpl dict helper. |
| chart/templates/api-server/api-server-serviceaccount.yaml | Switches api-server SA annotations rendering to shared tpl dict helper. |
| chart/templates/_helpers.yaml | Introduces airflow.tplDict helper; tpl-renders PgBouncer db/user fields and config ConfigMap name overrides. |
The previous printf-based approach concatenated multiple annotations into a single YAML line due to whitespace trimming. Replace with dict-building + toYaml for correct YAML output with any number of entries. Also adds toString for non-string value safety. Added test_tpl_rendered_multiple_annotations to verify 3 annotations (2 templated + 1 plain) render correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The worker-serviceaccount.yaml template serves both CeleryExecutor and KubernetesExecutor. Add test verifying tpl rendering works for the KubernetesExecutor path via workers.serviceAccount.annotations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@Miretpl can you re-review if OK now? |
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
…tations, metadataConnection, and config ConfigMap names (#64763) * Support tpl rendering in ServiceAccount annotations, metadataConnection, and config ConfigMap names The scheduler ServiceAccount already tpl-renders annotations via range/tpl, but all other ServiceAccounts (pgbouncer, webserver, workers, triggerer, dag-processor, api-server) use raw toYaml. This makes it impossible to use template expressions like {{ .Values.global.appName }}-sa@project.iam.gserviceaccount.com in annotation values for those components. Apply the same range/tpl pattern from the scheduler to all ServiceAccount templates for consistency. Also tpl-render data.metadataConnection.user and data.metadataConnection.db so wrapper charts can derive database credentials from template expressions instead of requiring static values. This affects the metadata connection secret and pgbouncer config. Also tpl-render webserverConfigConfigMapName and apiServerConfigConfigMapName so wrapper charts can use {{ .Release.Name }}-custom-config instead of hardcoding the release name. All changes are backward compatible: tpl on a plain string without template expressions returns the string unchanged. * Address review feedback: make tplDict generic, move tests to existing files - Remove nindent from tplDict helper, move indent to call sites - Move SA annotation tpl tests to test_annotations.py - Move metadataConnection tpl tests to test_metadata_connection_secret.py - Split Airflow 2/3 version tests into separate parametrized cases - Remove conditional logic from tests per review feedback - Fix result-backend nit (remove unnecessary parens) - Delete test_tpl_rendering.py (tests relocated) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Address round 2 review: split pgbouncer, use celery/k8s worker SA, fix versions - Separate pgbouncer into dedicated test (no conditional logic) - Use workers.celery.serviceAccount for Celery worker tpl test - Add workers.kubernetes.serviceAccount test for KubernetesExecutor - Use airflowVersion 2.11.0 (first supported 2.11) instead of 2.11.2 - Remove explicit airflowVersion from Airflow 3 tests (default is 3.x) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Remove kubernetes worker SA test — workers.kubernetes does not own SA template The worker SA template reads from workers.serviceAccount (merged from workers.celery), not workers.kubernetes.serviceAccount. Removed invalid test. All 82 tests in test_annotations.py and test_metadata_connection_secret.py pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix tplDict multi-annotation bug: use toYaml instead of printf The previous printf-based approach concatenated multiple annotations into a single YAML line due to whitespace trimming. Replace with dict-building + toYaml for correct YAML output with any number of entries. Also adds toString for non-string value safety. Added test_tpl_rendered_multiple_annotations to verify 3 annotations (2 templated + 1 plain) render correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add KubernetesExecutor worker SA tpl annotation test The worker-serviceaccount.yaml template serves both CeleryExecutor and KubernetesExecutor. Add test verifying tpl rendering works for the KubernetesExecutor path via workers.serviceAccount.annotations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- (cherry picked from commit 3a458de) Co-authored-by: shlomi tubul <33376277+shlomitubul@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Backport successfully created: chart/v1-2x-testNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
|
…tations, metadataConnection, and config ConfigMap names (#64763) (#65081) * Support tpl rendering in ServiceAccount annotations, metadataConnection, and config ConfigMap names The scheduler ServiceAccount already tpl-renders annotations via range/tpl, but all other ServiceAccounts (pgbouncer, webserver, workers, triggerer, dag-processor, api-server) use raw toYaml. This makes it impossible to use template expressions like {{ .Values.global.appName }}-sa@project.iam.gserviceaccount.com in annotation values for those components. Apply the same range/tpl pattern from the scheduler to all ServiceAccount templates for consistency. Also tpl-render data.metadataConnection.user and data.metadataConnection.db so wrapper charts can derive database credentials from template expressions instead of requiring static values. This affects the metadata connection secret and pgbouncer config. Also tpl-render webserverConfigConfigMapName and apiServerConfigConfigMapName so wrapper charts can use {{ .Release.Name }}-custom-config instead of hardcoding the release name. All changes are backward compatible: tpl on a plain string without template expressions returns the string unchanged. * Address review feedback: make tplDict generic, move tests to existing files - Remove nindent from tplDict helper, move indent to call sites - Move SA annotation tpl tests to test_annotations.py - Move metadataConnection tpl tests to test_metadata_connection_secret.py - Split Airflow 2/3 version tests into separate parametrized cases - Remove conditional logic from tests per review feedback - Fix result-backend nit (remove unnecessary parens) - Delete test_tpl_rendering.py (tests relocated) * Address round 2 review: split pgbouncer, use celery/k8s worker SA, fix versions - Separate pgbouncer into dedicated test (no conditional logic) - Use workers.celery.serviceAccount for Celery worker tpl test - Add workers.kubernetes.serviceAccount test for KubernetesExecutor - Use airflowVersion 2.11.0 (first supported 2.11) instead of 2.11.2 - Remove explicit airflowVersion from Airflow 3 tests (default is 3.x) * Remove kubernetes worker SA test — workers.kubernetes does not own SA template The worker SA template reads from workers.serviceAccount (merged from workers.celery), not workers.kubernetes.serviceAccount. Removed invalid test. All 82 tests in test_annotations.py and test_metadata_connection_secret.py pass. * Fix tplDict multi-annotation bug: use toYaml instead of printf The previous printf-based approach concatenated multiple annotations into a single YAML line due to whitespace trimming. Replace with dict-building + toYaml for correct YAML output with any number of entries. Also adds toString for non-string value safety. Added test_tpl_rendered_multiple_annotations to verify 3 annotations (2 templated + 1 plain) render correctly. * Add KubernetesExecutor worker SA tpl annotation test The worker-serviceaccount.yaml template serves both CeleryExecutor and KubernetesExecutor. Add test verifying tpl rendering works for the KubernetesExecutor path via workers.serviceAccount.annotations. --------- (cherry picked from commit 3a458de) Co-authored-by: shlomi tubul <33376277+shlomitubul@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The scheduler ServiceAccount already tpl-renders annotations via range/tpl, but all other ServiceAccounts (pgbouncer, webserver, workers, triggerer, dag-processor, api-server) use raw toYaml. This makes it impossible to use template expressions like
{{ .Values.global.appName }}-sa@project.iam.gserviceaccount.com in annotation values for those components.
Apply the same range/tpl pattern from the scheduler to all ServiceAccount templates for consistency.
Also tpl-render data.metadataConnection.user and
data.metadataConnection.db so wrapper charts can derive database credentials from template expressions instead of requiring static values. This affects the metadata connection secret and pgbouncer config.
Also tpl-render webserverConfigConfigMapName and
apiServerConfigConfigMapName so wrapper charts can use {{ .Release.Name }}-custom-config instead of hardcoding the release name.
All changes are backward compatible: tpl on a plain string without template expressions returns the string unchanged.
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Anthropic Claude Opus 4.6) following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.