Skip to content

Fixing precedence issue with the import of values.#12480

Merged
mattfarina merged 1 commit intohelm:mainfrom
mattfarina:fix-12460
Oct 12, 2023
Merged

Fixing precedence issue with the import of values.#12480
mattfarina merged 1 commit intohelm:mainfrom
mattfarina:fix-12460

Conversation

@mattfarina
Copy link
Copy Markdown
Collaborator

@mattfarina mattfarina commented Oct 11, 2023 •
edited
Loading

The ordering should be:

  1. User specified values (e.g CLI)
  2. Parent chart values
  3. Imported values
  4. Sub-chart values

This enables parent charts to import large set of values from a child and then override select values.

This change is needed for backwards compatibility.

Fixes #12460

What this PR does / why we need it: The values fixes in 3.13.0 had the wrong prioritization for values which broke backwards compatibility and did not make sense. Imported values should be able to be used (e.g., with a library chart) while being able override select values.

Special notes for your reviewer:

  • Tests that change here are restored to 3.12.x values for the tests.
  • This restores the previous ordering. See 3.12.3 here.

If applicable:

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

@mattfarina
Fixing precedence issue with the import of values.
25371e2 
The ordering should be:
1. User specified values (e.g CLI)
2. Parent chart values
3. Imported values
4. Sub-chart values

This enables parnet charts to import large set of values from a
child and then override select values.

This change is needed for backwards compatibility.

Fixes helm#12460

Signed-off-by: Matt Farina <matt.farina@suse.com>
@mattfarina mattfarina added the bug Categorizes issue or PR as related to a bug. label Oct 11, 2023
@mattfarina mattfarina added this to the 3.13.1 milestone Oct 11, 2023
@pull-request-size pull-request-size Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 11, 2023
@mattfarina mattfarina mentioned this pull request Oct 11, 2023
Closed
@gjenkins8
Copy link
Copy Markdown
Member

gjenkins8 commented Oct 11, 2023

LGTMe.

Tested this change against the example in #12460, looks good to me (it was also required to update the example chart's values image: tag: "" to be "latest" to produce valid output yaml; same as for Helm 3.12).

joejulian
joejulian approved these changes Oct 11, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@joejulian joejulian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passes every test I have to throw at it.

@atkrad atkrad mentioned this pull request Oct 11, 2023
Closed
@zegerius
Copy link
Copy Markdown
Contributor

zegerius commented Oct 12, 2023

This also fixes our regression, tested with https://github.com/ankitabhopatkar13/helm-values-issue.

$ diff <(/opt/homebrew/Cellar/helm/3.13.0/bin/helm upgrade --install umbrella umbrella --dry-run --values umbrella/values.yaml) <(/usr/local/bin/helm upgrade --install umbrella umbrella --dry-run --values umbrella/values.yaml)
25c25
<     - port: 9090
---
>     - port: 1234
47c47
<     - port: 9090
---
>     - port: 8080

and the diff between 3.12.3 and this PR is zero!

$ diff <(/opt/homebrew/Cellar/helm/3.12.3/bin/helm upgrade --install umbrella umbrella --dry-run --values umbrella/values.yaml) <(/usr/local/bin/helm upgrade --install umbrella umbrella --dry-run --values umbrella/values.yaml)

@mattfarina mattfarina merged commit fe595b6 into helm:main Oct 12, 2023
@mattfarina mattfarina deleted the fix-12460 branch October 12, 2023 13:10
@mattfarina mattfarina added needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. picked Indicates that a PR has been cherry-picked into the next release candidate. and removed needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. labels Oct 12, 2023
@gjenkins8 gjenkins8 mentioned this pull request Oct 13, 2023
Closed
@intetinte
Copy link
Copy Markdown

intetinte commented Oct 18, 2023 •
edited
Loading

This change has a flaw in the sense that the values defined in a sub-chart will not get the values from an import. The import only populates non-existing data in the sub-chart values.

Example:

exports:
sub-chart:
A:
A: hello
B: good
C: world
D: sunshine

Sub-chart values:
A:
A: null
B: bad
D:

Result:
A:
A:
B: bad
C: world
D:

Can add that the current order of precedence basically is:

1. User specified values (e.g CLI)
2. Sub-chart values
3. Imported values
4. Parent chart values

@intetinte intetinte mentioned this pull request Oct 19, 2023
Closed
@gjenkins8 gjenkins8 mentioned this pull request Dec 6, 2023
Closed
@gjenkins8 gjenkins8 mentioned this pull request Jan 5, 2024
Closed
@hedayat hedayat mentioned this pull request Jan 19, 2024
Closed
@joaocc joaocc mentioned this pull request Jan 30, 2024
Closed
@intetinte intetinte mentioned this pull request Feb 26, 2024
Closed
@scottrigby scottrigby mentioned this pull request May 16, 2024
Closed
3 tasks
@pisto pisto mentioned this pull request Jun 12, 2024
Merged
@kuwv kuwv mentioned this pull request Nov 14, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joejulian joejulian joejulian approved these changes

Assignees

No one assigned

Labels

bug Categorizes issue or PR as related to a bug. picked Indicates that a PR has been cherry-picked into the next release candidate. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

3.13.1

Development

Successfully merging this pull request may close these issues.

Helm 3.13 is not backward compatible with 3.12

5 participants

@mattfarina @gjenkins8 @zegerius @intetinte @joejulian