From 0714420bcfff2a7f9c48686e32551181b7f7c28b Mon Sep 17 00:00:00 2001 From: James Payne Date: Fri, 18 Apr 2025 18:06:11 +0100 Subject: [PATCH] Updatse coalesce.go to use log/slog and update tests to pass Signed-off-by: James Payne --- pkg/chart/v2/util/coalesce.go | 52 +++++------ pkg/chart/v2/util/coalesce_test.go | 24 ++--- pkg/chart/v2/util/utilities_test.go | 131 ++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 36 deletions(-) create mode 100644 pkg/chart/v2/util/utilities_test.go diff --git a/pkg/chart/v2/util/coalesce.go b/pkg/chart/v2/util/coalesce.go index 33d2d2833..ddee50696 100644 --- a/pkg/chart/v2/util/coalesce.go +++ b/pkg/chart/v2/util/coalesce.go @@ -18,7 +18,7 @@ package util import ( "fmt" - "log" + "log/slog" "github.com/mitchellh/copystructure" "github.com/pkg/errors" @@ -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(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(chrt, valsCopy, "", true) } func copyValues(vals map[string]interface{}) (Values, error) { @@ -96,13 +96,13 @@ type printFn func(format string, v ...interface{}) // Note, the merge argument specifies whether this is being used by MergeValues // or CoalesceValues. Coalescing removes null values and their keys in some // situations while merging keeps the null values. -func coalesce(printf printFn, ch *chart.Chart, dest map[string]interface{}, prefix string, merge bool) (map[string]interface{}, error) { - coalesceValues(printf, ch, dest, prefix, merge) - return coalesceDeps(printf, ch, dest, prefix, merge) +func coalesce(ch *chart.Chart, dest map[string]interface{}, prefix string, merge bool) (map[string]interface{}, error) { + coalesceValues(ch, dest, prefix, merge) + return coalesceDeps(ch, dest, prefix, merge) } // coalesceDeps coalesces the dependencies of the given chart. -func coalesceDeps(printf printFn, chrt *chart.Chart, dest map[string]interface{}, prefix string, merge bool) (map[string]interface{}, error) { +func coalesceDeps(chrt *chart.Chart, dest map[string]interface{}, prefix string, merge bool) (map[string]interface{}, error) { for _, subchart := range chrt.Dependencies() { if c, ok := dest[subchart.Name()]; !ok { // If dest doesn't already have the key, create it. @@ -114,10 +114,10 @@ func coalesceDeps(printf printFn, chrt *chart.Chart, dest map[string]interface{} dvmap := dv.(map[string]interface{}) subPrefix := concatPrefix(prefix, chrt.Metadata.Name) // Get globals out of dest and merge them into dvmap. - coalesceGlobals(printf, dvmap, dest, subPrefix, merge) + coalesceGlobals(dvmap, dest, subPrefix, merge) // Now coalesce the rest of the values. var err error - dest[subchart.Name()], err = coalesce(printf, subchart, dvmap, subPrefix, merge) + dest[subchart.Name()], err = coalesce(subchart, dvmap, subPrefix, merge) if err != nil { return dest, err } @@ -129,20 +129,20 @@ func coalesceDeps(printf printFn, chrt *chart.Chart, dest map[string]interface{} // coalesceGlobals copies the globals out of src and merges them into dest. // // For convenience, returns dest. -func coalesceGlobals(printf printFn, dest, src map[string]interface{}, prefix string, _ bool) { +func coalesceGlobals(dest, src map[string]interface{}, prefix string, _ bool) { var dg, sg map[string]interface{} if destglob, ok := dest[GlobalKey]; !ok { dg = make(map[string]interface{}) } else if dg, ok = destglob.(map[string]interface{}); !ok { - printf("warning: skipping globals because destination %s is not a table.", GlobalKey) + slog.Warn(fmt.Sprintf("warning: skipping globals because destination %s is not a table.", GlobalKey)) return } if srcglob, ok := src[GlobalKey]; !ok { sg = make(map[string]interface{}) } else if sg, ok = srcglob.(map[string]interface{}); !ok { - printf("warning: skipping globals because source %s is not a table.", GlobalKey) + slog.Warn(fmt.Sprintf("warning: skipping globals because source %s is not a table.", GlobalKey)) return } @@ -158,7 +158,7 @@ func coalesceGlobals(printf printFn, dest, src map[string]interface{}, prefix st dg[key] = vv } else { if destvmap, ok := destv.(map[string]interface{}); !ok { - printf("Conflict: cannot merge map onto non-map for %q. Skipping.", key) + slog.Warn(fmt.Sprintf("Conflict: cannot merge map onto non-map for %q. Skipping.", key)) } else { // Basically, we reverse order of coalesce here to merge // top-down. @@ -166,13 +166,13 @@ func coalesceGlobals(printf printFn, dest, src map[string]interface{}, prefix st // In this location coalesceTablesFullKey should always have // merge set to true. The output of coalesceGlobals is run // through coalesce where any nils will be removed. - coalesceTablesFullKey(printf, vv, destvmap, subPrefix, true) + coalesceTablesFullKey(vv, destvmap, subPrefix, true) dg[key] = vv } } } else if dv, ok := dg[key]; ok && istable(dv) { // It's not clear if this condition can actually ever trigger. - printf("key %s is table. Skipping", key) + slog.Warn(fmt.Sprintf("key %s is table. Skipping", key)) } else { // TODO: Do we need to do any additional checking on the value? dg[key] = val @@ -192,7 +192,7 @@ func copyMap(src map[string]interface{}) map[string]interface{} { // coalesceValues builds up a values map for a particular chart. // // Values in v will override the values in the chart. -func coalesceValues(printf printFn, c *chart.Chart, v map[string]interface{}, prefix string, merge bool) { +func coalesceValues(c *chart.Chart, v map[string]interface{}, prefix string, merge bool) { subPrefix := concatPrefix(prefix, c.Metadata.Name) // Using c.Values directly when coalescing a table can cause problems where @@ -206,7 +206,7 @@ func coalesceValues(printf printFn, c *chart.Chart, v map[string]interface{}, pr // means there is a problem in the deep copying package or something // wrong with c.Values. In this case we will use c.Values and report // an error. - printf("warning: unable to copy values, err: %s", err) + slog.Warn(fmt.Sprintf("warning: unable to copy values, err: %s", err)) vc = c.Values } else { vc, ok = valuesCopy.(map[string]interface{}) @@ -214,7 +214,7 @@ func coalesceValues(printf printFn, c *chart.Chart, v map[string]interface{}, pr // c.Values has a map[string]interface{} structure. If the copy of // it cannot be treated as map[string]interface{} there is something // strangely wrong. Log it and use c.Values - printf("warning: unable to convert values copy to values type") + slog.Warn(fmt.Sprintf("warning: unable to convert values copy to values type")) vc = c.Values } } @@ -234,7 +234,7 @@ func coalesceValues(printf printFn, c *chart.Chart, v map[string]interface{}, pr // If the original value is nil, there is nothing to coalesce, so we don't print // the warning if val != nil { - printf("warning: skipped value for %s.%s: Not a table.", subPrefix, key) + slog.Warn(fmt.Sprintf("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 @@ -242,7 +242,7 @@ func coalesceValues(printf printFn, c *chart.Chart, v map[string]interface{}, pr // Because v has higher precedence than nv, dest values override src // values. - coalesceTablesFullKey(printf, dest, src, concatPrefix(subPrefix, key), merge) + coalesceTablesFullKey(dest, src, concatPrefix(subPrefix, key), merge) } } } else { @@ -265,17 +265,17 @@ 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(dst, src, "", false) } func MergeTables(dst, src map[string]interface{}) map[string]interface{} { - return coalesceTablesFullKey(log.Printf, dst, src, "", true) + return coalesceTablesFullKey(dst, src, "", true) } // coalesceTablesFullKey merges a source map into a destination map. // // dest is considered authoritative. -func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, prefix string, merge bool) map[string]interface{} { +func coalesceTablesFullKey(dst, src map[string]interface{}, prefix string, merge bool) map[string]interface{} { // When --reuse-values is set but there are no modifications yet, return new values if src == nil { return dst @@ -293,12 +293,12 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref dst[key] = val } else if istable(val) { if istable(dv) { - coalesceTablesFullKey(printf, dv.(map[string]interface{}), val.(map[string]interface{}), fullkey, merge) + coalesceTablesFullKey(dv.(map[string]interface{}), val.(map[string]interface{}), fullkey, merge) } else { - printf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val) + slog.Warn(fmt.Sprintf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val)) } } else if istable(dv) && val != nil { - printf("warning: destination for %s is a table. Ignoring non-table value (%v)", fullkey, val) + slog.Warn(fmt.Sprintf("warning: destination for %s is a table. Ignoring non-table value (%v)", fullkey, val)) } } return dst diff --git a/pkg/chart/v2/util/coalesce_test.go b/pkg/chart/v2/util/coalesce_test.go index 3d4ee4fa8..829b6c939 100644 --- a/pkg/chart/v2/util/coalesce_test.go +++ b/pkg/chart/v2/util/coalesce_test.go @@ -18,7 +18,7 @@ package util import ( "encoding/json" - "fmt" + "log/slog" "testing" "github.com/stretchr/testify/assert" @@ -702,22 +702,26 @@ func TestCoalesceValuesWarnings(t *testing.T) { }, } - warnings := make([]string, 0) - printf := func(format string, v ...interface{}) { - t.Logf(format, v...) - warnings = append(warnings, fmt.Sprintf(format, v...)) - } + // Capture logs emitted from slog + defaultLogger := slog.Default() + logCaptureHandler := NewLogCaptureHandler(nil) + slog.SetDefault(slog.New(logCaptureHandler)) - _, err := coalesce(printf, c, vals, "", false) + _, err := coalesce(c, vals, "", false) if err != nil { t.Fatal(err) } + logOutput := logCaptureHandler.Records().AsMessageSlice() + t.Logf("vals: %v", vals) - assert.Contains(t, warnings, "warning: skipped value for level1.level2.level3.boat: Not a table.") - assert.Contains(t, warnings, "warning: destination for level1.level2.level3.spear.tip is a table. Ignoring non-table value (true)") - assert.Contains(t, warnings, "warning: cannot overwrite table with non table for level1.level2.level3.spear.sail (map[cotton:true])") + assert.Contains(t, logOutput, "warning: skipped value for level1.level2.level3.boat: Not a table.") + assert.Contains(t, logOutput, "warning: destination for level1.level2.level3.spear.tip is a table. Ignoring non-table value (true)") + assert.Contains(t, logOutput, "warning: cannot overwrite table with non table for level1.level2.level3.spear.sail (map[cotton:true])") + // Reset and set the default logger back to its original state + logCaptureHandler.Reset() + slog.SetDefault(defaultLogger) } func TestConcatPrefix(t *testing.T) { diff --git a/pkg/chart/v2/util/utilities_test.go b/pkg/chart/v2/util/utilities_test.go new file mode 100644 index 000000000..541128697 --- /dev/null +++ b/pkg/chart/v2/util/utilities_test.go @@ -0,0 +1,131 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package util + +import ( + "context" + "log/slog" + "sync" +) + +// A simple handler to record the log messages emitted by slog. +// Intended to be used only by tests +type LogCaptureHandler struct { + mu *sync.Mutex + records []slog.Record + opts slog.HandlerOptions +} + +type LogCaptureSlice []slog.Record + +func NewLogCaptureHandler(opts *slog.HandlerOptions) *LogCaptureHandler { + if opts == nil { + opts = &slog.HandlerOptions{} + } + return &LogCaptureHandler{ + mu: &sync.Mutex{}, + opts: *opts, + } +} + +func (h *LogCaptureHandler) Enabled(_ context.Context, _ slog.Level) bool { + // Handle all levels + return true +} + +func (h *LogCaptureHandler) Handle(_ context.Context, r slog.Record) error { + h.mu.Lock() + defer h.mu.Unlock() + h.records = append(h.records, r) + return nil +} + +// This is intentionally not following "standard" usage of WithAttrs and WithGroup with regard +// to handlers since we want to be able to capture all messages emitted from slog sources rather +// than each sub-logger generating its own slice of records +func (h *LogCaptureHandler) WithAttrs(attrs []slog.Attr) slog.Handler { + return &LogCaptureHandler{ + // Copy the mutex as we could be writing to the same records from multiple threads + mu: h.mu, + opts: slog.HandlerOptions{ + Level: h.opts.Level, + AddSource: h.opts.AddSource, + ReplaceAttr: h.opts.ReplaceAttr, + }, + + // Copy the same records as we want to be able to collate _all_ the log messages + // emitted by slog sources + records: h.records, + } +} + +// See comment on WithAttrs regarding reusing the mutex + records slice +func (h *LogCaptureHandler) WithGroup(name string) slog.Handler { + return &LogCaptureHandler{ + mu: h.mu, + opts: slog.HandlerOptions{ + Level: h.opts.Level, + AddSource: h.opts.AddSource, + ReplaceAttr: h.opts.ReplaceAttr, + }, + + // Copy the same records as we want to be able to collate _all_ the log messages + // emitted by slog sources + records: h.records, + } +} + +// Records returns a copy of the captured log records +func (h *LogCaptureHandler) Records() *LogCaptureSlice { + h.mu.Lock() + defer h.mu.Unlock() + + records := make([]slog.Record, len(h.records)) + copy(records, h.records) + + result := LogCaptureSlice(records) + return &result +} + +// Converts the captured log records into a map of log message and log level +func (l *LogCaptureSlice) AsMessageLevelMap() map[string]slog.Level { + + result := make(map[string]slog.Level) + for _, record := range *l { + result[record.Message] = record.Level + } + + return result +} + +// Converts the captured log records into a slice of log messages +func (l *LogCaptureSlice) AsMessageSlice() []string { + + result := make([]string, 0) + for _, record := range *l { + result = append(result, record.Message) + } + + return result +} + +// Reset clears the captured log messages. +func (h *LogCaptureHandler) Reset() { + h.mu.Lock() + defer h.mu.Unlock() + h.records = make([]slog.Record, 0) +}