H4HIP: Forward compatibility: `Chart.yaml` `minimumHelmVersion` field #370

gjenkins8 · 2024-11-19T06:01:35Z

No description provided.

Signed-off-by: George Jenkins <[email protected]>

marckhouzam

I like this

hips/hip-XXXX.md

Signed-off-by: George Jenkins <[email protected]>

mattfarina

I like the idea of a minimumHelmVersion field. I have some thoughts...

There is a capabilities property for the current version of Helm and it's possible to encode an error rendering into a template when you don't meet the minimum version. It's not as elegant as this.
Do we have some other new chart features so that we should have a new chart version? We are currently on chart apiVersion v2.
I'm questioning the required nature of this and when to keep it updated. Should this only be present if some new thing was introduced along the way? And, should it only have the minimum version for the oldest version that would support the chart? I'm thinking about end-users who use a WIDE range of Helm versions. There are so many old Helm versions in use and we aren't going to see this change.

mattfarina · 2024-11-21T14:58:35Z

hips/hip-XXXX.md

+The minor and patch fields are optional.
+And will be inferred as zeros if not specified.
+
+When loading a chart, iff the `minimumHelmVersion` exists, Helm will verify its version exceeds or equals the version constraint specified in the field.


iff -> if

This is one of my favorite contractions :) iff -> if and only if

https://www.merriam-webster.com/dictionary/iff

ie. the above reads like:

When loading a chart, if and only if the minimumHelmVersion exists, Helm will verify its version exceeds or equals the version constraint specified in the field.

I would think we would want to omit the use of abbreviations as the meaning may not be broadly understood, especially for those where English is not a first language

ok, fair. will drop usage of iff

mattfarina · 2024-11-21T14:59:06Z

hips/hip-XXXX.md

+
+When loading a chart, iff the `minimumHelmVersion` exists, Helm will verify its version exceeds or equals the version constraint specified in the field.
+
+When "strict" loading of `Chart.yaml` is used: ie. the loading of `Chart.yaml` errors when unknown fields are present, Helm will first 'peek' into `Chart.yaml` and extract the `minimumHelmVersion` field (iff it exists) and perform the version constraint check at this point.


iff -> if

mattfarina · 2024-11-21T15:21:48Z

hips/hip-XXXX.md

+While this may not be strictly necessary, it will provide a backstop for features that may be included in the chart by the user for their current Helm version or `helm create`.
+The user can remove/reduce the specified `minimumHelmVersion` if desired.
+
+`helm lint` will produce a warning, if the current version of Helm is newer than the minimum version constraint.


A warning if the current version is newer? Do you mean, if the current version is older?

I agree this is too aggressive.

I did originally mean newer, with the intention as follows. Will address in #370 (comment)

mattfarina · 2024-11-21T15:27:23Z

hips/hip-XXXX.md

+
+`helm lint` will produce a warning, if the current version of Helm is newer than the minimum version constraint.
+Or the `minimumHelmVersion` field is unset for the given chart.
+This is to encourage users to update the minimum version to currently utilized version of Helm (that the user is using to develop the chart).


We see a difference between people who create a chart and those who consume charts (see the profiles/roles). Consider all the charts people pull from repositories listed on Artifact Hub.

There is also a large variety of different versions of Helm in use. In our previous CDN we had data on this. While the majority were using a relatively new version, it was not unusual for people to use older version. Even those over 2 years old.

Even in those final reports, from just a few months ago, there were cases of people pulling 3.0.0 regularly. A bad idea but it's happening. Versions in use go further back than we would like.

If we want to make charts widely usable, do we want to have a relatively recent version of Helm set as the minimum version if there is no feature in a newer version of Helm (e.g., new template function) being used? What is the benefit?

mattfarina · 2024-11-21T15:34:41Z

hips/hip-XXXX.md

+
+## Abstract
+
+This HIP proposes supporting a `minimumHelmVersion` field in `Chart.yaml`.


