From 58aaf2f263793e1935c9fc93fac076824404942e Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Sun, 21 Sep 2025 15:03:22 +0800 Subject: [PATCH 1/8] Add test for coalescing values with empty map overrides Signed-off-by: Siew Kam Onn --- pkg/chart/common/util/coalesce.go | 3 +++ pkg/chart/common/util/coalesce_test.go | 35 ++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/pkg/chart/common/util/coalesce.go b/pkg/chart/common/util/coalesce.go index 5bfa1c608..481f90f76 100644 --- a/pkg/chart/common/util/coalesce.go +++ b/pkg/chart/common/util/coalesce.go @@ -303,6 +303,9 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref if dst == nil { return src } + if len(dst) == 0 && prefix != "" && !merge { + return dst + } for key, val := range dst { if val == nil { src[key] = nil diff --git a/pkg/chart/common/util/coalesce_test.go b/pkg/chart/common/util/coalesce_test.go index 871bfa8da..5109167cc 100644 --- a/pkg/chart/common/util/coalesce_test.go +++ b/pkg/chart/common/util/coalesce_test.go @@ -241,6 +241,41 @@ func TestCoalesceValues(t *testing.T) { is.Equal(valsCopy, vals) } +func TestCoalesceValuesEmptyMapOverride(t *testing.T) { + c := &chart.Chart{ + Metadata: &chart.Metadata{Name: "emptymap"}, + Values: map[string]interface{}{ + "config": map[string]interface{}{"foo": "bar"}, + "toDelete": map[string]interface{}{"baz": "qux"}, + }, + } + + overrides := map[string]interface{}{ + "config": map[string]interface{}{}, + "toDelete": nil, + } + + result, err := CoalesceValues(c, overrides) + if err != nil { + t.Fatal(err) + } + + config, ok := result["config"].(map[string]interface{}) + if !ok { + t.Fatalf("expected config to remain a map, got %T", result["config"]) + } + if len(config) != 0 { + t.Fatalf("expected config map to be empty, got %v", config) + } + if _, exists := config["foo"]; exists { + t.Fatalf("expected config override to drop default key, still found foo in %v", config) + } + + if _, exists := result["toDelete"]; exists { + t.Fatalf("expected toDelete key to be removed when set to nil override") + } +} + func ttpl(tpl string, v map[string]interface{}) (string, error) { var b bytes.Buffer tt := template.Must(template.New("t").Parse(tpl)) From 7a54bdca4491edcce18cba7e348d0121e7fb65cd Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Thu, 25 Sep 2025 11:23:19 +0800 Subject: [PATCH 2/8] Add test for CoalesceTables with empty destination table Signed-off-by: Siew Kam Onn --- pkg/chart/common/util/coalesce_test.go | 30 ++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/pkg/chart/common/util/coalesce_test.go b/pkg/chart/common/util/coalesce_test.go index 5109167cc..c1ac66922 100644 --- a/pkg/chart/common/util/coalesce_test.go +++ b/pkg/chart/common/util/coalesce_test.go @@ -436,6 +436,36 @@ func TestMergeValues(t *testing.T) { } func TestCoalesceTables(t *testing.T) { + t.Run("empty destination table overrides defaults", func(t *testing.T) { + t.Helper() + + dst := map[string]interface{}{ + "config": map[string]interface{}{}, + } + src := map[string]interface{}{ + "config": map[string]interface{}{ + "enabled": true, + "port": 8080, + }, + } + + CoalesceTables(dst, src) + + config, ok := dst["config"].(map[string]interface{}) + if !ok { + t.Fatalf("config should remain a map, got %T", dst["config"]) + } + if len(config) != 0 { + t.Fatalf("expected empty config map, got %v", config) + } + if _, exists := config["enabled"]; exists { + t.Fatal("expected default \"enabled\" key to be absent") + } + if _, exists := config["port"]; exists { + t.Fatal("expected default \"port\" key to be absent") + } + }) + dst := map[string]interface{}{ "name": "Ishmael", "address": map[string]interface{}{ From cf8ebe8e5c9dd47d8eda6cf12f08b925ebc7e854 Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Thu, 25 Sep 2025 11:26:21 +0800 Subject: [PATCH 3/8] Clarify behavior for empty destination tables in coalesceTablesFullKey function Signed-off-by: Siew Kam Onn --- pkg/chart/common/util/coalesce.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/chart/common/util/coalesce.go b/pkg/chart/common/util/coalesce.go index 481f90f76..7326ee685 100644 --- a/pkg/chart/common/util/coalesce.go +++ b/pkg/chart/common/util/coalesce.go @@ -303,6 +303,11 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref if dst == nil { return src } + // If dst is empty and we're operating on a sub-key (prefix != ""), + // bail out when not merging so that child charts don't inherit defaults + // for an explicitly empty table in the parent. We only do this for + // non-top-level coalescing (prefix != "") because top-level coalescing + // still needs to pull chart defaults across when dst is empty. if len(dst) == 0 && prefix != "" && !merge { return dst } From 2f2a54a0aa08a10894ba0df367e45e7f712a9531 Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Thu, 25 Sep 2025 16:41:20 +0800 Subject: [PATCH 4/8] Refactor tests in coalesce_test.go to use require and assert for better error handling and readability Signed-off-by: Siew Kam Onn --- pkg/chart/common/util/coalesce_test.go | 37 +++++++------------------- 1 file changed, 10 insertions(+), 27 deletions(-) diff --git a/pkg/chart/common/util/coalesce_test.go b/pkg/chart/common/util/coalesce_test.go index c1ac66922..f127b5f0f 100644 --- a/pkg/chart/common/util/coalesce_test.go +++ b/pkg/chart/common/util/coalesce_test.go @@ -25,6 +25,7 @@ import ( "text/template" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "helm.sh/helm/v4/pkg/chart/common" chart "helm.sh/helm/v4/pkg/chart/v2" @@ -256,24 +257,14 @@ func TestCoalesceValuesEmptyMapOverride(t *testing.T) { } result, err := CoalesceValues(c, overrides) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) config, ok := result["config"].(map[string]interface{}) - if !ok { - t.Fatalf("expected config to remain a map, got %T", result["config"]) - } - if len(config) != 0 { - t.Fatalf("expected config map to be empty, got %v", config) - } - if _, exists := config["foo"]; exists { - t.Fatalf("expected config override to drop default key, still found foo in %v", config) - } + require.Truef(t, ok, "expected config to remain a map, got %T", result["config"]) + assert.Empty(t, config, "expected config map to be empty") + assert.NotContains(t, config, "foo", "expected config override to drop default key") - if _, exists := result["toDelete"]; exists { - t.Fatalf("expected toDelete key to be removed when set to nil override") - } + assert.NotContains(t, result, "toDelete", "expected toDelete key to be removed when set to nil override") } func ttpl(tpl string, v map[string]interface{}) (string, error) { @@ -452,18 +443,10 @@ func TestCoalesceTables(t *testing.T) { CoalesceTables(dst, src) config, ok := dst["config"].(map[string]interface{}) - if !ok { - t.Fatalf("config should remain a map, got %T", dst["config"]) - } - if len(config) != 0 { - t.Fatalf("expected empty config map, got %v", config) - } - if _, exists := config["enabled"]; exists { - t.Fatal("expected default \"enabled\" key to be absent") - } - if _, exists := config["port"]; exists { - t.Fatal("expected default \"port\" key to be absent") - } + require.Truef(t, ok, "config should remain a map, got %T", dst["config"]) + assert.Empty(t, config, "expected empty config map") + assert.NotContains(t, config, "enabled", "expected default \"enabled\" key to be absent") + assert.NotContains(t, config, "port", "expected default \"port\" key to be absent") }) dst := map[string]interface{}{ From b09950663210ddd62af54b6e92cf85b1d039c2a9 Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Fri, 10 Oct 2025 18:41:52 +0800 Subject: [PATCH 5/8] Refactor coalesce functions to handle empty maps Reordered coalesceValues and coalesceTablesFullKey to convert explicit empty overrides into empty maps before merging. This prevents default map entries from resurfacing, even for nested tables. Additionally, the regression suite is expanded to cover nested empty overrides, ensuring empty maps are preserved across different override shapes. Signed-off-by: Siew Kam Onn --- pkg/chart/common/util/coalesce.go | 74 +++++++++++++++++++++----- pkg/chart/common/util/coalesce_test.go | 58 ++++++++++++++++++++ pkg/strvals/parser.go | 13 ++++- pkg/strvals/parser_test.go | 5 ++ 4 files changed, 135 insertions(+), 15 deletions(-) diff --git a/pkg/chart/common/util/coalesce.go b/pkg/chart/common/util/coalesce.go index 7326ee685..e5f7664c6 100644 --- a/pkg/chart/common/util/coalesce.go +++ b/pkg/chart/common/util/coalesce.go @@ -239,7 +239,23 @@ func coalesceValues(printf printFn, c chart.Charter, v map[string]interface{}, p // This allows Helm's various sources of values (value files or --set) to // remove incompatible keys from any previous chart, file, or set values. delete(v, key) - } else if dest, ok := value.(map[string]interface{}); ok { + continue + } + + if isExplicitEmptyOverride(value) { + // Interpret empty slices from values sources (for example, --set key={}) + // as an explicit request for an empty map so chart defaults are removed. + dest := map[string]interface{}{} + v[key] = dest + + if src, ok := val.(map[string]interface{}); ok { + merge := childChartMergeTrue(c, key, merge) + coalesceTablesFullKey(printf, dest, src, concatPrefix(subPrefix, key), merge) + } + continue + } + + if dest, ok := value.(map[string]interface{}); ok { // if v[key] is a table, merge nv's val table into v[key]. src, ok := val.(map[string]interface{}) if !ok { @@ -248,14 +264,16 @@ func coalesceValues(printf printFn, c chart.Charter, v map[string]interface{}, p if val != nil { printf("warning: skipped value for %s.%s: Not a table.", subPrefix, key) } - } else { - // If the key is a child chart, coalesce tables with Merge set to true - merge := childChartMergeTrue(c, key, merge) - - // Because v has higher precedence than nv, dest values override src - // values. - coalesceTablesFullKey(printf, dest, src, concatPrefix(subPrefix, key), merge) + continue } + + // If the key is a child chart, coalesce tables with Merge set to true + merge := childChartMergeTrue(c, key, merge) + + // Because v has higher precedence than nv, dest values override src + // values. + coalesceTablesFullKey(printf, dest, src, concatPrefix(subPrefix, key), merge) + continue } } else { // If the key is not in v, copy it from nv. @@ -320,17 +338,36 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref // values. for key, val := range src { fullkey := concatPrefix(prefix, key) - if dv, ok := dst[key]; ok && !merge && dv == nil { - delete(dst, key) - } else if !ok { + dv, ok := dst[key] + if !ok { dst[key] = val - } else if istable(val) { + continue + } + + if !merge && dv == nil { + delete(dst, key) + continue + } + + if isExplicitEmptyOverride(dv) { + empty := map[string]interface{}{} + dst[key] = empty + if srcMap, ok := val.(map[string]interface{}); ok { + coalesceTablesFullKey(printf, empty, srcMap, fullkey, merge) + } + continue + } + + if istable(val) { if istable(dv) { coalesceTablesFullKey(printf, dv.(map[string]interface{}), val.(map[string]interface{}), fullkey, merge) } else { printf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val) } - } else if istable(dv) && val != nil { + continue + } + + if istable(dv) && val != nil { printf("warning: destination for %s is a table. Ignoring non-table value (%v)", fullkey, val) } } @@ -342,3 +379,14 @@ func istable(v interface{}) bool { _, ok := v.(map[string]interface{}) return ok } + +func isExplicitEmptyOverride(v interface{}) bool { + switch vv := v.(type) { + case []interface{}: + return len(vv) == 0 || (len(vv) == 1 && vv[0] == "") + case []string: + return len(vv) == 0 || (len(vv) == 1 && vv[0] == "") + default: + return false + } +} diff --git a/pkg/chart/common/util/coalesce_test.go b/pkg/chart/common/util/coalesce_test.go index f127b5f0f..3d1b110bc 100644 --- a/pkg/chart/common/util/coalesce_test.go +++ b/pkg/chart/common/util/coalesce_test.go @@ -267,6 +267,64 @@ func TestCoalesceValuesEmptyMapOverride(t *testing.T) { assert.NotContains(t, result, "toDelete", "expected toDelete key to be removed when set to nil override") } +func TestCoalesceValuesEmptySliceOverrideForMap(t *testing.T) { + newChart := func() *chart.Chart { + return &chart.Chart{ + Metadata: &chart.Metadata{Name: "emptymap"}, + Values: map[string]interface{}{ + "config": map[string]interface{}{ + "foo": "bar", + "nested": map[string]interface{}{"baz": "qux"}, + }, + }, + } + } + + t.Run("empty interface slice", func(t *testing.T) { + overrides := map[string]interface{}{ + "config": []interface{}{}, + } + + result, err := CoalesceValues(newChart(), overrides) + require.NoError(t, err) + + config, ok := result["config"].(map[string]interface{}) + require.Truef(t, ok, "expected config to remain a map, got %T", result["config"]) + assert.Empty(t, config, "expected config map to be empty") + }) + + t.Run("legacy single empty element slice", func(t *testing.T) { + overrides := map[string]interface{}{ + "config": []interface{}{""}, + } + + result, err := CoalesceValues(newChart(), overrides) + require.NoError(t, err) + + config, ok := result["config"].(map[string]interface{}) + require.Truef(t, ok, "expected config to remain a map, got %T", result["config"]) + assert.Empty(t, config, "expected config map to be empty") + }) + + t.Run("nested empty slice", func(t *testing.T) { + overrides := map[string]interface{}{ + "config": map[string]interface{}{ + "nested": []interface{}{}, + }, + } + + result, err := CoalesceValues(newChart(), overrides) + require.NoError(t, err) + + config, ok := result["config"].(map[string]interface{}) + require.Truef(t, ok, "expected config to remain a map, got %T", result["config"]) + + nested, ok := config["nested"].(map[string]interface{}) + require.Truef(t, ok, "expected nested override to remain a map, got %T", config["nested"]) + assert.Empty(t, nested, "expected nested map to be empty") + }) +} + func ttpl(tpl string, v map[string]interface{}) (string, error) { var b bytes.Buffer tt := template.Must(template.New("t").Parse(tpl)) diff --git a/pkg/strvals/parser.go b/pkg/strvals/parser.go index c65e98c84..a889ab9b4 100644 --- a/pkg/strvals/parser.go +++ b/pkg/strvals/parser.go @@ -485,8 +485,17 @@ func (t *parser) valList() ([]interface{}, error) { return list, err case last == '}': // If this is followed by ',', consume it. - if r, _, e := t.sc.ReadRune(); e == nil && r != ',' { - t.sc.UnreadRune() + r, _, e := t.sc.ReadRune() + if e == nil { + if r != ',' { + t.sc.UnreadRune() + } + } else if e != io.EOF { + return list, e + } + if len(rs) == 0 && len(list) == 0 { + // Interpret "{}" as an explicit empty map, not an empty list. + return []interface{}{map[string]interface{}{}}, nil } v, e := t.reader(rs) list = append(list, v) diff --git a/pkg/strvals/parser_test.go b/pkg/strvals/parser_test.go index 73403fc52..9e83bb4ac 100644 --- a/pkg/strvals/parser_test.go +++ b/pkg/strvals/parser_test.go @@ -265,6 +265,11 @@ func TestParseSet(t *testing.T) { map[string]interface{}{"name1": []string{"value1", "value2"}}, false, }, + { + "emptylist={}", + map[string]interface{}{"emptylist": []interface{}{}}, + false, + }, { "name1={value1,value2},name2={value1,value2}", map[string]interface{}{ From 47956d503aeb7378ac85d62d088493fbdd080d34 Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Fri, 10 Oct 2025 18:58:11 +0800 Subject: [PATCH 6/8] Update TestParseSet to handle nested empty lists in input Signed-off-by: Siew Kam Onn --- pkg/strvals/parser_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/strvals/parser_test.go b/pkg/strvals/parser_test.go index 9e83bb4ac..fa8a28b9e 100644 --- a/pkg/strvals/parser_test.go +++ b/pkg/strvals/parser_test.go @@ -267,7 +267,7 @@ func TestParseSet(t *testing.T) { }, { "emptylist={}", - map[string]interface{}{"emptylist": []interface{}{}}, + map[string]interface{}{"emptylist": []interface{}{map[string]interface{}{}}}, false, }, { From dfa49faa7923b73cac259c08636b7e3f23d0276e Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Fri, 10 Oct 2025 21:21:32 +0800 Subject: [PATCH 7/8] Fix isExplicitEmptyOverride for empty maps handling Enhanced the isExplicitEmptyOverride function to correctly identify empty maps and placeholders created by --set key={}, preventing unintentional reintroduction of chart defaults. Added regression tests to ensure proper treatment of empty map placeholders and validated that CLI values maintain empty maps after coalescing. Signed-off-by: Siew Kam Onn --- pkg/chart/common/util/coalesce.go | 27 +++++++++++- pkg/chart/common/util/coalesce_test.go | 13 ++++++ pkg/cli/values/options_test.go | 57 ++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 2 deletions(-) diff --git a/pkg/chart/common/util/coalesce.go b/pkg/chart/common/util/coalesce.go index e5f7664c6..c2b0ecd97 100644 --- a/pkg/chart/common/util/coalesce.go +++ b/pkg/chart/common/util/coalesce.go @@ -382,10 +382,33 @@ func istable(v interface{}) bool { func isExplicitEmptyOverride(v interface{}) bool { switch vv := v.(type) { + case map[string]interface{}: + return len(vv) == 0 + case map[interface{}]interface{}: + return len(vv) == 0 case []interface{}: - return len(vv) == 0 || (len(vv) == 1 && vv[0] == "") + switch len(vv) { + case 0: + return true + case 1: + switch item := vv[0].(type) { + case string: + return item == "" + default: + return isExplicitEmptyOverride(item) + } + default: + return false + } case []string: - return len(vv) == 0 || (len(vv) == 1 && vv[0] == "") + switch len(vv) { + case 0: + return true + case 1: + return vv[0] == "" + default: + return false + } default: return false } diff --git a/pkg/chart/common/util/coalesce_test.go b/pkg/chart/common/util/coalesce_test.go index 3d1b110bc..a56c3f37b 100644 --- a/pkg/chart/common/util/coalesce_test.go +++ b/pkg/chart/common/util/coalesce_test.go @@ -306,6 +306,19 @@ func TestCoalesceValuesEmptySliceOverrideForMap(t *testing.T) { assert.Empty(t, config, "expected config map to be empty") }) + t.Run("explicit empty map placeholder", func(t *testing.T) { + overrides := map[string]interface{}{ + "config": []interface{}{map[string]interface{}{}}, + } + + result, err := CoalesceValues(newChart(), overrides) + require.NoError(t, err) + + config, ok := result["config"].(map[string]interface{}) + require.Truef(t, ok, "expected config to remain a map, got %T", result["config"]) + assert.Empty(t, config, "expected config map to be empty") + }) + t.Run("nested empty slice", func(t *testing.T) { overrides := map[string]interface{}{ "config": map[string]interface{}{ diff --git a/pkg/cli/values/options_test.go b/pkg/cli/values/options_test.go index fe1afc5d2..eee363237 100644 --- a/pkg/cli/values/options_test.go +++ b/pkg/cli/values/options_test.go @@ -26,6 +26,11 @@ import ( "strings" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "helm.sh/helm/v4/pkg/chart/common/util" + chart "helm.sh/helm/v4/pkg/chart/v2" "helm.sh/helm/v4/pkg/getter" ) @@ -248,6 +253,58 @@ func TestReadFile(t *testing.T) { } } +func TestMergeValuesExplicitEmptyOverrides(t *testing.T) { + newChart := func() *chart.Chart { + return &chart.Chart{ + Metadata: &chart.Metadata{Name: "emptymap"}, + Values: map[string]interface{}{ + "key": map[string]interface{}{"default": true}, + }, + } + } + + tests := []struct { + name string + setup func(t *testing.T) *Options + }{ + { + name: "empty map from values file", + setup: func(t *testing.T) *Options { + dir := t.TempDir() + file := filepath.Join(dir, "values.yaml") + require.NoError(t, os.WriteFile(file, []byte("key: {}\n"), 0o600)) + return &Options{ValueFiles: []string{file}} + }, + }, + { + name: "empty map from set flag", + setup: func(_ *testing.T) *Options { + return &Options{Values: []string{"key={}"}} + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + opts := tt.setup(t) + + overrides, err := opts.MergeValues(getter.Providers{}) + require.NoError(t, err) + + result, err := util.CoalesceValues(newChart(), overrides) + require.NoError(t, err) + + raw, ok := result["key"] + require.True(t, ok, "expected key to be present") + + actual, ok := raw.(map[string]interface{}) + require.True(t, ok, "expected key to be a map but got %T", raw) + assert.Empty(t, actual) + }) + } +} + // TestReadFileErrorMessages tests specific error scenarios and their messages func TestReadFileErrorMessages(t *testing.T) { tests := []struct { From 69d007f70e62bdd7fa116a60abc102426acbdd04 Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Sat, 11 Oct 2025 17:32:26 +0800 Subject: [PATCH 8/8] Add helper function calls in TestMergeValuesExplicitEmptyOverrides for clarity Signed-off-by: Siew Kam Onn --- pkg/cli/values/options_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/cli/values/options_test.go b/pkg/cli/values/options_test.go index eee363237..e06e8c8f6 100644 --- a/pkg/cli/values/options_test.go +++ b/pkg/cli/values/options_test.go @@ -270,6 +270,7 @@ func TestMergeValuesExplicitEmptyOverrides(t *testing.T) { { name: "empty map from values file", setup: func(t *testing.T) *Options { + t.Helper() dir := t.TempDir() file := filepath.Join(dir, "values.yaml") require.NoError(t, os.WriteFile(file, []byte("key: {}\n"), 0o600)) @@ -278,7 +279,8 @@ func TestMergeValuesExplicitEmptyOverrides(t *testing.T) { }, { name: "empty map from set flag", - setup: func(_ *testing.T) *Options { + setup: func(t *testing.T) *Options { + t.Helper() return &Options{Values: []string{"key={}"}} }, },