fgalind1 commented on code in PR #31066:
URL: https://github.com/apache/airflow/pull/31066#discussion_r1237821395
##########
chart/INSTALL:
##########
Review Comment:
> newsfragment instead
do you have any suggestion where this should be or what do you mean by that?
:) happy to move/amend wherever needed 😃
##########
chart/templates/secrets/fernetkey-secret.yaml:
##########
@@ -35,7 +35,7 @@ metadata:
{{- toYaml . | nindent 4 }}
{{- end }}
annotations:
- "helm.sh/hook": "pre-install"
+ "helm.sh/hook": "pre-install,pre-upgrade"
Review Comment:
if the name changes it will never be installed anymore :( - do you have any
suggestion? maybe an option in values.yaml like reinstallFernetKey? which adds
the pre-upgrade hook optional? otherwise we'll never be able to update the
Secret and all the references of volume mounts will be broken. that was the
reason why I added it here but I'm open for suggestions :) Or we can keep these
secrets as they were with the old names?
##########
tests/charts/airflow_aux/test_basic_helm_chart.py:
##########
@@ -116,6 +116,49 @@ def test_basic_deployments(self, version):
"test-label"
), f"Missing label test-label on {k8s_name}. Current labels:
{labels}"
+ def test_basic_deployments_with_standard_naming(self):
+ k8s_objects = render_chart(
+ "test-basic",
+ {"useStandardNaming": True},
+ )
+ list_of_kind_names_tuples = {
+ (k8s_object["kind"], k8s_object["metadata"]["name"]) for
k8s_object in k8s_objects
+ }
+ expected = {
+ ("ServiceAccount", "test-basic-airflow-create-user-job"),
+ ("ServiceAccount", "test-basic-airflow-migrate-database-job"),
+ ("ServiceAccount", "test-basic-airflow-redis"),
+ ("ServiceAccount", "test-basic-airflow-scheduler"),
+ ("ServiceAccount", "test-basic-airflow-statsd"),
+ ("ServiceAccount", "test-basic-airflow-triggerer"),
+ ("ServiceAccount", "test-basic-airflow-webserver"),
+ ("ServiceAccount", "test-basic-airflow-worker"),
+ ("Secret", "test-basic-airflow-metadata"),
+ ("Secret", "test-basic-airflow-broker-url"),
+ ("Secret", "test-basic-airflow-fernet-key"),
+ ("Secret", "test-basic-airflow-webserver-secret-key"),
+ ("Secret", "test-basic-airflow-redis-password"),
+ ("ConfigMap", "test-basic-airflow-config"),
+ ("ConfigMap", "test-basic-airflow-statsd"),
+ ("Role", "test-basic-airflow-pod-launcher-role"),
+ ("Role", "test-basic-airflow-pod-log-reader-role"),
+ ("RoleBinding", "test-basic-airflow-pod-launcher-rolebinding"),
+ ("RoleBinding", "test-basic-airflow-pod-log-reader-rolebinding"),
+ ("Service", "test-basic-airflow-redis"),
+ ("Service", "test-basic-airflow-statsd"),
+ ("Service", "test-basic-airflow-webserver"),
+ ("Service", "test-basic-airflow-worker"),
+ ("Deployment", "test-basic-airflow-scheduler"),
+ ("Deployment", "test-basic-airflow-statsd"),
+ ("Deployment", "test-basic-airflow-webserver"),
+ ("StatefulSet", "test-basic-airflow-redis"),
+ ("StatefulSet", "test-basic-airflow-worker"),
+ ("Job", "test-basic-airflow-create-user"),
+ ("Job", "test-basic-airflow-run-airflow-migrations"),
+ }
+ for kind_name in expected:
+ assert kind_name in list_of_kind_names_tuples
Review Comment:
good call - addressed in
https://github.com/apache/airflow/pull/31066/commits/5fb9f8e05b9c55516ba6a161775892dc396a6335
##########
chart/templates/_helpers.yaml:
##########
@@ -23,7 +23,40 @@ We truncate at 63 chars because some Kubernetes name fields
are limited to this
If release name contains chart name it will be used as a full name.
*/}}
{{- define "airflow.fullname" -}}
- {{- if .Values.fullnameOverride }}
+ {{- if not .Values.useStandardNaming }}
+ {{- .Release.Name }}
+ {{- else if .Values.fullnameOverride }}
+ {{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" }}
+ {{- else }}
+ {{- $name := default .Chart.Name .Values.nameOverride }}
+ {{- if contains $name .Release.Name }}
+ {{- .Release.Name | trunc 63 | trimSuffix "-" }}
+ {{- else }}
+ {{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}
+ {{- end }}
+ {{- end }}
+{{- end }}
+
+{{/*
+Create a default fully qualified app name for objects that already contained
airflow in the name
+We truncate at 63 chars because some Kubernetes name fields are limited to
this (by the DNS naming spec).
+If release name contains chart name it will be used as a full name.
+*/}}
+{{- define "airflow.name" -}}
+ {{ if .Values.fullnameOverride }}
+ {{- printf "%s" .Values.fullnameOverride | trunc 63 | trimSuffix "-" }}
+ {{- else }}
+ {{- $name := default .Chart.Name .Values.nameOverride }}
+ {{- if contains $name .Release.Name }}
+ {{- .Release.Name | trunc 63 | trimSuffix "-" }}
+ {{- else }}
+ {{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}
Review Comment:
if we add -airflow-" we endup with double airflow e.g.
`release-name-airflow-airflow-metadata` as Chart.Name is already airflow -
that's why I didn't kept airflow there
##########
chart/templates/webserver/webserver-ingress.yaml:
##########
@@ -65,6 +65,7 @@ spec:
{{- .Values.ingress.web.hosts | default (list
.Values.ingress.web.host) | toYaml | nindent 8 }}
secretName: {{ .Values.ingress.web.tls.secretName }}
{{- end }}
+ {{- $fullname := (include "airflow.fullname" .) }}
Review Comment:
good one - addressed in next commit
https://github.com/apache/airflow/pull/31066/commits/5fb9f8e05b9c55516ba6a161775892dc396a6335
--
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]
