Added labels following helm best practises

[fjagugar]

Description of the change

Adds labels to chart templates using the Helm best practises: https://helm.sh/docs/chart_best_practices/labels/#standard-labels

Benefits

Tools already using the label syntax encouraged by Helm will now be able to use that syntax with kubapps.

Possible drawbacks

Only added new tags. Old tags have not been removed for backward compatibility.

[andresmgot]

Thanks for adding the new labels @fjagugar! I have some suggestions to make this easier.

Apart from that, I believe adding labels is backward compatible but can you confirm if adding the labels doesn't break the upgrade? (If you can run helm upgrade ./chart/kubeapps ... without errors, that should be enough). We are in the process of adding a test for this in the CI but is not ready yet.

[andresmgot]

Can you also add the other two annotations that are recommended: app.kubernetes.io/name and app.kubernetes.io/managed-by?

[andresmgot]

Also, I see that these labels are defined in the _helpers.tpl file too:

{{/*
Common labels
*/}}
{{- define "kubeapps.labels" -}}
app: {{ include "kubeapps.name" . }}
chart: {{ include "kubeapps.chart" . }}
release: {{ .Release.Name }}
heritage: {{ .Release.Service }}
{{- end -}}

We can add the new labels there and simply use the helper everywhere (to avoid having to add the labels one by one).

[fjagugar]

I see that not all the labels added in all the cases use the same parameters. It seem the app/app.kubernetes.io/name usually changes.

I've created a new template that adds all the labels that are common except those two ones and used it wherever those labels were the same.

[andresmgot]

🤦 this is a bit of a mess (our fault). I guess app should have been always the same but we changed it in several places and now we cannot fix it without making it a breaking change.

From the definition in the docs: app.kubernetes.io/name: This should be the app name, reflecting the entire app. Usually {{ template "name" . }} is used for this. This is used by many Kubernetes manifests, and is not Helm-specific.. Can we at least have a common app.kubernetes.io/name? We can move that new label to the common group.

[andresmgot]

here you are just adding one of the labels, is that intentional?

[fjagugar]

Yes, as this did not had the chart tag before. I'm only adding the app.kubernetes.io/instance which is the equivalent to the release one.

[fjagugar]

Apart from that, I believe adding labels is backward compatible but can you confirm if adding the labels doesn't break the upgrade? (If you can run helm upgrade ./chart/kubeapps ... without errors, that should be enough). We are in the process of adding a test for this in the CI but is not ready yet.

I tested and the upgrade seems to work without issues.

[fjagugar]

Yes, as this did not had the chart tag before. I'm only adding the app.kubernetes.io/instance which is the equivalent to the release one.

[fjagugar]

I see that not all the labels added in all the cases use the same parameters. It seem the app/app.kubernetes.io/name usually changes.

I've created a new template that adds all the labels that are common except those two ones and used it wherever those labels were the same.

[fjagugar]

I've created a diff on the template generated using helm template before and after the changes:

template_differential.txt

[andresmgot]

Thanks for the careful update @fjagugar ! I think we can clean it up a little bit more.

[andresmgot]

🤦 this is a bit of a mess (our fault). I guess app should have been always the same but we changed it in several places and now we cannot fix it without making it a breaking change.

From the definition in the docs: app.kubernetes.io/name: This should be the app name, reflecting the entire app. Usually {{ template "name" . }} is used for this. This is used by many Kubernetes manifests, and is not Helm-specific.. Can we at least have a common app.kubernetes.io/name? We can move that new label to the common group.

[andresmgot]

I see this matchLabels is not used anywhere. Can we delete it?

[fjagugar]

New template diff generated:
template_differential.txt

[andresmgot]

Thanks for the changes