We already have a property called KubeVersion to specify a range for Kubernetes support. Should this be called HelmVersion and accept a range in the same way? The range syntax being the same used elsewhere in Helm.

Happy to call the field HelmVersion (naming is hard :) )

I don't think we should/need to support anything for than a single, implied to be the minimum version, constraint however. I've addressed this here:

https://github.com/helm/community/pull/370/files#diff-e94b50b2c7a2d45c81db342448ae78950464d77488a62b315a2b715549fe1a95R95

sabre1041

Looks good. A few comments

hips/hip-XXXX.md

sabre1041 · 2024-11-24T11:15:42Z

hips/hip-XXXX.md

+The minor and patch fields are optional.
+And will be inferred as zeros if not specified.
+
+When loading a chart, iff the `minimumHelmVersion` exists, Helm will verify its version exceeds or equals the version constraint specified in the field.


I would think we would want to omit the use of abbreviations as the meaning may not be broadly understood, especially for those where English is not a first language

hips/hip-XXXX.md

sabre1041 · 2024-11-24T11:22:28Z

hips/hip-XXXX.md

+When "strict" loading of `Chart.yaml` is used: ie. the loading of `Chart.yaml` errors when unknown fields are present, Helm will first 'peek' into `Chart.yaml` and extract the `minimumHelmVersion` field (iff it exists) and perform the version constraint check at this point.
+This is to provide better UX that simply erroring on any potential new fields in `Chart.yaml` that are unknown to the current version of Helm.
+
+`helm create` will pre-fill Chart.yaml's `minimumHelmVersion` field with the current version of Helm.


Should this new proposed parameter be added at chart instantiation? Other similar values, like KubeVersion are not added automatically

hips/hip-XXXX.md

sabre1041 · 2024-11-24T16:27:37Z

hips/hip-XXXX.md

