Skip to content

Conversation

snay2
Copy link
Contributor

@snay2 snay2 commented Feb 1, 2022

Issue #, if available: N/A

Description of changes: The Helm chart version has been out of sync with the NTH app version for a long time. NTH v1.14.1 contained some breaking changes to the Helm chart, so this is a good opportunity to get them back in sync.

A corresponding PR will be opened to https://github.com/aws/eks-charts after this.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

1.14.1 also contained breaking changes to the Helm chart, so we would
  rather release that as a major or minor version rather than a patch
  version
@snay2 snay2 requested a review from a team as a code owner February 1, 2022 17:45
@stevehipwell
Copy link
Contributor

I'd advise against trying to match the chart and app versions, how about chart v1.0.0?

@snay2
Copy link
Contributor Author

snay2 commented Feb 1, 2022

I'd advise against trying to match the chart and app versions

@stevehipwell Tell me more. Why might matching the versions be a bad idea?

@stevehipwell
Copy link
Contributor

@stevehipwell Tell me more. Why might matching the versions be a bad idea?

Because the versioning of the chart shouldn't be tied to the app/binary as the versions serve two completely different purposes, usually a change in the app will be reflected in the chart (but not always) but the chart version can increment independently of the app version. This is a common issue in aggregation scenarios.

One example I know of currently is the Cluster Autoscaler team currently trying to figure out how their versioning strategy of using K8s major + K8s minor as their version base and then only being able to set the patch version would allow for a mid release feature update.

Another closer to home example would be Helm 4 changing the chart API to v3 which would need a major chart version upgrade even if nothing changed in the payload. I had a slightly less extreme version of this in my charts where I needed to make ingress changes and had to bump all the major versions.

@snay2 snay2 marked this pull request as draft February 1, 2022 19:35
@bwagner5
Copy link
Contributor

bwagner5 commented Feb 1, 2022

I see your point @stevehipwell. A lot of charts in the eks-charts repo don't even really version the helm charts (much like how we've handled our version up to this point... just increment to increment). Not saying that's good just the state of things right now. In general, I like to tie the helm chart release as closely as possible with the NTH version release since at the end of the day, the two are very coupled. So for example, if there was an issue in the helm chart causing an issue, maybe a parameter isn't passed properly to the NTH args parsing functionality. For simplicity, I'd view that bug as a bug for both the helm chart and the NTH version because of the coupling (I'm also happy to be told that's completely wrong since maybe there's a non-trivial group of people consuming NTH plain yaml files from the releases, that may or may not have the distribution bug... although they are generated from the helm chart). But in the case of any bug, I think we'd want to cut a new NTH version release and allow for our full suite of tests and automations to run to verify both the helm chart, NTH's application logic, and the two together (also ec2-bot cutting the release to eks-charts).

@snay2
Copy link
Contributor Author

snay2 commented Feb 1, 2022

Brandon is correct that we don't have the tooling or the need to release the helm chart independently of the NTH app itself, so for now we must continue to increment both versions every time we need to make a change in either. However, they don't necessarily need to match, and Steve has presented good reasons for why they should not be identical.

I think our best course of action now is to leave the helm chart version at 0.16.1, since that is the version number referenced in the NTH v1.14.1 release. In the future, we'll be more cognizant of breaking changes in the helm chart and ensure that the version number reflects that reality.

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.

3 participants