fix(*): Helm v3 handling of APIVersion v1 charts dependencies #7009

TBBle · 2019-11-19T01:29:34Z

This fixes unexpected changes in APIVersion1 (Helm v2) Charts when they are operated on by Helm v3 using package or dependency update.

The fixes:

requirements.yaml was being unconditionally merged into Chart.yaml, which produced packages which failed helm lint.
requirements.lock was being converted into Chart.lock by helm dep up on an APIVersion1 chart.
The code that loads requirements.lock and requirements.yaml in APIVersion1 charts for backwards compatibility, was incorrectly excluding those files from helm package and the Files collection in templates. As well as Files behaviour changing, this also broke sub-chart aliases for Helm v2 as those are stored in requirements.yaml.

Helm v3 incorrectly reads aliases from Chart.yaml's dependencies map in APIVersion1 charts, but I didn't change that, as helm lint warns you that this is an unexpected combination already, and it's as well as requirements.yaml so doesn't break anything.

The changes in the third commit (ae74c91) are possibly not go-idiomatic, and I specifically welcome feedback or suggested improvements.

Fixes: #6974

TBBle · 2019-11-20T07:20:08Z

I haven't added tests to cover the APIVersion v1 behaviours; the changes were visible in the test runs as they broke the dependency tests which were testing APIVersion v2 behaviours with a generated APIVersion v1 chart. Instead I changed those tests to test the behaviour of APIVersion v2 charts, and they pass again.

TBBle · 2019-12-06T02:34:14Z

Is there any kind of process to go through to have a PR reviewed? This's been sitting idle for a couple of weeks now, and I'd love to know if there's more I need to do.

hickeyma

@TBBle Thank you for pushing this PR. It is a good fix. I have added some comments for consideration.
One other change is that the requirements.lock file should not be saved when packaging. The check could potentially be added before: https://github.com/helm/helm/blob/master/pkg/chartutil/save.go#L182

cmd/helm/dependency_update_test.go

pkg/chartutil/chartfile.go

pkg/chartutil/save.go

hickeyma · 2019-12-09T16:27:24Z

@TBBle Thanks for the details. Some additional feedback below.

requirements.yaml was being unconditionally merged into Chart.yaml, which produced packages which failed helm lint.

I am unable to reproduce this. Can you give more details about it?

The changes in the third commit (ae74c91) are possibly not go-idiomatic, and I specifically welcome feedback or suggested improvements.

Added feedback in #7009 (comment)

TBBle · 2019-12-10T11:07:18Z

Why should requirements.lock be excluded from packaging of APIVersion1 charts? It was included in packages generated by Helm v2, reconfirmed with Helm v2.16.2 with the same setup as the following repro case.

TBBle · 2019-12-10T11:23:08Z

requirements.yaml was being unconditionally merged into Chart.yaml, which produced packages which failed helm lint.

I am unable to reproduce this. Can you give more details about it?

With Helm v2.16.2:

helm create testchart

Add a requirements.yaml:

dependencies:
  - name: magic-ip-address
    version: ">0"
    repository: "@stable"

With Helm v2.16.2:

helm dependency update testchart

With Helm v3.0.0 or v3.0.1

helm package testchart
helm lint testchart-0.1.0.tgz

This will reproduce the failure:

==> Linting .\testchart-0.1.0.tgz
[INFO] Chart.yaml: icon is recommended
[ERROR] Chart.yaml: dependencies are not valid in the Chart file with apiVersion 'v1'. They are valid in apiVersion 'v2'   
Error: 1 chart(s) linted, 1 chart(s) failed

hickeyma · 2019-12-10T11:59:41Z

Why should requirements.lock be excluded from packaging of APIVersion1 charts? It was included in packages generated by Helm v2, reconfirmed with Helm v2.16.2 with the same setup as the following repro case

I agree that it is included in Helm 2 but the lock file is not included in Helm 3. Is it still needed in the package as you will have the dependencies included? I would probably like to mirror Helm 3 in this regard.

hickeyma · 2019-12-10T12:01:00Z

requirements.yaml was being unconditionally merged into Chart.yaml, which produced packages which failed helm lint.

Thanks for clarification of this in #7009 (comment). I missed the subtly of doing update first with Helm 2.

