Update log messages to better leverage the slog k:v pairs

Update log messages to use consistent language throughout

Signed-off-by: James Payne <jamoflaw@gmail.com>

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

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()

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{