Improve consistency of values.yaml & misc#64559
Conversation
jscheffl
left a comment
There was a problem hiding this comment.
Wow, impressive rework! Thanks!
bugraoz93
left a comment
There was a problem hiding this comment.
Thanks, Przemysław! Agree with Jens, impressive :D
There was a problem hiding this comment.
Pull request overview
This PR refactors the Helm chart configuration documentation to make values.yaml structure and commenting more consistent, and includes a few small functional/schema tweaks to align defaults and improve generated airflow.cfg formatting.
Changes:
- Reorganized and rewrote many
chart/values.yamlcomments for consistency (descriptions above fields, examples below, standardized deprecation notes). - Tweaked
airflow.cfgrendering in the ConfigMap template to add spacing between sections. - Added a missing
default: nullforworkers.celery.priorityClassNamein the JSON schema.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| chart/values.yaml | Large-scale comment/documentation consistency updates plus a few clarified deprecation notes/examples. |
| chart/values.schema.json | Adds an explicit default for priorityClassName to match chart defaults/intent. |
| chart/templates/configmaps/configmap.yaml | Adjusts whitespace trimming to separate rendered airflow.cfg sections with a blank line. |
| # The secret name should follow naming convention of the application where resources are | ||
| # name {{ .Release-name }}-<POSTFIX>. In case of the keytab file, the postfix is "kerberos-keytab" | ||
| # So if your release is named "my-release" the name of the secret should be "my-release-kerberos-keytab" | ||
| # name '{{ .Release-name }}-<postfix>'. In case of the keytab file, the '<postfix>' is "kerberos-keytab". |
There was a problem hiding this comment.
The Helm template reference in this example is invalid: .Release-name is not a valid field. This should use .Release.Name (and keep naming consistent with other chart examples).
| # kubectl create secret generic {{ .Release.name }}-kerberos-keytab --from-file=kerberos.keytab | ||
| # | ||
| # If you have keytab file you can do it with similar: | ||
| # kubectl create secret generic {{ .Release.name }}-kerberos-keytab --from-file=kerberos.keytab |
There was a problem hiding this comment.
This example uses .Release.name, which is not a valid Helm template field (Helm uses .Release.Name). As written, users copy/pasting this will get template rendering errors.
| # This is automatically generated from `.Values.dagProcessor.dagBundleConfigList` using the `dag_bundle_config_list` helper function. | ||
| # Direct override is only possible via `dagProcessor.dagBundleConfigList` configuration |
There was a problem hiding this comment.
The comment says direct override is only possible via dagProcessor.dagBundleConfigList, but config.dag_processor.dag_bundle_config_list can still be overridden directly in user-supplied values (since config is rendered into airflow.cfg). Please adjust the wording to avoid misleading users (e.g., describe it as generated by default and recommend using dagProcessor.dagBundleConfigList).
| # This is automatically generated from `.Values.dagProcessor.dagBundleConfigList` using the `dag_bundle_config_list` helper function. | |
| # Direct override is only possible via `dagProcessor.dagBundleConfigList` configuration | |
| # This value is generated by default from `.Values.dagProcessor.dagBundleConfigList` using the `dag_bundle_config_list` helper function. | |
| # It is recommended to configure this via `dagProcessor.dagBundleConfigList` rather than overriding `config.dag_processor.dag_bundle_config_list` directly. |
The goal of this PR is to make Helm's
values.yamlfile more consistent in structure + some comments were rewritten to be clearer for users. After this PR, comments invalues.yamlfile are:Furthermore, the smaller changes were added to this PR, like:
workers.celery.priorityClassNamein thevalues.schema.jsonfileworkers.hostAliasessecurityContextdeprecations + forwebserverSecretAnnotationsWas generative AI tooling used to co-author this PR?
{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.