lint and validate dependency metadata to reference dependencies with … by dastrobu · Pull Request #9176 · helm/helm

dastrobu · 2020-12-28T18:21:32Z

…a unique key (name or alias)

Report charts with the following bad dependency specifications as bad charts:

dependencies:
- name: foo
  alias: baz # ← baz used twice
  version: 1.0.0
- name: bar
  alias: baz # ← baz used twice
  version: 1.0.0

dependencies:
- name: foo
  alias: bar # ← shadows chart below
  version: 1.0.0
- name: bar
  version: 1.0.0

dependencies:
- name: foo
  version: 1.0.0
- name: foo # ← chart with same name as above (although version or repo will be different, this will not work currently)
  version: 1.2.3

Closes #9169

Signed-off-by: Daniel Strobusch 1847260+dastrobu@users.noreply.github.com

What this PR does / why we need it:

Special notes for your reviewer:

If applicable:

this PR contains documentation
this PR contains unit tests
this PR has been tested for backwards compatibility

pkg/chart/metadata_test.go

sbose78

LGTM

dastrobu · 2021-06-25T17:51:11Z

Rebased onto current main.

jburnitz · 2022-07-02T04:19:26Z

why isn't this merged

dastrobu · 2022-12-08T19:08:15Z

Rebased onto current main.

pkg/chart/metadata_test.go

Maxine-van-der-Smissen

Note: I am not a maintainer.

Thanks for making the PR

…a unique key (name or alias) Report charts with the following bad dependency specifications as bad charts: dependencies: - name: foo alias: baz # ← baz used twice version: 1.0.0 - name: bar alias: baz # ← baz used twice version: 1.0.0 dependencies: - name: foo alias: bar # ← shadows chart below version: 1.0.0 - name: bar version: 1.0.0 dependencies: - name: foo version: 1.0.0 - name: foo # ← chart with same name as above (although version or repo will be different, this will not work currently) version: 1.2.3 Closes helm#9169 Signed-off-by: Daniel Strobusch <1847260+dastrobu@users.noreply.github.com>

…ndency-shadowing

joejulian

I tried being creative but I can't break this. Nice work!

sabre1041

LGTM

dbluxo · 2024-01-24T14:57:07Z

Hey @dastrobu 👋 , I'm now (v3.14.0) getting error messages, is this a non valid use case?:

dependencies:
  - name: kube-rbac-proxy
    version: "0.9.1"
    repository: "<...>"
    alias: loki-query-proxy
    condition: loki-query-proxy.enabled
  - name: kube-rbac-proxy
    version: "0.9.1"
    repository: "<...>"
    alias: loki-ingestion-proxy
    condition: loki-ingestion-proxy.enabled

index.go:366: skipping loading invalid entry for chart "<...>" "1.3.0" from <...>: validation: more than one dependency with name or alias "kube-rbac-proxy"

Or am I missing something?

bt909 · 2024-01-24T16:23:21Z

According to the documentation for the Chart.yaml using name twice with separate aliases seems to be okay.

Quote: alias: (optional) Alias to be used for the chart. Useful when you have to add the same chart multiple times

But now this seemed to be treated as error!

dastrobu · 2024-01-25T07:52:15Z

index.go:366: skipping loading invalid entry for chart "<...>" "1.3.0" from <...>: validation: more than one dependency with name or alias "kube-rbac-proxy"

Or am I missing something?

@dbluxo I guess your case should work as long as the repo and version is identical for both dependencies. Otherwise you will hit the case described in #9169 (undefined behaviour).
As this PR is closed, I'd suggest to create a new issue for this.
I can try to create a fix for this. Note that I am not a maintainer, though.

dbluxo · 2024-01-25T10:30:27Z

@dastrobu Thank you for your response. I have created an issue, would be great if our use case (same repository and version) would work again.

ziqianggeoffreychen · 2024-01-25T13:40:57Z

index.go:366: skipping loading invalid entry for chart "<...>" "1.3.0" from <...>: validation: more than one dependency with name or alias "kube-rbac-proxy"
Or am I missing something?

