grafana / grafana-operator

An operator for Grafana that installs and manages Grafana instances, Dashboards and Datasources through Kubernetes/OpenShift CRs
https://grafana.github.io/grafana-operator/
Apache License 2.0
867 stars 385 forks source link

Add ability to have additional labels on the deployment #1560

Closed David-John-Nintex closed 2 weeks ago

David-John-Nintex commented 3 months ago

This is an option to fix #1546

With the latest helm chart changes, the ability to add additional labels to the deployment was removed

CLAassistant commented 3 months ago

CLA assistant check
All committers have signed the CLA.

NissesSenap commented 3 months ago

@David-John-Nintex why isn't additionalLabels an option? It will look a bit ugly since you will have to redefine a few labels, but I think it should do the trick.

Would prefer not to add a single config for a single user.

David-John-Nintex commented 3 months ago

@David-John-Nintex why isn't additionalLabels an option? It will look a bit ugly since you will have to redefine a few labels, but I think it should do the trick.

While it would work, then we have to keep it in line with the chart updates, since some of the values there are generated by helm, however if this is the recommended way of doing it, then it should work

Would prefer not to add a single config for a single user.

I can see this, was wondering, however, if it would be something others would also like to use, but if not, then yes, I do agree, one thing for one user is not a great thing

David-John-Nintex commented 3 months ago

@NissesSenap additionalLabels is not an option for us, as we would have to redefine the labels that are already defined in the grafana-operator.labels helper function (namely app.kubernetes.io/version and app.kubernetes.io/managed-by so these would be redefined everywhere where we use the labels

NissesSenap commented 3 months ago

I think it's a very limited pool of companies that have this need, but we can wait with the issue and see if it gets any thumbs up.

@David-John-Nintex I don't understand your answer. Why wouldn't it be possible to do exactly that? I think the additional labels would overwrite the existing default labels. Can you share how it would look?

Another options since your company have this great idea, would be to have a mutating webhook that solves this for you automatically for example through OPA, this way you don't have to label your applications in all third party apps, that I assume you do today.

David-John-Nintex commented 3 months ago

@NissesSenap I agree my company has a very strict policy to get things into our clusters.

However, looking at the latest changes that have been made to your helm charts, you have now removed the ability to add additional labels to the deployment (with the merging of the labels and the additionalLabels) you now cannot add any additional labels to the Deployment

I have updated my PR to include the ability to add additionalLabels to the deployment as you were previously able to add, except this time specifically for the Deployment

theSuess commented 3 months ago

It's still possible to add labels to the deployment, those labels will however also be applied to all other resources.

If we decide to go with deployment.additonalLabels, I'd like to see the same option for all other resources as well.

David-John-Nintex commented 3 months ago

It's still possible to add labels to the deployment, those labels will however also be applied to all other resources.

If we decide to go with deployment.additonalLabels, I'd like to see the same option for all other resources as well.

If you look at https://github.com/grafana/grafana-operator/blob/e328412e8e2eff3d0155edcbd86950101ca37742/deploy/helm/grafana-operator/templates/deployment.yaml#L20-L22 only the selectorLabels are set on the Pod created by the Deployment

As far as I can see this is the only resource where the merging of the additionalLabels into the grafana-operator.labels here has broken the ability to set custom labels

theSuess commented 3 months ago

I see, you want this to apply to the selector instead of the deployment. In that case, would it suffice to add

{{- with .Values.additionalLabels }}
{{ toYaml . }}
{{- end }}

to the selectorLabels template?

David-John-Nintex commented 3 months ago

I see, you want this to apply to the selector instead of the deployment. In that case, would it suffice to add

{{- with .Values.additionalLabels }}
{{ toYaml . }}
{{- end }}

to the selectorLabels template?

No, then you'll effectively be making selectorLabels the same as labels which would work for me, but i think you would probably have to add

{{- with .Values.additionalLabels }}
{{ toYaml . }}
{{- end }}

to the label: part in the deployment template

github-actions[bot] commented 2 months ago

This PR hasn't been updated for a while, marking as stale

jrRibeiro commented 2 months ago

I agree with @David-John-Nintex, I currently also have this limitation as I need to add labels to pods, not just the deployment, and was going to open a PR when I found this one. 👍

I don't agree with the key of the value, though. Not a standard, but usually charts use a .Values.podLabels key to add extra pod labels (e.g. Grafana itself).

No need to create a deployment parent key just to set labels.

Can you please double check @NissesSenap @theSuess?

github-actions[bot] commented 1 month ago

This PR hasn't been updated for a while, marking as stale

pharaujo commented 3 weeks ago

Hey, I missed this issue so I opened a duplicate one (#1642). In my issue I suggest changing the macro from including grafana-operator.selectorLabels to grafana-operator.labels in spec.template.metadata.labels, as it would cleanly fix everyone's issue.

I see, you want this to apply to the selector instead of the deployment. In that case, would it suffice to add

{{- with .Values.additionalLabels }}
{{ toYaml . }}
{{- end }}

to the selectorLabels template?

You can't do this, because it would break the deployment on update. Selector labels are immutable after being set, which is why they are templated separately from the additional labels.

David-John-Nintex commented 2 weeks ago

This is a stale PR, and a new one has been opened #1649