Skip to content

Conversation

@logand22
Copy link
Contributor

@logand22 logand22 commented Dec 4, 2023

Changes selector labels to not change per helm release to fix #140. These new selectors are more similar to https://github.com/cert-manager/cert-manager/blob/202a80e2184b1b479dbb5d03f32aafc4b2c811b9/deploy/charts/cert-manager/templates/service.yaml#L13-L14.

This might require a major version bump of helm chart version because it will also cause issues when upgrading to this.

@logand22 logand22 requested a review from davidcollom as a code owner December 4, 2023 21:55
@davidcollom
Copy link
Collaborator

I agree that this is a breaking change - but the helm chart and application version are still on a 0.x release basis which should be infured as "beta" or unstable in some respect.

I feel that the right approach was to move to the helper method over defining the labels multiple times being much more error-prone

@logand22
Copy link
Contributor Author

I agree that this is a breaking change - but the helm chart and application version are still on a 0.x release basis which should be infured as "beta" or unstable in some respect.

Understandable.

@logand22
Copy link
Contributor Author

I feel that the right approach was to move to the helper method over defining the labels multiple times being much more error-prone.

Would you suggest removing this line then to reduce repetition but still fix the issue?

helm.sh/chart: {{ include "version-checker.chart" . }}

@davidcollom
Copy link
Collaborator

I feel that the right approach was to move to the helper method over defining the labels multiple times being much more error-prone.

Would you suggest removing this line then to reduce repetition but still fix the issue?

helm.sh/chart: {{ include "version-checker.chart" . }}

Yea, sure - I'd accept a PR with that line removed! 👍

@logand22
Copy link
Contributor Author

Ended up creating a separate helper instead since I think having the helm.sh/chart is actually helpful outside of the selector templates since it describes the version of the helm chart.

Let me know if you have any more concerns.

Copy link
Collaborator

@davidcollom davidcollom left a comment

Choose a reason for hiding this comment

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

I like that approach much better 👍

@davidcollom davidcollom merged commit 8473f86 into jetstack:main Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Helm Chart Upgrades Fail due to immutable selector fields in deployment.

3 participants