TBBle · 2019-12-11T00:11:37Z

Why should requirements.lock be excluded from packaging of APIVersion1 charts? It was included in packages generated by Helm v2, reconfirmed with Helm v2.16.2 with the same setup as the following repro case

I agree that it is included in Helm 2 but the lock file is not included in Helm 3. Is it still needed in the package as you will have the dependencies included? I would probably like to mirror Helm 3 in this regard.

Personally, I'd keep it around for API stability reasons, in case someone's scripts are using .Files to extract the real subchart versions from requirements.lock or similar. It seems a reasonably-arbitrary API break for v1 to suddenly disappear the file.

Fixes #6974. This ensures that when reading a Chart marked with APIVersion v1, we maintain the behaviour of Helm v2 and include the requirements.yaml and requirements.lock in the Files collection, and hence produce charts that work correctly with Helm v2. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net>

This keeps the on-disk format consistent after `helm dependency update` of an APIVersion1 Chart. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net>

This fixes `helm lint` against an APIVersion1 chart packaged with Helm v3. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net>

As the generated chart contains no requirements.yaml in its files list, but has dependencies in its metadata, it is not a valid APIVersion v1 chart. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net>

Specifically for the charts that have dependencies, the generated chart contains no requirements.yaml in its files but has dependencies in its metadata. Hence it is not a valid APIVersion v1 chart. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net>

hickeyma

LGTM, thanks for the fix @TBBle

technosophos · 2019-12-17T17:47:32Z

picked into release-3.0

* Include requirements.* as Files in APIVersionV1 Fixes #6974. This ensures that when reading a Chart marked with APIVersion v1, we maintain the behaviour of Helm v2 and include the requirements.yaml and requirements.lock in the Files collection, and hence produce charts that work correctly with Helm v2. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net> * Write out requirements.lock for APIVersion1 Charts This keeps the on-disk format consistent after `helm dependency update` of an APIVersion1 Chart. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net> * Exclude 'dependencies' from APVersion1 Chart.yaml This fixes `helm lint` against an APIVersion1 chart packaged with Helm v3. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net> * Generate APIVersion v2 charts for dependency tests As the generated chart contains no requirements.yaml in its files list, but has dependencies in its metadata, it is not a valid APIVersion v1 chart. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net> * Generate APIVersion v2 charts for manager tests Specifically for the charts that have dependencies, the generated chart contains no requirements.yaml in its files but has dependencies in its metadata. Hence it is not a valid APIVersion v1 chart. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net>

Nitive · 2019-12-24T16:28:23Z

@TBBle @hickeyma generating requirements.lock instead of Chart.lock was a breaking change for helmfile, you can see details in roboll/helmfile#1052. Not sure if it should be fixed on a helm side

TBBle · 2019-12-29T23:52:33Z

As noted in roboll/helmfile#1052, the change back to requirements.yaml for Helm APIVersion 1 charts was reverting an incorrect API breakage, and has now been fixed in helmfile, which was incorrectly generating APIVersion 1 (by default) charts with the APIVersion 2 Chart.lock; so were the unit tests in Helm v3 itself until this fix, so it's not surprising, just a shame this wasn't caught earlier in the Helm v3 release cycle.

) * Include requirements.* as Files in APIVersionV1 Fixes helm#6974. This ensures that when reading a Chart marked with APIVersion v1, we maintain the behaviour of Helm v2 and include the requirements.yaml and requirements.lock in the Files collection, and hence produce charts that work correctly with Helm v2. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net> * Write out requirements.lock for APIVersion1 Charts This keeps the on-disk format consistent after `helm dependency update` of an APIVersion1 Chart. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net> * Exclude 'dependencies' from APVersion1 Chart.yaml This fixes `helm lint` against an APIVersion1 chart packaged with Helm v3. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net> * Generate APIVersion v2 charts for dependency tests As the generated chart contains no requirements.yaml in its files list, but has dependencies in its metadata, it is not a valid APIVersion v1 chart. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net> * Generate APIVersion v2 charts for manager tests Specifically for the charts that have dependencies, the generated chart contains no requirements.yaml in its files but has dependencies in its metadata. Hence it is not a valid APIVersion v1 chart. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net>

