Add new standard labels and allow custom labels #45
Conversation
| {{- /* Allow an app suffix name to be passed in to append to the fullname */}} | ||
| {{- $defaultAppName := include "fullname" . }} | ||
| {{- $appName := printf "%s%s" $defaultAppName (default "" .appSuffix) }} | ||
| {{- /* Existing Legacy labels for passivity */}} |
There was a problem hiding this comment.
I added these comments with the intention to document why they were here, however this top one does create an extra empty line the in the helm template
apiVersion: apps/v1
kind: Deployment
metadata:
name: release-name-athens-proxy-jaeger
labels:
app: release-name-athens-proxy-jaeger
chart: athens-proxy-0.7.0
release: "release-name"
heritage: "Helm"
app.kubernetes.io/name: release-name-athens-proxy-jaeger
helm.sh/chart: athens-proxy-0.7.0
app.kubernetes.io/managed-by: "Helm"
app.kubernetes.io/instance: "release-name"
app.kubernetes.io/version: "v0.12.0"
athensIs: awesome
spec:
This output seems generally accepted by the helm docs https://helm.sh/docs/chart_best_practices/templates/#whitespace-in-generated-templates but i'm happy to remove these comments if the tradeoff isn't worth it
There was a problem hiding this comment.
As long as the indentation is correct, it's fine. I've seen many helm charts with some empty lines. The comment should generally not be necessary when the code is readable, but it may help newcomers and does not hurt.
| chart: "{{ .Chart.Name }}-{{ .Chart.Version }}" | ||
| release: "{{ .Release.Name }}" | ||
| heritage: "{{ .Release.Service }}" | ||
| {{- include "athens.metaLabels" (dict "appSuffix" "-jaeger" "Values" .Values "Release" .Release "Chart" .Chart) | nindent 4 }} |
There was a problem hiding this comment.
This feels a bit gross since it has to be aware of the values being used by the underlying helper, but I wanted to find a way to re-use the helper for these couple of unique cases that have a different name specified currently.
FWIW, these differences seem a bit strange, I'd normally expect all of the resources created by the helm chart to have this same app name. However, I tried to keep things as passive as possible.
I did consider creating a separate helper function that was metaLabelsWithoutAppName that would be re-used by the new helper and could be used here and have the app and app.kubernetes.io/name be set here explicity. I favored this approach but am happy to change things up
There was a problem hiding this comment.
I agree, not the nicest include I've seen, but it helps keeping the labels consistent and may not be worth to add an extra helper.
|
Thank you for your contribution and effort put in @alex-bezek, unfortunately I overlooked that this PR did not bump the chart version to trigger a release, so I just merged #46 to take care of that. |
|
thanks @DrPsychick and @michalpristas ! |
Resolves #44
Why
This chart appears to have the standard labels used with helm 2. This adds in the new helm 3 standard labels and also allows the user to pass in additional labels that will get applied to all resources.
How
appSuffixto be passed in for passivity for the jaeger and storage usecasesValidation
Run
helm template charts/athens-proxy --set extraLabels.athensIs=awesome --set jaeger.enabled=trueagainst main and this branch and diff themDiff
Example Output from template