Copilot commented on code in PR #64763:
URL: https://github.com/apache/airflow/pull/64763#discussion_r3066499352
##########
helm-tests/tests/helm_tests/airflow_aux/test_annotations.py:
##########
@@ -264,6 +264,116 @@ def test_annotations_are_added(self, values, show_only,
expected_annotations):
assert k in obj["metadata"]["annotations"]
assert v == obj["metadata"]["annotations"][k]
+ @pytest.mark.parametrize(
+ ("values_key", "show_only"),
+ [
+ ("scheduler", "templates/scheduler/scheduler-serviceaccount.yaml"),
+ ("triggerer", "templates/triggerer/triggerer-serviceaccount.yaml"),
+ ],
+ )
+ def test_tpl_rendered_annotations_airflow_2(self, values_key, show_only):
+ """Test SA annotations support tpl rendering for Airflow 2.x
components."""
+ k8s_objects = render_chart(
+ values={
+ "airflowVersion": "2.11.0",
+ values_key: {
+ "serviceAccount": {
+ "annotations": {
+ "iam.gke.io/gcp-service-account": "{{
.Release.Name }}[email protected]",
+ },
+ },
+ },
+ },
Review Comment:
The new `airflow.tplDict` path has a failure mode specifically when multiple
annotations are provided (YAML line concatenation). The current tests only
assert a single annotation key/value; please add at least one test case that
sets 2+ annotations and asserts both render correctly to catch invalid YAML
output regressions.
##########
chart/templates/_helpers.yaml:
##########
@@ -50,6 +50,12 @@ If release name contains chart name it will be used as a
full name.
{{- end }}
{{- end }}
+{{- define "airflow.tplDict" -}}
+ {{- range $key, $value := .values }}
+ {{- printf "%s: %s" $key (tpl $value $.context | quote) }}
+ {{- end }}
Review Comment:
The helper currently emits entries without guaranteed newline separation due
to `{{- ... }}` whitespace trimming in the loop, which can concatenate multiple
key/value pairs into a single invalid YAML line (it won’t show up in tests that
only use one annotation). Also, `tpl $value ...` requires `$value` to be a
string; passing a non-string (even accidentally) will hard-fail template
rendering. A robust fix is to build a new dict where each value is
`tpl`-rendered from a string representation, then output it via `toYaml` so
YAML formatting/newlines are correct for any number of entries.
```suggestion
{{- $rendered := dict -}}
{{- range $key, $value := .values }}
{{- $_ := set $rendered $key (tpl (toString $value) $.context) -}}
{{- end }}
{{- toYaml $rendered -}}
```
##########
chart/templates/_helpers.yaml:
##########
@@ -599,7 +605,11 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{- end }}
{{- define "airflow_webserver_config_configmap_name" -}}
- {{- default (printf "%s-webserver-config" (include "airflow.fullname" .))
.Values.webserver.webserverConfigConfigMapName }}
+ {{- if .Values.webserver.webserverConfigConfigMapName }}
+ {{- tpl .Values.webserver.webserverConfigConfigMapName . }}
+ {{- else }}
+ {{- printf "%s-webserver-config" (include "airflow.fullname" .) }}
+ {{- end }}
{{- end }}
Review Comment:
This introduces new behavior (allowing `tpl` expressions in
`webserverConfigConfigMapName` / `apiServerConfigConfigMapName`). There are
Helm unit tests added for other `tpl` features in this PR, but none shown for
these name helpers; please add a test that sets e.g. `{{ .Release.Name
}}-custom-config` and asserts the rendered ConfigMap name matches, to prevent
regressions.
--
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]