helm-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 19, 2019

hickeyma added awaiting review bug Categorizes issue or PR as related to a bug. labels Dec 9, 2019

hickeyma requested changes Dec 9, 2019

View reviewed changes

cmd/helm/dependency_update_test.go Show resolved Hide resolved

pkg/chartutil/chartfile.go Outdated Show resolved Hide resolved

pkg/chartutil/save.go Outdated Show resolved Hide resolved

hickeyma changed the title ~~Fix APIVersion1 charts breakages when mutated with Helm v3~~ fix(*): Helm v3 handling of APIVersion v1 charts dependencies Dec 9, 2019

Paul "Hampy" Hampson added 5 commits December 11, 2019 11:30

Write out requirements.lock for APIVersion1 Charts

a911d7f

This keeps the on-disk format consistent after `helm dependency update` of an APIVersion1 Chart. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net>

Exclude 'dependencies' from APVersion1 Chart.yaml

e984e5b

This fixes `helm lint` against an APIVersion1 chart packaged with Helm v3. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net>

Generate APIVersion v2 charts for dependency tests

2396573

As the generated chart contains no requirements.yaml in its files list, but has dependencies in its metadata, it is not a valid APIVersion v1 chart. Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net>

hickeyma approved these changes Dec 11, 2019

View reviewed changes

hickeyma merged commit 0cb0eac into helm:master Dec 11, 2019

hickeyma removed the awaiting review label Dec 11, 2019

hickeyma modified the milestones: 2.16.2, 3.0.2 Dec 11, 2019

TBBle deleted the include_requirements_files_if_present branch December 13, 2019 01:38

hickeyma mentioned this pull request Dec 16, 2019

Lock digest differs when performing dependency build on chart in Helm 2 and then Helm 3 #7233

Closed

technosophos added the picked Indicates that a PR has been cherry-picked into the next release candidate. label Dec 17, 2019

Nitive mentioned this pull request Dec 24, 2019

helmfile deps is failing with Chart.lock: no such file or directory with helm 3.0.2 roboll/helmfile#1052

Closed

stefanprodan mentioned this pull request Jan 7, 2020

Update Helm packages to 3.0.2 and 2.16.1 fluxcd/helm-operator#177

Merged

hickeyma mentioned this pull request Jan 27, 2020

helm 3.0.2 dependency update builds requirements.lock instead of Chart.lock file #7464

Closed

iawia002 mentioned this pull request Jul 31, 2023

Update the chart version to v2 to fix the dependency dropping issue kubesphere/ksbuilder#42

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(*): Helm v3 handling of APIVersion v1 charts dependencies #7009

fix(*): Helm v3 handling of APIVersion v1 charts dependencies #7009

TBBle commented Nov 19, 2019

TBBle commented Nov 20, 2019

TBBle commented Dec 6, 2019

hickeyma left a comment

hickeyma commented Dec 9, 2019

TBBle commented Dec 10, 2019 •

edited

TBBle commented Dec 10, 2019

hickeyma commented Dec 10, 2019

hickeyma commented Dec 10, 2019

TBBle commented Dec 11, 2019

hickeyma left a comment •

edited

technosophos commented Dec 17, 2019

Nitive commented Dec 24, 2019 •

edited

TBBle commented Dec 29, 2019

fix(*): Helm v3 handling of APIVersion v1 charts dependencies #7009

fix(*): Helm v3 handling of APIVersion v1 charts dependencies #7009

Conversation

TBBle commented Nov 19, 2019

TBBle commented Nov 20, 2019

TBBle commented Dec 6, 2019

hickeyma left a comment

Choose a reason for hiding this comment

hickeyma commented Dec 9, 2019

TBBle commented Dec 10, 2019 • edited

TBBle commented Dec 10, 2019

hickeyma commented Dec 10, 2019

hickeyma commented Dec 10, 2019

TBBle commented Dec 11, 2019

hickeyma left a comment • edited

Choose a reason for hiding this comment

technosophos commented Dec 17, 2019

Nitive commented Dec 24, 2019 • edited

TBBle commented Dec 29, 2019

TBBle commented Dec 10, 2019 •

edited

hickeyma left a comment •

edited

Nitive commented Dec 24, 2019 •

edited