@dbluxo I guess your case should work as long as the repo and version is identical for both dependencies. Otherwise you will hit the case described in #9169 (undefined behaviour). As this PR is closed, I'd suggest to create a new issue for this. I can try to create a fix for this. Note that I am not a maintainer, though.

Hi @dastrobu , I opened an issue related to this change. In my case, the two dependencies are exactly the same with different aliases. But the alias is removed by JFrog in its index.yaml metadata. If all versions of the helm charts are with the same kind (aliases duplicate), it seems they will show up in search result. But that's not reliable. Refer to my issue for more details. Thanks.
#12744

helm-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 28, 2020

sbose78 reviewed Jan 4, 2021

View reviewed changes

pkg/chart/metadata_test.go Show resolved Hide resolved

dastrobu force-pushed the #9169/lint-dependency-shadowing branch from ff51cdf to a9f1df0 Compare January 6, 2021 09:56

sbose78 approved these changes Jan 19, 2021

View reviewed changes

dastrobu force-pushed the #9169/lint-dependency-shadowing branch from a9f1df0 to 1b3801a Compare June 25, 2021 05:08

joejulian added this to the 3.11.0 milestone Oct 4, 2022

dastrobu force-pushed the #9169/lint-dependency-shadowing branch from 1b3801a to a75251f Compare December 8, 2022 19:07

dastrobu force-pushed the #9169/lint-dependency-shadowing branch from a75251f to 4c23745 Compare December 8, 2022 19:12

dastrobu requested a review from sbose78 December 10, 2022 09:01

mattfarina modified the milestones: 3.11.0, 3.12.0 Jan 18, 2023

joejulian modified the milestones: 3.12.0, 3.13.0 May 5, 2023

Maxine-van-der-Smissen reviewed May 22, 2023

View reviewed changes

pkg/chart/metadata_test.go Outdated Show resolved Hide resolved

Maxine-van-der-Smissen approved these changes May 22, 2023

View reviewed changes

dastrobu force-pushed the #9169/lint-dependency-shadowing branch from 4c23745 to 6a4035a Compare May 23, 2023 07:03

Merge remote-tracking branch 'upstream/main' into helm#9169/lint-depe…

126333c

…ndency-shadowing

joejulian self-assigned this Aug 24, 2023

joejulian approved these changes Sep 6, 2023

View reviewed changes

joejulian added the Has One Approval This PR has one approval. It still needs a second approval to be merged. label Sep 6, 2023

mattfarina modified the milestones: 3.13.0, 3.14.0 Sep 25, 2023

sabre1041 approved these changes Jan 8, 2024

View reviewed changes

mattfarina approved these changes Jan 8, 2024

View reviewed changes

mattfarina merged commit 83a76ce into helm:main Jan 8, 2024

dastrobu deleted the #9169/lint-dependency-shadowing branch January 8, 2024 20:57

ziqianggeoffreychen mentioned this pull request Jan 24, 2024

Invalid helm charts are not fully excluded in helm search result #12744

Closed

dbluxo mentioned this pull request Jan 25, 2024

validation: more than one dependency with name or alias #12748

Closed

gjenkins8 mentioned this pull request Feb 8, 2024

Fix: Ignore alias validation error for index load #12789

Merged

3 tasks

Conversation

dastrobu commented Dec 28, 2020

Uh oh!

Uh oh!

sbose78 left a comment

Choose a reason for hiding this comment

Uh oh!

dastrobu commented Jun 25, 2021

Uh oh!

jburnitz commented Jul 2, 2022

Uh oh!

dastrobu commented Dec 8, 2022

Uh oh!

Uh oh!

Maxine-van-der-Smissen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joejulian left a comment

Choose a reason for hiding this comment

Uh oh!

sabre1041 left a comment

Choose a reason for hiding this comment

Uh oh!

dbluxo commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bt909 commented Jan 24, 2024

Uh oh!

dastrobu commented Jan 25, 2024

Uh oh!

dbluxo commented Jan 25, 2024

Uh oh!

ziqianggeoffreychen commented Jan 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Maxine-van-der-Smissen left a comment •

edited

Loading

dbluxo commented Jan 24, 2024 •

edited

Loading