diff --git a/pkg/chart/v2/util/coalesce.go b/pkg/chart/v2/util/coalesce.go index b287b8324..448f2d61d 100644 --- a/pkg/chart/v2/util/coalesce.go +++ b/pkg/chart/v2/util/coalesce.go @@ -133,14 +133,16 @@ func coalesceGlobals(log *slog.Logger, dest, src map[string]interface{}, prefix if destglob, ok := dest[GlobalKey]; !ok { dg = make(map[string]interface{}) } else if dg, ok = destglob.(map[string]interface{}); !ok { - log.Warn(fmt.Sprintf("skipping globals because destination %s is not a table.", GlobalKey), slog.String("key", GlobalKey), slog.String("error", "not a table")) + // Destination is not a table + log.Warn("skipping coalescing global values", slog.String("key", GlobalKey), slog.String("error", "key is not a table")) return } if srcglob, ok := src[GlobalKey]; !ok { sg = make(map[string]interface{}) } else if sg, ok = srcglob.(map[string]interface{}); !ok { - log.Warn(fmt.Sprintf("skipping globals because source %s is not a table.", GlobalKey), slog.String("key", GlobalKey), slog.String("error", "not a table")) + // Source is not a table + log.Warn("skipping coalescing global values", slog.String("key", GlobalKey), slog.String("error", "key is not a table")) return } @@ -156,7 +158,7 @@ func coalesceGlobals(log *slog.Logger, dest, src map[string]interface{}, prefix dg[key] = vv } else { if destvmap, ok := destv.(map[string]interface{}); !ok { - log.Warn(fmt.Sprintf("cannot merge map onto non-map for %q. Skipping.", key), slog.String("key", key), slog.String("error", "cannot merge map onto non-map")) + log.Warn("skipping key", slog.String("key", key), slog.String("error", "cannot merge table and non-table values")) } else { // Basically, we reverse order of coalesce here to merge // top-down. @@ -170,7 +172,7 @@ func coalesceGlobals(log *slog.Logger, dest, src map[string]interface{}, prefix } } else if dv, ok := dg[key]; ok && istable(dv) { // It's not clear if this condition can actually ever trigger. - log.Warn(fmt.Sprintf("key %s is table. Skipping", key), slog.String("key", key), slog.String("error", "cannot merge as key is a table")) + log.Warn("skipping key", slog.String("key", key), slog.String("error", "cannot merge table and non-table values")) } else { // TODO: Do we need to do any additional checking on the value? dg[key] = val @@ -232,7 +234,7 @@ func coalesceValues(c *chart.Chart, v map[string]interface{}, prefix string, mer // If the original value is nil, there is nothing to coalesce, so we don't print // the warning if val != nil { - slog.Warn(fmt.Sprintf("skipped value for %s.%s", subPrefix, key), slog.String("chart", c.Name()), slog.String("key", fmt.Sprintf("%s.%s", subPrefix, key)), slog.String("error", "not a table")) + slog.Warn("skipping key", slog.String("chart", c.Name()), slog.String("key", fmt.Sprintf("%s.%s", subPrefix, key)), slog.String("error", "cannot merge table and non-table values")) } } else { // If the key is a child chart, coalesce tables with Merge set to true @@ -297,10 +299,10 @@ func coalesceTablesFullKey(log *slog.Logger, dst, src map[string]interface{}, pr if istable(dv) { coalesceTablesFullKey(log, dv.(map[string]interface{}), val.(map[string]interface{}), fullkey, merge) } else { - log.Warn(fmt.Sprintf("cannot overwrite table with non table for %s (%v)", fullkey, val), slog.String("key", fullkey), slog.String("error", "cannot overwrite table with non-table")) + log.Warn("skipping key", slog.String("key", fullkey), slog.String("error", "cannot merge table and non-table values")) } } else if istable(dv) && val != nil { - log.Warn(fmt.Sprintf("destination for %s is a table. Ignoring non-table value (%v)", fullkey, val), slog.String("key", fullkey), slog.String("error", "destination is a table")) + log.Warn("skipping key", slog.String("key", fullkey), slog.String("error", "cannot merge table and non-table values")) } } return dst diff --git a/pkg/chart/v2/util/coalesce_test.go b/pkg/chart/v2/util/coalesce_test.go index 55c34ee68..3da7f7106 100644 --- a/pkg/chart/v2/util/coalesce_test.go +++ b/pkg/chart/v2/util/coalesce_test.go @@ -716,6 +716,7 @@ func TestCoalesceValuesWarnings(t *testing.T) { capturedLogOutput := handler.Capture() t.Logf("vals: %v", vals) + t.Logf("logs: %s", capturedLogOutput.String()) // All warnings should have context as to where the warning is being emitted capturedLogOutput.Filter(RecordLevelMatches(slog.LevelWarn)). @@ -724,25 +725,93 @@ func TestCoalesceValuesWarnings(t *testing.T) { HasAttr("error"). HasAttr("key") - capturedLogOutput.Filter(RecordMessageMatches("skipped value for level1.level2.level3.boat")). + capturedLogOutput.Filter(RecordMessageMatches("skipping key")). + Filter(RecordHasAttrValue("key", "level1.level2.level3.boat")). AssertThat(t). MatchesExactly(1). AtLevel(slog.LevelWarn). - HasAttrValueString("chart", "level3") + HasAttrValueString("chart", "level3"). + HasAttrValueString("error", "cannot merge table and non-table values") - capturedLogOutput.Filter(RecordMessageMatches("destination for level1.level2.level3.spear.tip is a table. Ignoring non-table value (true)")). + capturedLogOutput.Filter(RecordMessageMatches("skipping key")). + Filter(RecordHasAttrValue("key", "level1.level2.level3.spear.tip")). AssertThat(t). MatchesExactly(1). AtLevel(slog.LevelWarn). HasAttrValueString("chart", "level3"). - HasAttr("error") + HasAttrValueString("error", "cannot merge table and non-table values") - capturedLogOutput.Filter(RecordMessageMatches("cannot overwrite table with non table for level1.level2.level3.spear.sail (map[cotton:true])")). + capturedLogOutput.Filter(RecordMessageMatches("skipping key")). + Filter(RecordHasAttrValue("key", "level1.level2.level3.spear.sail")). AssertThat(t). MatchesExactly(1). AtLevel(slog.LevelWarn). HasAttrValueString("chart", "level3"). - HasAttr("error") + HasAttrValueString("error", "cannot merge table and non-table values") + + // Reset and set the default logger back to its original state + handler.Reset() + slog.SetDefault(defaultLogger) +} + +func TestCoalesceValuesTopLevelGlobalsWarningsSrc(t *testing.T) { + + /* + Test case - user supplies a chart with the values setting the entire globals block to a value (true) + and not a map(table). This should result in a warning and immediately skips the global block + */ + c := withDeps(&chart.Chart{ + Metadata: &chart.Metadata{Name: "level1"}, + Values: map[string]interface{}{ + "global": map[string]interface{}{ + "inner": true, + }, + "name": "moby", + }, + }, withDeps(&chart.Chart{ + Metadata: &chart.Metadata{Name: "level2"}, + Values: map[string]interface{}{ + "global": map[string]interface{}{ + "inner": true, + }, + "name": "pequod", + }, + })) + + vals := map[string]interface{}{ + "global": true, + "name": "newmoby", + } + + // Get all logs emitted from slog + defaultLogger := slog.Default() + handler := NewLogCaptureHandler(nil) + slog.SetDefault(slog.New(handler)) + + _, err := coalesce(c, vals, "", false) + if err != nil { + t.Fatal(err) + } + + // Capture the logs + capturedLogOutput := handler.Capture() + + t.Logf("vals: %v", vals) + t.Logf("logs: %s", capturedLogOutput.String()) + + // All warnings should have context as to where the warning is being emitted + capturedLogOutput.Filter(RecordLevelMatches(slog.LevelWarn)). + AssertThat(t). + HasAttr("chart"). + HasAttr("error"). + HasAttr("key") + + capturedLogOutput.Filter(RecordMessageMatches("skipping coalescing global values")). + AssertThat(t). + MatchesExactly(1). + AtLevel(slog.LevelWarn). + HasAttrValueString("chart", "level1"). + HasAttrValueString("key", "global") // Reset and set the default logger back to its original state handler.Reset() diff --git a/pkg/chart/v2/util/utilities_test.go b/pkg/chart/v2/util/utilities_test.go index d8a6188e7..da6b51d67 100644 --- a/pkg/chart/v2/util/utilities_test.go +++ b/pkg/chart/v2/util/utilities_test.go @@ -151,6 +151,18 @@ func (h *LogCaptureHandler) Capture() *LogCaptureSlice { return &result } +func (c *LogCaptureSlice) String() string { + result := "" + for i, record := range c.records { + if i != 0 { + result += "\n" + } + result += record.record.Message + } + + return result +} + func (c *LogCaptureSlice) Filter(filterFn func(r *LogCaptureRecord) bool) *LogCaptureSlice { result := LogCaptureSlice{ records: make([]LogCaptureRecord, 0), @@ -207,6 +219,30 @@ func RecordLevelBelow(level slog.Level) func(r *LogCaptureRecord) bool { } } +func RecordHasAttr(key string) func(r *LogCaptureRecord) bool { + return func(r *LogCaptureRecord) bool { + for _, attr := range r.attrs { + if attr.Key == key { + return true + } + } + + return false + } +} + +func RecordHasAttrValue(key string, value string) func(r *LogCaptureRecord) bool { + return func(r *LogCaptureRecord) bool { + for _, attr := range r.attrs { + if attr.Key == key && attr.Value.String() == value { + return true + } + } + + return false + } +} + // Start an assertion builder for test validation func (c *LogCaptureSlice) AssertThat(t *testing.T) *LogCaptureSliceAssertionBuilder { return &LogCaptureSliceAssertionBuilder{