Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix(values): erase parent-defined subchart defaults when user sets null
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 #31919

Signed-off-by: Abhay Chaurasiya <abhaychaurasiya19@gmail.com>
Signed-off-by: abhay1999 <abhaychaurasiya19@gmail.com>
  • Loading branch information
abhay1999 committed Mar 28, 2026
commit 11b6255e3e3d6cb2809ff886b78ca7b17506f02b
34 changes: 34 additions & 0 deletions pkg/chart/common/util/coalesce.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,19 @@ func coalesceDeps(printf printFn, chrt chart.Charter, dest map[string]any, prefi
if err != nil {
return dest, err
}
// When coalescing, erase any remaining nil values in the subchart result
// that were user-supplied null overrides of parent-chart defaults.
// These are not erased at subchart level because the subchart has no
// default for the key; without this pass they would persist as nil.
if !merge {
if subMap, ok := dest[sub.Name()].(map[string]any); ok {
if parentVals, ok := ch.Values()[sub.Name()]; ok {
if parentValsMap, ok := parentVals.(map[string]any); ok {
eraseNullsWithParentDefaults(subMap, parentValsMap)
}
}
}
}
}
}
return dest, nil
Expand Down Expand Up @@ -189,6 +202,25 @@ func coalesceGlobals(printf printFn, dest, src map[string]any, prefix string, _
dest[common.GlobalKey] = dg
}

// eraseNullsWithParentDefaults removes nil-valued keys from dst where the
// corresponding parent chart default was non-nil. This handles the case where
// a user sets a subchart key to null to erase a parent-defined default, but
// the subchart itself has no default for that key (so subchart-level null
// erasure never runs).
func eraseNullsWithParentDefaults(dst, parentDefs map[string]any) {
for key, dv := range dst {
pdv, inParent := parentDefs[key]
if !inParent {
continue
}
if dv == nil && pdv != nil {
delete(dst, key)
} else if istable(dv) && istable(pdv) {
eraseNullsWithParentDefaults(dv.(map[string]any), pdv.(map[string]any))
}
}
}

func copyMap(src map[string]any) map[string]any {
m := make(map[string]any, len(src))
maps.Copy(m, src)
Expand Down Expand Up @@ -249,6 +281,8 @@ func coalesceValues(printf printFn, c chart.Charter, v map[string]any, prefix st
}
} else {
// If the key is a child chart, coalesce tables with Merge set to true
// to preserve user-supplied null values so they can be erased at the
// subchart level (where subchart defaults are processed).
merge := childChartMergeTrue(c, key, merge)

// Because v has higher precedence than nv, dest values override src
Expand Down
79 changes: 79 additions & 0 deletions pkg/chart/common/util/coalesce_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -732,6 +732,85 @@ func TestConcatPrefix(t *testing.T) {
assert.Equal(t, "a.b", concatPrefix("a", "b"))
}

// TestCoalesceValuesNullErasureInSubchart tests that a user-supplied null value
// erases a key defined in the parent chart's values.yaml for a subchart,
// even when that key has no default in the subchart's own values.yaml.
// Regression test for https://github.com/helm/helm/issues/31919
func TestCoalesceValuesNullErasureInSubchart(t *testing.T) {
is := assert.New(t)

// Parent chart defines "key" for subchart in its own values.yaml.
// The subchart itself does NOT define "key" in its values.yaml.
subchart := &chart.Chart{
Metadata: &chart.Metadata{Name: "mysubchart"},
Values: map[string]any{
"other": "subchart_default",
},
}
parent := withDeps(&chart.Chart{
Metadata: &chart.Metadata{Name: "parent"},
Values: map[string]any{
"mysubchart": map[string]any{
"key": "parent_defined_value",
},
},
}, subchart)

// User passes null for mysubchart.key to erase the parent-defined default.
vals := map[string]any{
"mysubchart": map[string]any{
"key": nil,
},
}

v, err := CoalesceValues(parent, vals)
is.NoError(err)

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"])

Comment on lines +768 to +773

Copilot AI Mar 28, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

// "key" should be removed — user explicitly set it to null to erase the parent default
_, present := sub["key"]
is.False(present, "Expected mysubchart.key to be erased by user null, but it is still present")
}

// TestCoalesceValuesNullErasurePreservesUserValues ensures that when a parent chart
// defines a null for a subchart key, the user's non-nil value is still preserved.
func TestCoalesceValuesNullErasurePreservesUserValues(t *testing.T) {
is := assert.New(t)

subchart := &chart.Chart{
Metadata: &chart.Metadata{Name: "mysubchart"},
Values: map[string]any{},
}
parent := withDeps(&chart.Chart{
Metadata: &chart.Metadata{Name: "parent"},
Values: map[string]any{
"mysubchart": map[string]any{
"key": nil, // parent chart defaults this to null
},
},
}, subchart)

// User provides a real value — should not be erased by the parent's null default
vals := map[string]any{
"mysubchart": map[string]any{
"key": "user_value",
},
}

v, err := CoalesceValues(parent, vals)
is.NoError(err)

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")
}
Comment on lines +805 to +809

Copilot AI Mar 28, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same fix applied here — require.True now guards the type assertion before sub["key"] is accessed.


// TestCoalesceValuesEmptyMapWithNils tests the full CoalesceValues scenario
// from issue #31643 where chart has data: {} and user provides data: {foo: bar, baz: ~}
func TestCoalesceValuesEmptyMapWithNils(t *testing.T) {
Expand Down