From de0d39795748c3ac7e13d33a9988b69d170e7323 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Wed, 21 Feb 2024 20:51:52 -0500 Subject: [PATCH 1/9] fix: added extra conditional to warning Signed-off-by: Jesse Simpson --- pkg/chart/v2/util/coalesce.go | 2 +- pkg/chart/v2/util/coalesce_test.go | 32 ++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/pkg/chart/v2/util/coalesce.go b/pkg/chart/v2/util/coalesce.go index a3e0f5ae8..9699faf75 100644 --- a/pkg/chart/v2/util/coalesce.go +++ b/pkg/chart/v2/util/coalesce.go @@ -300,7 +300,7 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref } else { printf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val) } - } else if istable(dv) && val != nil { + } else if istable(dv) && val != nil && len(dv.(map[string]interface{})) > 0 { printf("warning: destination for %s is a table. Ignoring non-table value (%v)", fullkey, val) } } diff --git a/pkg/chart/v2/util/coalesce_test.go b/pkg/chart/v2/util/coalesce_test.go index e2c45a435..5ba66ce6d 100644 --- a/pkg/chart/v2/util/coalesce_test.go +++ b/pkg/chart/v2/util/coalesce_test.go @@ -717,6 +717,38 @@ func TestCoalesceValuesWarnings(t *testing.T) { } +func TestCoalesceValuesWarningsWithEmptyDefaultMaps(t *testing.T) { + + c := withDeps(&chart.Chart{ + Metadata: &chart.Metadata{Name: "level1"}, + Values: map[string]interface{}{ + "something": map[string]interface{}{ + "somethingElse": []interface{}{}, + }, + }, + }) + + vals := map[string]interface{}{ + "something": map[string]interface{}{ + "somethingElse": map[string]interface{}{}, + }, + } + + warnings := make([]string, 0) + printf := func(format string, v ...interface{}) { + t.Logf(format, v...) + warnings = append(warnings, fmt.Sprintf(format, v...)) + } + + _, err := coalesce(printf, c, vals, "", false) + if err != nil { + t.Fatal(err) + } + + t.Logf("vals: %v", vals) + assert.NotContains(t, warnings, "warning: destination for level1.something.somethingElse is a table. Ignoring non-table value ([])") +} + func TestConcatPrefix(t *testing.T) { assert.Equal(t, "b", concatPrefix("", "b")) assert.Equal(t, "a.b", concatPrefix("a", "b")) From 415e3ce9db70ba979dd156fae6934f0706df4a8e Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Thu, 22 Feb 2024 08:59:20 -0500 Subject: [PATCH 2/9] fix: added an empty string check to val Signed-off-by: Jesse Simpson --- pkg/chart/v2/util/coalesce.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/chart/v2/util/coalesce.go b/pkg/chart/v2/util/coalesce.go index 9699faf75..d5d163c5e 100644 --- a/pkg/chart/v2/util/coalesce.go +++ b/pkg/chart/v2/util/coalesce.go @@ -300,7 +300,7 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref } else { printf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val) } - } else if istable(dv) && val != nil && len(dv.(map[string]interface{})) > 0 { + } else if istable(dv) && val != nil && val != "" && len(dv.(map[string]interface{})) > 0 { printf("warning: destination for %s is a table. Ignoring non-table value (%v)", fullkey, val) } } From 34cfd617dce2b83e92f42af2ce88893f6a92a04b Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sat, 24 Feb 2024 11:38:54 -0500 Subject: [PATCH 3/9] refactor: broke boolean expression into functions that protect against cast panics Signed-off-by: Jesse Simpson --- pkg/chart/v2/util/coalesce.go | 2 +- pkg/chart/v2/util/values.go | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/pkg/chart/v2/util/coalesce.go b/pkg/chart/v2/util/coalesce.go index d5d163c5e..307603bae 100644 --- a/pkg/chart/v2/util/coalesce.go +++ b/pkg/chart/v2/util/coalesce.go @@ -300,7 +300,7 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref } else { printf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val) } - } else if istable(dv) && val != nil && val != "" && len(dv.(map[string]interface{})) > 0 { + } else if istable(dv) && val != nil && isNonEmptyString(val) && isNonEmptyTable(dv) { printf("warning: destination for %s is a table. Ignoring non-table value (%v)", fullkey, val) } } diff --git a/pkg/chart/v2/util/values.go b/pkg/chart/v2/util/values.go index 6850e8b9b..5dbc2a3a3 100644 --- a/pkg/chart/v2/util/values.go +++ b/pkg/chart/v2/util/values.go @@ -179,6 +179,24 @@ func istable(v interface{}) bool { return ok } +func isNonEmptyTable(val interface{}) bool { + table, ok := val.(map[string]interface{}) + if !ok { + return false + } + + return len(table) > 0 +} + +func isNonEmptyString(val interface{}) bool { + stringContent, ok := val.(string) + if !ok { + return false + } + + return stringContent != "" +} + // PathValue takes a path that traverses a YAML structure and returns the value at the end of that path. // The path starts at the root of the YAML structure and is comprised of YAML keys separated by periods. // Given the following YAML data the value at path "chapter.one.title" is "Loomings". From 5c225a1bb4363f8f5cac2af26a00f36fbd6909b4 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sat, 24 Feb 2024 11:40:44 -0500 Subject: [PATCH 4/9] test: adds empty string test Signed-off-by: Jesse Simpson --- pkg/chart/v2/util/coalesce_test.go | 34 ++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/pkg/chart/v2/util/coalesce_test.go b/pkg/chart/v2/util/coalesce_test.go index 5ba66ce6d..9559ca81d 100644 --- a/pkg/chart/v2/util/coalesce_test.go +++ b/pkg/chart/v2/util/coalesce_test.go @@ -749,6 +749,40 @@ func TestCoalesceValuesWarningsWithEmptyDefaultMaps(t *testing.T) { assert.NotContains(t, warnings, "warning: destination for level1.something.somethingElse is a table. Ignoring non-table value ([])") } +func TestCoalesceValuesWarningsWithEmptyStringDefault(t *testing.T) { + + c := withDeps(&chart.Chart{ + Metadata: &chart.Metadata{Name: "level1"}, + Values: map[string]interface{}{ + "auth": map[string]interface{}{ + "existingSecret": "", + }, + }, + }) + + vals := map[string]interface{}{ + "auth": map[string]interface{}{ + "existingSecret": map[string]interface{}{ + "name": "secretName", + }, + }, + } + + warnings := make([]string, 0) + printf := func(format string, v ...interface{}) { + t.Logf(format, v...) + warnings = append(warnings, fmt.Sprintf(format, v...)) + } + + _, err := coalesce(printf, c, vals, "", false) + if err != nil { + t.Fatal(err) + } + + t.Logf("vals: %v", vals) + assert.NotContains(t, warnings, "warning: destination for level1.auth.existingSecret is a table. Ignoring non-table value ()") +} + func TestConcatPrefix(t *testing.T) { assert.Equal(t, "b", concatPrefix("", "b")) assert.Equal(t, "a.b", concatPrefix("a", "b")) From 17f72572b0109cfbb813d8abccfb9c26ae3c018b Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Wed, 14 May 2025 15:36:23 -0400 Subject: [PATCH 5/9] fix: adjust use case for when val is boolean Signed-off-by: Jesse Simpson --- pkg/chart/v2/util/values.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/pkg/chart/v2/util/values.go b/pkg/chart/v2/util/values.go index 5dbc2a3a3..11347152f 100644 --- a/pkg/chart/v2/util/values.go +++ b/pkg/chart/v2/util/values.go @@ -189,12 +189,8 @@ func isNonEmptyTable(val interface{}) bool { } func isNonEmptyString(val interface{}) bool { - stringContent, ok := val.(string) - if !ok { - return false - } - - return stringContent != "" + valueString := fmt.Sprintf("%s", val) + return valueString != "" } // PathValue takes a path that traverses a YAML structure and returns the value at the end of that path. From a92794b4fff575c2649de66d67f98a37b3c60a46 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Wed, 14 May 2025 19:00:09 -0400 Subject: [PATCH 6/9] fix: adjust %s to %v Signed-off-by: Jesse Simpson --- pkg/chart/v2/util/values.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/chart/v2/util/values.go b/pkg/chart/v2/util/values.go index 11347152f..5549580ce 100644 --- a/pkg/chart/v2/util/values.go +++ b/pkg/chart/v2/util/values.go @@ -189,7 +189,7 @@ func isNonEmptyTable(val interface{}) bool { } func isNonEmptyString(val interface{}) bool { - valueString := fmt.Sprintf("%s", val) + valueString := fmt.Sprintf("%v", val) return valueString != "" } From 9d578f27ef6bd35ebc50cc3d82e5b35ce60d1edc Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Fri, 15 Aug 2025 09:10:35 -0400 Subject: [PATCH 7/9] fix: added extra conditional to warning about non-table values for expected tables Port change to charts v3 Signed-off-by: Jesse Simpson --- internal/chart/v3/util/coalesce.go | 2 +- internal/chart/v3/util/coalesce_test.go | 66 +++++++++++++++++++++++++ internal/chart/v3/util/values.go | 14 ++++++ 3 files changed, 81 insertions(+), 1 deletion(-) diff --git a/internal/chart/v3/util/coalesce.go b/internal/chart/v3/util/coalesce.go index caea2e119..82ba97e2a 100644 --- a/internal/chart/v3/util/coalesce.go +++ b/internal/chart/v3/util/coalesce.go @@ -300,7 +300,7 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref } else { printf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val) } - } else if istable(dv) && val != nil { + } else if istable(dv) && val != nil && isNonEmptyString(val) && isNonEmptyTable(dv) { printf("warning: destination for %s is a table. Ignoring non-table value (%v)", fullkey, val) } } diff --git a/internal/chart/v3/util/coalesce_test.go b/internal/chart/v3/util/coalesce_test.go index 4770b601d..38784af60 100644 --- a/internal/chart/v3/util/coalesce_test.go +++ b/internal/chart/v3/util/coalesce_test.go @@ -717,6 +717,72 @@ func TestCoalesceValuesWarnings(t *testing.T) { } +func TestCoalesceValuesWarningsWithEmptyDefaultMaps(t *testing.T) { + + c := withDeps(&chart.Chart{ + Metadata: &chart.Metadata{Name: "level1"}, + Values: map[string]interface{}{ + "something": map[string]interface{}{ + "somethingElse": []interface{}{}, + }, + }, + }) + + vals := map[string]interface{}{ + "something": map[string]interface{}{ + "somethingElse": map[string]interface{}{}, + }, + } + + warnings := make([]string, 0) + printf := func(format string, v ...interface{}) { + t.Logf(format, v...) + warnings = append(warnings, fmt.Sprintf(format, v...)) + } + + _, err := coalesce(printf, c, vals, "", false) + if err != nil { + t.Fatal(err) + } + + t.Logf("vals: %v", vals) + assert.NotContains(t, warnings, "warning: destination for level1.something.somethingElse is a table. Ignoring non-table value ([])") +} + +func TestCoalesceValuesWarningsWithEmptyStringDefault(t *testing.T) { + + c := withDeps(&chart.Chart{ + Metadata: &chart.Metadata{Name: "level1"}, + Values: map[string]interface{}{ + "auth": map[string]interface{}{ + "existingSecret": "", + }, + }, + }) + + vals := map[string]interface{}{ + "auth": map[string]interface{}{ + "existingSecret": map[string]interface{}{ + "name": "secretName", + }, + }, + } + + warnings := make([]string, 0) + printf := func(format string, v ...interface{}) { + t.Logf(format, v...) + warnings = append(warnings, fmt.Sprintf(format, v...)) + } + + _, err := coalesce(printf, c, vals, "", false) + if err != nil { + t.Fatal(err) + } + + t.Logf("vals: %v", vals) + assert.NotContains(t, warnings, "warning: destination for level1.auth.existingSecret is a table. Ignoring non-table value ()") +} + func TestConcatPrefix(t *testing.T) { assert.Equal(t, "b", concatPrefix("", "b")) assert.Equal(t, "a.b", concatPrefix("a", "b")) diff --git a/internal/chart/v3/util/values.go b/internal/chart/v3/util/values.go index 8e1a14b45..43fb2c2ab 100644 --- a/internal/chart/v3/util/values.go +++ b/internal/chart/v3/util/values.go @@ -179,6 +179,20 @@ func istable(v interface{}) bool { return ok } +func isNonEmptyTable(val interface{}) bool { + table, ok := val.(map[string]interface{}) + if !ok { + return false + } + + return len(table) > 0 +} + +func isNonEmptyString(val interface{}) bool { + valueString := fmt.Sprintf("%v", val) + return valueString != "" +} + // PathValue takes a path that traverses a YAML structure and returns the value at the end of that path. // The path starts at the root of the YAML structure and is comprised of YAML keys separated by periods. // Given the following YAML data the value at path "chapter.one.title" is "Loomings". From d4f968f51e39426896ea45af57116b39e62be9d7 Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Fri, 15 Aug 2025 09:12:35 -0400 Subject: [PATCH 8/9] fix: undo changes to chart v2 Signed-off-by: Jesse Simpson --- pkg/chart/v2/util/coalesce.go | 2 +- pkg/chart/v2/util/coalesce_test.go | 66 ------------------------------ pkg/chart/v2/util/values.go | 14 ------- 3 files changed, 1 insertion(+), 81 deletions(-) diff --git a/pkg/chart/v2/util/coalesce.go b/pkg/chart/v2/util/coalesce.go index 307603bae..a3e0f5ae8 100644 --- a/pkg/chart/v2/util/coalesce.go +++ b/pkg/chart/v2/util/coalesce.go @@ -300,7 +300,7 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref } else { printf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val) } - } else if istable(dv) && val != nil && isNonEmptyString(val) && isNonEmptyTable(dv) { + } else if istable(dv) && val != nil { printf("warning: destination for %s is a table. Ignoring non-table value (%v)", fullkey, val) } } diff --git a/pkg/chart/v2/util/coalesce_test.go b/pkg/chart/v2/util/coalesce_test.go index 9559ca81d..e2c45a435 100644 --- a/pkg/chart/v2/util/coalesce_test.go +++ b/pkg/chart/v2/util/coalesce_test.go @@ -717,72 +717,6 @@ func TestCoalesceValuesWarnings(t *testing.T) { } -func TestCoalesceValuesWarningsWithEmptyDefaultMaps(t *testing.T) { - - c := withDeps(&chart.Chart{ - Metadata: &chart.Metadata{Name: "level1"}, - Values: map[string]interface{}{ - "something": map[string]interface{}{ - "somethingElse": []interface{}{}, - }, - }, - }) - - vals := map[string]interface{}{ - "something": map[string]interface{}{ - "somethingElse": map[string]interface{}{}, - }, - } - - warnings := make([]string, 0) - printf := func(format string, v ...interface{}) { - t.Logf(format, v...) - warnings = append(warnings, fmt.Sprintf(format, v...)) - } - - _, err := coalesce(printf, c, vals, "", false) - if err != nil { - t.Fatal(err) - } - - t.Logf("vals: %v", vals) - assert.NotContains(t, warnings, "warning: destination for level1.something.somethingElse is a table. Ignoring non-table value ([])") -} - -func TestCoalesceValuesWarningsWithEmptyStringDefault(t *testing.T) { - - c := withDeps(&chart.Chart{ - Metadata: &chart.Metadata{Name: "level1"}, - Values: map[string]interface{}{ - "auth": map[string]interface{}{ - "existingSecret": "", - }, - }, - }) - - vals := map[string]interface{}{ - "auth": map[string]interface{}{ - "existingSecret": map[string]interface{}{ - "name": "secretName", - }, - }, - } - - warnings := make([]string, 0) - printf := func(format string, v ...interface{}) { - t.Logf(format, v...) - warnings = append(warnings, fmt.Sprintf(format, v...)) - } - - _, err := coalesce(printf, c, vals, "", false) - if err != nil { - t.Fatal(err) - } - - t.Logf("vals: %v", vals) - assert.NotContains(t, warnings, "warning: destination for level1.auth.existingSecret is a table. Ignoring non-table value ()") -} - func TestConcatPrefix(t *testing.T) { assert.Equal(t, "b", concatPrefix("", "b")) assert.Equal(t, "a.b", concatPrefix("a", "b")) diff --git a/pkg/chart/v2/util/values.go b/pkg/chart/v2/util/values.go index 5549580ce..6850e8b9b 100644 --- a/pkg/chart/v2/util/values.go +++ b/pkg/chart/v2/util/values.go @@ -179,20 +179,6 @@ func istable(v interface{}) bool { return ok } -func isNonEmptyTable(val interface{}) bool { - table, ok := val.(map[string]interface{}) - if !ok { - return false - } - - return len(table) > 0 -} - -func isNonEmptyString(val interface{}) bool { - valueString := fmt.Sprintf("%v", val) - return valueString != "" -} - // PathValue takes a path that traverses a YAML structure and returns the value at the end of that path. // The path starts at the root of the YAML structure and is comprised of YAML keys separated by periods. // Given the following YAML data the value at path "chapter.one.title" is "Loomings". From edfc7eb318ec40955c6ea47f5a65c8203643a32b Mon Sep 17 00:00:00 2001 From: Jesse Simpson Date: Sun, 17 Aug 2025 12:33:09 -0400 Subject: [PATCH 9/9] refactor: replace log.Printf with slog.Warn all print calls from these functions are warn anyway Signed-off-by: Jesse Simpson --- internal/chart/v3/util/coalesce.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/chart/v3/util/coalesce.go b/internal/chart/v3/util/coalesce.go index 82ba97e2a..2029e1be5 100644 --- a/internal/chart/v3/util/coalesce.go +++ b/internal/chart/v3/util/coalesce.go @@ -18,7 +18,7 @@ package util import ( "fmt" - "log" + "log/slog" "maps" "github.com/mitchellh/copystructure" @@ -47,7 +47,7 @@ func CoalesceValues(chrt *chart.Chart, vals map[string]interface{}) (Values, err if err != nil { return vals, err } - return coalesce(log.Printf, chrt, valsCopy, "", false) + return coalesce(slog.Warn, chrt, valsCopy, "", false) } // MergeValues is used to merge the values in a chart and its subcharts. This @@ -69,7 +69,7 @@ func MergeValues(chrt *chart.Chart, vals map[string]interface{}) (Values, error) if err != nil { return vals, err } - return coalesce(log.Printf, chrt, valsCopy, "", true) + return coalesce(slog.Warn, chrt, valsCopy, "", true) } func copyValues(vals map[string]interface{}) (Values, error) { @@ -263,11 +263,11 @@ func childChartMergeTrue(chrt *chart.Chart, key string, merge bool) bool { // // dest is considered authoritative. func CoalesceTables(dst, src map[string]interface{}) map[string]interface{} { - return coalesceTablesFullKey(log.Printf, dst, src, "", false) + return coalesceTablesFullKey(slog.Warn, dst, src, "", false) } func MergeTables(dst, src map[string]interface{}) map[string]interface{} { - return coalesceTablesFullKey(log.Printf, dst, src, "", true) + return coalesceTablesFullKey(slog.Warn, dst, src, "", true) } // coalesceTablesFullKey merges a source map into a destination map.