+It is thought a "minimum" constraint is preferred over a generic "constraint" system.
+ie. where a user could list multiple version constraints to be satisfied
+Similar to e.g. [pip's requirement-specifiers][3].
+This is because Helm makes great effort to remain [backwards compatible][4].


Suggested change

This is because Helm makes great effort to remain [backwards compatible][4].

This is because Helm makes great effort to remain [backwards compatibility][4].

hips/hip-XXXX.md

sabre1041 · 2024-11-24T16:34:40Z

hips/hip-XXXX.md

+However, since core Helm capabilities are only every additive due to Helm's [backwards-compatibility rules][4], a capabilities based system is redundant.
+
+*NB: at the time of writing, plugins for extending Helm's functionality are being heavily discussed.
+And while plugins will likely allow extending capabilities independent of the Helm version.


Combine with the following sentence? Its currently a fragment

oops, right

Co-authored-by: Andrew Block <[email protected]> Signed-off-by: George Jenkins <[email protected]>

pull-request-size bot added the size/L label Nov 19, 2024

gjenkins8 changed the title ~~Forward compatibility HIP~~ HIP: Forward compatibility: Chart.yaml minimumHelmVersion Nov 19, 2024

gjenkins8 force-pushed the hip_forward_compat branch from 50a2bd5 to dcd384b Compare November 19, 2024 06:53

Forward compatibility HIP

47c276b

Signed-off-by: George Jenkins <[email protected]>

gjenkins8 force-pushed the hip_forward_compat branch from dcd384b to 47c276b Compare November 19, 2024 06:53

marckhouzam reviewed Nov 19, 2024

View reviewed changes

hips/hip-XXXX.md Outdated Show resolved Hide resolved

hips/hip-XXXX.md Outdated Show resolved Hide resolved

hips/hip-XXXX.md Outdated Show resolved Hide resolved

hips/hip-XXXX.md Outdated Show resolved Hide resolved

gjenkins8 changed the title ~~HIP: Forward compatibility: Chart.yaml minimumHelmVersion~~ HIP: Forward compatibility: Chart.yaml minimumHelmVersion field Nov 19, 2024

gjenkins8 added 2 commits November 19, 2024 09:34

grammar

8322920

Signed-off-by: George Jenkins <[email protected]>

tidy up a little

ed5860a

Signed-off-by: George Jenkins <[email protected]>

gjenkins8 changed the title ~~HIP: Forward compatibility: Chart.yaml minimumHelmVersion field~~ H4HIP: Forward compatibility: Chart.yaml minimumHelmVersion field Nov 19, 2024

typoi

9c88194

Signed-off-by: George Jenkins <[email protected]>

mattfarina reviewed Nov 21, 2024

View reviewed changes

sabre1041 reviewed Nov 24, 2024

View reviewed changes

gjenkins8 mentioned this pull request Nov 24, 2024

H4HIP: Default to strict Chart.yaml loading #371

Open

gjenkins8 and others added 6 commits November 24, 2024 10:35

Update hips/hip-XXXX.md

aaa9759

Co-authored-by: Andrew Block <[email protected]> Signed-off-by: George Jenkins <[email protected]>

Update hips/hip-XXXX.md

395980c

Co-authored-by: Andrew Block <[email protected]> Signed-off-by: George Jenkins <[email protected]>

Update hips/hip-XXXX.md

16aaa96

Co-authored-by: Andrew Block <[email protected]> Signed-off-by: George Jenkins <[email protected]>

Update hips/hip-XXXX.md

32fbf22

Co-authored-by: Andrew Block <[email protected]> Signed-off-by: George Jenkins <[email protected]>

Update hips/hip-XXXX.md

c46f37a

Co-authored-by: Andrew Block <[email protected]> Signed-off-by: George Jenkins <[email protected]>

Update hips/hip-XXXX.md

65777bb

Co-authored-by: Andrew Block <[email protected]> Signed-off-by: George Jenkins <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

H4HIP: Forward compatibility: `Chart.yaml` `minimumHelmVersion` field #370

H4HIP: Forward compatibility: `Chart.yaml` `minimumHelmVersion` field #370

gjenkins8 commented Nov 19, 2024

marckhouzam left a comment

mattfarina left a comment

mattfarina Nov 21, 2024

gjenkins8 Nov 21, 2024 •

edited

Loading

sabre1041 Nov 24, 2024

gjenkins8 Nov 24, 2024

mattfarina Nov 21, 2024

mattfarina Nov 21, 2024

gjenkins8 Nov 24, 2024

mattfarina Nov 21, 2024

mattfarina Nov 21, 2024

gjenkins8 Nov 24, 2024

sabre1041 left a comment

sabre1041 Nov 24, 2024

sabre1041 Nov 24, 2024

sabre1041 Nov 24, 2024

sabre1041 Nov 24, 2024

gjenkins8 Nov 29, 2024


		When loading a chart, iff the `minimumHelmVersion` exists, Helm will verify its version exceeds or equals the version constraint specified in the field.

		When "strict" loading of `Chart.yaml` is used: ie. the loading of `Chart.yaml` errors when unknown fields are present, Helm will first 'peek' into `Chart.yaml` and extract the `minimumHelmVersion` field (iff it exists) and perform the version constraint check at this point.


		## Abstract

		This HIP proposes supporting a `minimumHelmVersion` field in `Chart.yaml`.

	This is because Helm makes great effort to remain [backwards compatible][4].
	This is because Helm makes great effort to remain [backwards compatibility][4].

H4HIP: Forward compatibility: Chart.yaml minimumHelmVersion field #370

Are you sure you want to change the base?

H4HIP: Forward compatibility: Chart.yaml minimumHelmVersion field #370

Conversation

gjenkins8 commented Nov 19, 2024

marckhouzam left a comment

Choose a reason for hiding this comment

mattfarina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjenkins8 Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabre1041 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

H4HIP: Forward compatibility: `Chart.yaml` `minimumHelmVersion` field #370

H4HIP: Forward compatibility: `Chart.yaml` `minimumHelmVersion` field #370

gjenkins8 Nov 21, 2024 •

edited

Loading