search

jetstack / version-checker

Kubernetes utility for exposing image versions in use, compared to latest available upstream, as metrics.
https://jetstack.io
Apache License 2.0
665 stars 76 forks source link

Update selector labels for Deployment and Service #141

Closed logand22 closed 4 months ago

logand22 commented 7 months ago

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.

davidcollom commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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? https://github.com/jetstack/version-checker/blob/e3502202046823d07ef8bd60b7eb542b23d4eaad/deploy/charts/version-checker/templates/_helpers.tpl#L21

davidcollom commented 4 months ago

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?

https://github.com/jetstack/version-checker/blob/e3502202046823d07ef8bd60b7eb542b23d4eaad/deploy/charts/version-checker/templates/_helpers.tpl#L21

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

logand22 commented 4 months ago

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.