fix: do not merge and import values from disabled dependencies #8679
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the 1st PR splitted from helm#6876 Golang allows implicit conversion between `map[string]interface{}` and `chartutil.Values`, but not when converted to `interface{}` first. The usage of `chartutil.Values` as values of a root `chartutil.Values` leads to various bugs, only avoided by the usage of `.AsMap` and some random checks. This PR finally gets rid of those random bugs by limitating the usage of `chartutil.Values` and preventing its conversion to `iterface{}`. - All functions now return `map[string]interface{}`. Implicit conversion to `chartutil.Values` makes those changes quite transparent, and avoid accidentally turning `chartutil.Values` to `interface{}`. - All creations of a `chartutil.Values{...}` have been replaced by a `map[string]interface{}{...}`. - In general, `chartutil.Values` are only used in one of those 2 cases: - when `chartutil.Values` methods are used (often as argument of a function, implicitly converted on function call) - when `chartutil.Values` represents the root value for templates (as in `chart.Values` expressions) - `chartutil.Values.AsMap` has been removed to discorage the usage of `chartutil.Values` (implicit conversion works anyway). - Tries to convert `interface{}` to `chartutil.Values` have also been removed to avoid hiding bad usages of `chartutil.Values`. Signed-off-by: Aurélien Lambert <aure@olli-ai.com>
This is the 2nd PR splitted from helm#6876, and should be merged after helm#8677 Prior to this PR, values from disabled dependencies would still be merged and imported to the parent values. This didn't have much consequences, but is incompatible with an implemention of values templating. This PR makes the following changes: - `pkg/chartutil/coalesce.go` and `pkg/chartutil/dependencies.go` are no longer recursive, recursivity is handled by `pkg/engine/engine.go` - `chartutils.ToRenderValues` does not merge chart values anymore, this is done by `pkg/engine/engine.go` - `pkg/engine/engine.go` now uses a recursive `Engine.updateRenderValues` function that: - parses and merge `values.yaml` - validates the values along the schema - evaluates if sub-charts are enabled - recursively treat the enabled subcharts - import the values of the enabled subcharts - some `pkg/actions` have been addapted to the new way of merging values Signed-off-by: Aurélien Lambert <aure@olli-ai.com>
helm-bot
added
the
size/XXL
aure-olli
added a commit
to aure-olli/helm
that referenced
this pull request
Sep 3, 2020
This is the 3rd and last PR splitted from helm#6876, and should be merged after helm#8677 and helm#8679 There have been many requests for a way to use templates for `values.yaml` (helm#2492, helm#2133, ...). The main reason being that it's currently impossible to derive the values of the subcharts from templates. However, having templates in `values.yaml` make them unparsable and create a "chicken or the egg" problem when rendering them. This PR offers an intuitive solution without such a problem: an optional `values/` directory which templates are rendered using `values.yaml`, and then merge them with it. Those `values/` templates are only required for specific cases, work the same way as `templates/` templates, and `values.yaml` will remain the main parsable source of value. Values evaluation is now made in 2 step. First the `values.yaml` file is read. Then the `values/***.yaml` files are rendered using those values (`values/_***.tpl` files ara still allowed as helpers), and are used to update the values. For each chart and subchart, the following steps are performed: 1. If the chart has a parent, parent sub-values and global values are retrieved. 2. The chart values are coalesced with the retrieved value, the chart values are not authoritative. 3. The chart `values/` templates are rendered, the same way `templates/` templates are. The values are not updated during this step. 4. The values are updated with the rendered `values/` templates, sorted by name. The `values/` templates are authoritative and can replace values from the chart `values.yaml` or the previous `values/` templates. 5. The dependencies conditions and tags are evaluated over the updated values, and disabled dependencies are removed. 6. The same process is performed on enabled dependencies. Once values are recursively updated, and once import values are treated on the enabled dependencies, those values are used for templates rendering. Signed-off-by: Aurélien Lambert <aure@olli-ai.com>
aure-olli
added a commit
to aure-olli/helm
that referenced
this pull request
Sep 3, 2020
This is the 3rd and last PR splitted from helm#6876, and should be merged after helm#8677 and helm#8679 There have been many requests for a way to use templates for `values.yaml` (helm#2492, helm#2133, ...). The main reason being that it's currently impossible to derive the values of the subcharts from templates. However, having templates in `values.yaml` make them unparsable and create a "chicken or the egg" problem when rendering them. This PR offers an intuitive solution without such a problem: an optional `values/` directory which templates are rendered using `values.yaml`, and then merge them with it. Those `values/` templates are only required for specific cases, work the same way as `templates/` templates, and `values.yaml` will remain the main parsable source of value. Values evaluation is now made in 2 step. First the `values.yaml` file is read. Then the `values/***.yaml` files are rendered using those values (`values/_***.tpl` files ara still allowed as helpers), and are used to update the values. For each chart and subchart, the following steps are performed: 1. If the chart has a parent, parent sub-values and global values are retrieved. 2. The chart values are coalesced with the retrieved value, the chart values are not authoritative. 3. The chart `values/` templates are rendered, the same way `templates/` templates are. The values are not updated during this step. 4. The values are updated with the rendered `values/` templates, sorted by name. The `values/` templates are authoritative and can replace values from the chart `values.yaml` or the previous `values/` templates. 5. The dependencies conditions and tags are evaluated over the updated values, and disabled dependencies are removed. 6. The same process is performed on enabled dependencies. Once values are recursively updated, and once import values are treated on the enabled dependencies, those values are used for templates rendering. Signed-off-by: Aurélien Lambert <aure@olli-ai.com>
bacongobbler
requested changes
Sep 4, 2020
There was a problem hiding this comment.
See discussion in #6876. This proposal requires a HIP before a proper review can be conducted.
|
ping. |
|
@bacongobbler judging by the reactions on the comment #6876 (comment) I don't think anybody is going to write a HIP. "death by bureaucracy" is really what happened here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the 2nd PR splitted from #6876, and should be merged after #8677
What this PR does / why we need it:
Prior to this PR, values from disabled dependencies would still be merged and imported to the parent values. This didn't have much consequences, but is incompatible with an implemention of values templating.
This PR makes the following changes:
pkg/chartutil/coalesce.goandpkg/chartutil/dependencies.goare no longer recursive, recursivity is handled bypkg/engine/engine.gochartutils.ToRenderValuesdoes not merge chart values anymore, this is done bypkg/engine/engine.gopkg/engine/engine.gonow uses a recursiveEngine.updateRenderValuesfunction that:- parses and merge
values.yaml- validates the values along the schema
- evaluates if sub-charts are enabled
- recursively treat the enabled subcharts
- import the values of the enabled subcharts
pkg/actionshave been addapted to the new way of merging valuesSigned-off-by: Aurélien Lambert aure@olli-ai.com
Special notes for your reviewer:
We should wait for the next PRs to merge this one, I may include few additional changes in the future (but it should globally stay the same)
If applicable: