[GitHub] [airflow] fgalind1 commented on a diff in pull request #31066: Support naming customization on helm chart resources

["fgalind1 (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org