From 6737e1638b67dc41e08540969cba36c5fe2e858b Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Sun, 21 Sep 2025 15:03:22 +0800 Subject: [PATCH 1/4] 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 726c11ac2d9376e99722bd44c2cfbea5ae93eed0 Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Thu, 25 Sep 2025 11:23:19 +0800 Subject: [PATCH 2/4] 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 55606f057d4a654105a806e114e405f570bdabc8 Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Thu, 25 Sep 2025 11:26:21 +0800 Subject: [PATCH 3/4] 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 3468a10cbf0ac2516fe4090d2f4e963214340e11 Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Thu, 25 Sep 2025 16:41:20 +0800 Subject: [PATCH 4/4] 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{}{