fix(values): erase parent-defined subchart defaults when user sets null#31976
fix(values): erase parent-defined subchart defaults when user sets null#31976
Conversation
When a parent chart defines a value for a subchart in its own values.yaml, and the user supplies null for that key via --values, the null should erase the parent-defined default per documented Helm behavior. Previously, childChartMergeTrue correctly preserved null values through the parent coalesce pass so they could be erased during subchart-level coalescing (when the subchart itself has a default for that key). However, when the key only exists in the parent chart's values.yaml and not in the subchart's own values.yaml, no cleanup occurred — the nil persisted in the final result. Fix: after coalescing each subchart dependency, call eraseNullsWithParentDefaults to remove any remaining nil values where the parent chart defined a non-nil default. This covers the gap without disturbing keys already cleaned up at subchart level. Fixes helm#31919 Signed-off-by: Abhay Chaurasiya <abhaychaurasiya19@gmail.com> Signed-off-by: abhay1999 <abhaychaurasiya19@gmail.com>
edfc702 to
11b6255
Compare
There was a problem hiding this comment.
Pull request overview
Fixes Helm values coalescing behavior so that a user-supplied null correctly removes parent-chart-defined subchart defaults (when the subchart itself has no default for that key), addressing #31919.
Changes:
- Add a post-coalesce cleanup pass for each dependency to remove nil keys that correspond to non-nil parent-chart defaults.
- Introduce
eraseNullsWithParentDefaultshelper to perform targeted nil key deletion (including nested tables). - Add regression tests covering both null erasure and preserving user non-nil values when parent default is
null.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pkg/chart/common/util/coalesce.go |
Adds dependency-level nil cleanup (eraseNullsWithParentDefaults) after subchart coalescing in CoalesceValues mode. |
pkg/chart/common/util/coalesce_test.go |
Adds regression tests for null erasure of parent-defined subchart keys and preservation of user values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sub, ok := v["mysubchart"].(map[string]any) | ||
| is.True(ok, "mysubchart is not a map") | ||
|
|
||
| // "other" should still be present (subchart default) | ||
| is.Equal("subchart_default", sub["other"]) | ||
|
|
There was a problem hiding this comment.
These tests use assert.True(ok, ...) after a type assertion and then immediately index into the map (sub["other"], sub["key"]). If the assertion fails, assert does not stop the test, so this will panic and obscure the actual failure. Prefer a fatal check (e.g., require.True / require.IsType, or if !ok { t.Fatalf(...); return }) before using sub.
There was a problem hiding this comment.
Good catch. Fixed in the follow-up commit — switched to require.True / require.NoError so the test fails fast instead of panicking on the map indexing.
| sub, ok := v["mysubchart"].(map[string]any) | ||
| is.True(ok, "mysubchart is not a map") | ||
|
|
||
| is.Equal("user_value", sub["key"], "user value should not be overwritten by parent null default") | ||
| } |
There was a problem hiding this comment.
Same pattern here: assert.True(ok, ...) won’t stop the test, and sub["key"] will panic if the type assertion fails. Use a fatal guard (require or explicit t.Fatalf + return) before indexing into sub.
There was a problem hiding this comment.
Same fix applied here — require.True now guards the type assertion before sub["key"] is accessed.
Use require.True/require.NoError instead of assert.True/assert.NoError for type assertion guards before map indexing, so the test stops immediately on failure rather than panicking. Signed-off-by: Abhay Chaurasiya <abhaychaurasiya19@gmail.com> Signed-off-by: abhay1999 <abhaychaurasiya19@gmail.com>
|
The The two earlier CI runs on this branch both passed with the same test suite, suggesting this is a pre-existing intermittent failure in |
What this fixes
Fixes #31919
When a parent chart defines a key for a subchart in its own
values.yaml, and the user suppliesnullfor that key via--values, the null should erase the parent-defined default — this is documented Helm behavior.Broken scenario:
Before this fix,
apiKeyremained asnullin the rendered output instead of being deleted.Root cause
childChartMergeTruecorrectly forcesmerge=truefor subchart tables so that user-supplied null values are preserved through the parent-chart coalescing pass, allowing them to be erased later at subchart level (where the subchart's own defaults are processed).However, when a key is defined only in the parent chart's
values.yaml(not in the subchart's ownvalues.yaml), no subchart-level cleanup ever runs — the nil persists in the final result.Fix
After coalescing each subchart dependency in
coalesceDeps, call the neweraseNullsWithParentDefaultshelper which removes nil-valued keys where the parent chart had a non-nil default. This closes the gap for parent-only keys without disturbing keys that were already cleaned up at subchart level.Tests
TestCoalesceValuesNullErasureInSubchart: reproduces Regression in null merging betweenparent.valuesand--values#31919 — verifies user null erases parent-defined subchart default when subchart has no own default for the key.TestCoalesceValuesNullErasurePreservesUserValues: verifies that a parent chart's null default does NOT erase a user's non-nil value for the same key.go test ./pkg/...— 0 failures).