Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing precedence issue with the import of values. #12480

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

mattfarina
Copy link
Collaborator

@mattfarina mattfarina commented Oct 11, 2023

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

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
@gjenkins8
Copy link
Contributor

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).

Copy link
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.

@zegerius
Copy link
Contributor

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
5 checks passed
@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
@intetinte
Copy link

intetinte commented Oct 18, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Helm 3.13 is not backward compatible with 3.12
5 participants