Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove helm chart version from labels #1114

Merged
merged 1 commit into from Nov 15, 2022

Conversation

FlorianLaunay
Copy link
Contributor

@FlorianLaunay FlorianLaunay commented Nov 8, 2022

SUMMARY

Remove the Helm chart version from helm.sh/chart labels in all Helm templates.

Fixes #1057

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
ADDITIONAL INFORMATION

Including chart version in the matchLabel selector of the AWX deployment template causes problem during the helm upgrade of the AWX operator. Indeed this field is now immutable for deployments in recent versions of Kubernetes (https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#label-selector-updates). So running a helm upgrade with a chart containing a deployment template with a different content in the spec.selector.matchLabels field is not allowed and causes an error.

An acceptable solution when using kustomize is to remove chart version in labels. This will allow to continue to adhere to Helm best practices by having a relationship between metadata.labels and selector.matchLabels fields.

Tested on devel with make helm-chart and executing multiple helm upgrade

@rooftopcellist
Copy link
Member

@miles-w-3 do you mind taking a look at this? If you are OK with this change, I can merge.

@miles-w-3
Copy link
Contributor

miles-w-3 commented Nov 9, 2022

I would be in favor of changing the label to just be the chart name rather than including the version, but it is not a functionality blocker to remove it. Generally, the helm.sh/chart is a nice way to get all resources deployed from a helm chart through kubectl. However, this label isn't present on the custom resources so we're currently not supporting it anyway.

I'm fine with removing it altogether, but maybe the best compromise would be to put just

helm.sh/chart: awx-operator

on all chart resources.

@rooftopcellist
Copy link
Member

@FlorianLaunay what do you think about the approach of changing the label?

@FlorianLaunay
Copy link
Contributor Author

FlorianLaunay commented Nov 14, 2022

@miles-w-3 @rooftopcellist TBH I hadn't thought about just removing the version in all labels 😇 but this is also fine for me. This will allow to continue to adhere to Helm best practices by having a relationship between metadata.labels and selector.matchLabels fields.
Should I modify this PR or open a new one ?

@janorn
Copy link
Contributor

janorn commented Nov 14, 2022

Do it in this PR.

@FlorianLaunay FlorianLaunay changed the title remove helm.sh/chart selector from helm chart deployment template remove helm chart version from labels Nov 14, 2022
@FlorianLaunay
Copy link
Contributor Author

Rebase and PR modification done

Copy link
Member

@rooftopcellist rooftopcellist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @FlorianLaunay !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Awx-operator Helm upgrade fails due to version labels in selector
4 participants