DerekHeldtWerle commented on a change in pull request #14152:
URL: https://github.com/apache/airflow/pull/14152#discussion_r573883993
##########
File path: chart/templates/_helpers.yaml
##########
@@ -15,6 +15,32 @@
# specific language governing permissions and limitations
# under the License.
+{{/*
+Expand the name of the chart.
+*/}}
+{{- define "airflow.name" -}}
+{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
+{{- end }}
+
+{{/*
+Create a default fully qualified app 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.fullname" -}}
+{{- if .Values.fullnameOverride }}
Review comment:
I'll get them added to the README 👍
When you issue a [helm create](https://helm.sh/docs/helm/helm_create/)
command, the additions above (and
[others](https://helm.sh/docs/helm/helm_create/)) are added to the
`_helpers.yaml` file to help follow the best practices set forward by the helm
team. An explanation on the need for the two separate options can be found
[here](https://stackoverflow.com/a/63839389). Since we aren't leveraging the
other items created by the `helm create` command (namely the common labels and
selector labels) I can remove the `airflow.name` definition as its currently
unused.
I think future looking a different PR that adds the common labels, selector
labels, and the `airflow.name` to set the aforementioned labels generated by
the `helm create` command would also be worthwhile to push out to all of the
manifests
##########
File path: chart/templates/_helpers.yaml
##########
@@ -15,6 +15,32 @@
# specific language governing permissions and limitations
# under the License.
+{{/*
+Expand the name of the chart.
+*/}}
+{{- define "airflow.name" -}}
+{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
+{{- end }}
+
+{{/*
+Create a default fully qualified app 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.fullname" -}}
+{{- if .Values.fullnameOverride }}
Review comment:
I'll get it added to the README 👍
When you issue a [helm create](https://helm.sh/docs/helm/helm_create/)
command, the additions above (and
[others](https://helm.sh/docs/helm/helm_create/)) are added to the
`_helpers.yaml` file to help follow the best practices set forward by the helm
team. An explanation on the need for the two separate options can be found
[here](https://stackoverflow.com/a/63839389). Since we aren't leveraging the
other items created by the `helm create` command (namely the common labels and
selector labels) I can remove the `airflow.name` definition as its currently
unused.
I think future looking a different PR that adds the common labels, selector
labels, and the `airflow.name` to set the aforementioned labels generated by
the `helm create` command would also be worthwhile to push out to all of the
manifests
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
