From 02d070ebdc2f9dc4553898d00d1ed998fe339bca Mon Sep 17 00:00:00 2001 From: Rafael da Fonseca Date: Fri, 7 Mar 2025 16:08:44 +0000 Subject: [PATCH 1/6] feat: Merge lists when passing multiple values files, allowing to signal a unique key for merging lists of maps Signed-off-by: Rafael da Fonseca --- pkg/chart/v2/loader/load.go | 63 +++++++++++++++++++++++++++++++++++-- 1 file changed, 61 insertions(+), 2 deletions(-) diff --git a/pkg/chart/v2/loader/load.go b/pkg/chart/v2/loader/load.go index 3c5463720..c00481f4b 100644 --- a/pkg/chart/v2/loader/load.go +++ b/pkg/chart/v2/loader/load.go @@ -24,6 +24,7 @@ import ( "log" "os" "path/filepath" + "reflect" "strings" "github.com/pkg/errors" @@ -236,21 +237,79 @@ func LoadValues(data io.Reader) (map[string]interface{}, error) { // MergeMaps merges two maps. If a key exists in both maps, the value from b will be used. // If the value is a map, the maps will be merged recursively. +// If the value is a list, the lists will be merged func MergeMaps(a, b map[string]interface{}) map[string]interface{} { out := make(map[string]interface{}, len(a)) for k, v := range a { out[k] = v } for k, v := range b { - if v, ok := v.(map[string]interface{}); ok { + + if val, ok := v.(map[string]interface{}); ok { if bv, ok := out[k]; ok { if bv, ok := bv.(map[string]interface{}); ok { - out[k] = MergeMaps(bv, v) + out[k] = MergeMaps(bv, val) continue } } + } else if reflect.TypeOf(v).Kind() == reflect.Slice { + if sourceList, ok := out[k].([]map[string]interface{}); ok { + + val, ok := v.([]map[string]interface{}) + if !ok { + log.Println("Property mismatch during merge") + out[k] = v + continue + } + + out[k] = MergeMapLists(sourceList, val) + continue + } else if sourceList, ok := out[k].([]interface{}); ok { + if val, ok := v.([]interface{}); ok { + out[k] = append(sourceList, val...) + } else { + out[k] = v + } + continue + } + } out[k] = v } return out } + +// MergeMapLists merges two lists of maps. If a prefix of * is set on a map key, +// that key will be used to de-duplicate/merge with the source map +func MergeMapLists(a, b []map[string]interface{}) []map[string]interface{} { + out := a + for j, mapEntry := range b { + var mergeKey string + var mergeValue interface{} + for k, v := range mapEntry { + if strings.HasPrefix(k, "*") { + mergeKey = k + mergeValue = v + b[j][strings.TrimPrefix(mergeKey, "*")] = v + delete(b[j], mergeKey) + break + } + } + if len(mergeKey) > 0 { + strippedMergeKey := strings.TrimPrefix(mergeKey, "*") + for i, sourceMapEntry := range out { + for k, v := range sourceMapEntry { + if (k == strippedMergeKey || k == mergeKey) && v == mergeValue { + mergedMapEntry := MergeMaps(sourceMapEntry, mapEntry) + out[i] = mergedMapEntry + break + } + } + } + } else { + out = append(out, mapEntry) + } + } + return out + +} From 825fc2b0bc0e848ccaca96d0902be54c890d3328 Mon Sep 17 00:00:00 2001 From: Rafael da Fonseca Date: Fri, 7 Mar 2025 16:27:49 +0000 Subject: [PATCH 2/6] only merge lists with prefix Signed-off-by: Rafael da Fonseca --- pkg/chart/v2/loader/load.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/pkg/chart/v2/loader/load.go b/pkg/chart/v2/loader/load.go index c00481f4b..3dd2d4c90 100644 --- a/pkg/chart/v2/loader/load.go +++ b/pkg/chart/v2/loader/load.go @@ -34,6 +34,8 @@ import ( chart "helm.sh/helm/v4/pkg/chart/v2" ) +const mergePrefix = "*" + // ChartLoader loads a chart. type ChartLoader interface { Load() (*chart.Chart, error) @@ -252,23 +254,25 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { continue } } - } else if reflect.TypeOf(v).Kind() == reflect.Slice { - if sourceList, ok := out[k].([]map[string]interface{}); ok { + } else if reflect.TypeOf(v).Kind() == reflect.Slice && strings.HasPrefix(k, mergePrefix) { + strippedKey := strings.TrimPrefix(k, mergePrefix) + out[strippedKey] = out[k] + delete(out, k) + if sourceList, ok := out[strippedKey].([]map[string]interface{}); ok { val, ok := v.([]map[string]interface{}) if !ok { log.Println("Property mismatch during merge") - out[k] = v continue } - out[k] = MergeMapLists(sourceList, val) + out[strippedKey] = MergeMapLists(sourceList, val) continue - } else if sourceList, ok := out[k].([]interface{}); ok { + } else if sourceList, ok := out[strippedKey].([]interface{}); ok { if val, ok := v.([]interface{}); ok { - out[k] = append(sourceList, val...) + out[strippedKey] = append(sourceList, val...) } else { - out[k] = v + out[strippedKey] = v } continue } @@ -287,16 +291,16 @@ func MergeMapLists(a, b []map[string]interface{}) []map[string]interface{} { var mergeKey string var mergeValue interface{} for k, v := range mapEntry { - if strings.HasPrefix(k, "*") { + if strings.HasPrefix(k, mergePrefix) { mergeKey = k mergeValue = v - b[j][strings.TrimPrefix(mergeKey, "*")] = v + b[j][strings.TrimPrefix(mergeKey, mergePrefix)] = v delete(b[j], mergeKey) break } } if len(mergeKey) > 0 { - strippedMergeKey := strings.TrimPrefix(mergeKey, "*") + strippedMergeKey := strings.TrimPrefix(mergeKey, mergePrefix) for i, sourceMapEntry := range out { for k, v := range sourceMapEntry { if (k == strippedMergeKey || k == mergeKey) && v == mergeValue { From baa0fdf6e7cd0459e0d7a637f7990cf9b8e5b2af Mon Sep 17 00:00:00 2001 From: Rafael da Fonseca Date: Fri, 14 Mar 2025 09:16:00 +0000 Subject: [PATCH 3/6] Fix type assertions Signed-off-by: Rafael da Fonseca --- pkg/chart/v2/loader/load.go | 70 +++++++++++++++++++++++++------------ 1 file changed, 47 insertions(+), 23 deletions(-) diff --git a/pkg/chart/v2/loader/load.go b/pkg/chart/v2/loader/load.go index 3dd2d4c90..7cc47415a 100644 --- a/pkg/chart/v2/loader/load.go +++ b/pkg/chart/v2/loader/load.go @@ -24,7 +24,6 @@ import ( "log" "os" "path/filepath" - "reflect" "strings" "github.com/pkg/errors" @@ -34,7 +33,7 @@ import ( chart "helm.sh/helm/v4/pkg/chart/v2" ) -const mergePrefix = "*" +const mergePrefix = "\\*" // ChartLoader loads a chart. type ChartLoader interface { @@ -232,6 +231,7 @@ func LoadValues(data io.Reader) (map[string]interface{}, error) { }); err != nil { return nil, errors.Wrap(err, "cannot unmarshal yaml document") } + values = MergeMaps(values, currentMap) } return values, nil @@ -246,7 +246,6 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { out[k] = v } for k, v := range b { - if val, ok := v.(map[string]interface{}); ok { if bv, ok := out[k]; ok { if bv, ok := bv.(map[string]interface{}); ok { @@ -254,27 +253,38 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { continue } } - } else if reflect.TypeOf(v).Kind() == reflect.Slice && strings.HasPrefix(k, mergePrefix) { + } else { strippedKey := strings.TrimPrefix(k, mergePrefix) - out[strippedKey] = out[k] - delete(out, k) - if sourceList, ok := out[strippedKey].([]map[string]interface{}); ok { + if out[k] != nil { + out[strippedKey] = out[k] + delete(out, k) + } + if sourceList, ok := out[strippedKey].([]any); ok && strings.HasPrefix(k, mergePrefix) { - val, ok := v.([]map[string]interface{}) - if !ok { - log.Println("Property mismatch during merge") - continue - } + _, isMapSlice := sourceList[0].(map[string]any) + if isMapSlice { + val, ok := v.([]any) + if !ok { + // List is explicitly made null on a subsequent file + if v == nil { + delete(out, strippedKey) + continue + } else { + log.Printf("Property \"%s\" mismatch during merge", strippedKey) + continue + } + } - out[strippedKey] = MergeMapLists(sourceList, val) - continue - } else if sourceList, ok := out[strippedKey].([]interface{}); ok { - if val, ok := v.([]interface{}); ok { - out[strippedKey] = append(sourceList, val...) - } else { - out[strippedKey] = v + out[strippedKey] = MergeMapLists(sourceList, val) + continue + } else if sourceList, ok := out[strippedKey].([]any); ok { + if val, ok := v.([]any); ok { + out[strippedKey] = append(sourceList, val...) + } else { + out[strippedKey] = v + } + continue } - continue } } @@ -285,23 +295,37 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { // MergeMapLists merges two lists of maps. If a prefix of * is set on a map key, // that key will be used to de-duplicate/merge with the source map -func MergeMapLists(a, b []map[string]interface{}) []map[string]interface{} { +func MergeMapLists(a, b []any) []any { out := a for j, mapEntry := range b { + mapEntry, ok := mapEntry.(map[string]any) + if !ok { + continue + } + var mergeKey string var mergeValue interface{} for k, v := range mapEntry { if strings.HasPrefix(k, mergePrefix) { mergeKey = k mergeValue = v - b[j][strings.TrimPrefix(mergeKey, mergePrefix)] = v - delete(b[j], mergeKey) + bj, ok := b[j].(map[string]any) + if !ok { + continue + } + bj[strings.TrimPrefix(mergeKey, mergePrefix)] = v + delete(bj, mergeKey) break } } if len(mergeKey) > 0 { strippedMergeKey := strings.TrimPrefix(mergeKey, mergePrefix) + for i, sourceMapEntry := range out { + sourceMapEntry, ok := sourceMapEntry.(map[string]any) + if !ok { + continue + } for k, v := range sourceMapEntry { if (k == strippedMergeKey || k == mergeKey) && v == mergeValue { mergedMapEntry := MergeMaps(sourceMapEntry, mapEntry) From 79ff4200944656369e03959e3a86e5c59213bba9 Mon Sep 17 00:00:00 2001 From: Rafael da Fonseca Date: Fri, 14 Mar 2025 09:19:57 +0000 Subject: [PATCH 4/6] Apply var naming change suggestions Signed-off-by: Rafael da Fonseca --- pkg/chart/v2/loader/load.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/chart/v2/loader/load.go b/pkg/chart/v2/loader/load.go index 7cc47415a..e87436218 100644 --- a/pkg/chart/v2/loader/load.go +++ b/pkg/chart/v2/loader/load.go @@ -303,23 +303,23 @@ func MergeMapLists(a, b []any) []any { continue } - var mergeKey string - var mergeValue interface{} + var uniqueKey string + var dedupValue interface{} for k, v := range mapEntry { if strings.HasPrefix(k, mergePrefix) { - mergeKey = k - mergeValue = v + uniqueKey = k + dedupValue = v bj, ok := b[j].(map[string]any) if !ok { continue } - bj[strings.TrimPrefix(mergeKey, mergePrefix)] = v - delete(bj, mergeKey) + bj[strings.TrimPrefix(uniqueKey, mergePrefix)] = v + delete(bj, uniqueKey) break } } - if len(mergeKey) > 0 { - strippedMergeKey := strings.TrimPrefix(mergeKey, mergePrefix) + if len(uniqueKey) > 0 { + strippedMergeKey := strings.TrimPrefix(uniqueKey, mergePrefix) for i, sourceMapEntry := range out { sourceMapEntry, ok := sourceMapEntry.(map[string]any) @@ -327,7 +327,7 @@ func MergeMapLists(a, b []any) []any { continue } for k, v := range sourceMapEntry { - if (k == strippedMergeKey || k == mergeKey) && v == mergeValue { + if (k == strippedMergeKey || k == uniqueKey) && v == dedupValue { mergedMapEntry := MergeMaps(sourceMapEntry, mapEntry) out[i] = mergedMapEntry break From 788fbc772ce6a1ca9e963085d02bbb6a14aeefdc Mon Sep 17 00:00:00 2001 From: James M Leddy Date: Mon, 22 Sep 2025 17:10:01 -0400 Subject: [PATCH 5/6] chore(merge-lists): Add tests Per the parent pull request #30632, add tests Signed-off-by: James M Leddy --- pkg/chart/v2/loader/load.go | 86 +++++----- pkg/chart/v2/loader/load_test.go | 274 ++++++++++++++++++++++++++++++- 2 files changed, 319 insertions(+), 41 deletions(-) diff --git a/pkg/chart/v2/loader/load.go b/pkg/chart/v2/loader/load.go index 9f8f2dc7e..7e80c35d9 100644 --- a/pkg/chart/v2/loader/load.go +++ b/pkg/chart/v2/loader/load.go @@ -249,40 +249,60 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { continue } } - } else { + } else if strings.HasPrefix(k, mergePrefix) { strippedKey := strings.TrimPrefix(k, mergePrefix) if out[k] != nil { out[strippedKey] = out[k] delete(out, k) } - if sourceList, ok := out[strippedKey].([]any); ok && strings.HasPrefix(k, mergePrefix) { - - _, isMapSlice := sourceList[0].(map[string]any) - if isMapSlice { - val, ok := v.([]any) - if !ok { - // List is explicitly made null on a subsequent file - if v == nil { - delete(out, strippedKey) - continue - } else { - log.Printf("Property \"%s\" mismatch during merge", strippedKey) - continue - } - } + // Prefer non-prefixed in case of conflict + if b[strippedKey] != nil { + out[strippedKey] = b[strippedKey] + continue + } + + // List is explicitly made null on a subsequent file + if v == nil { + delete(out, strippedKey) + continue + } + + if sourceList, ok := out[strippedKey].([]map[string]interface{}); ok { + if val, ok := v.([]map[string]interface{}); ok { out[strippedKey] = MergeMapLists(sourceList, val) continue - } else if sourceList, ok := out[strippedKey].([]any); ok { - if val, ok := v.([]any); ok { - out[strippedKey] = append(sourceList, val...) - } else { - out[strippedKey] = v - } + } + + log.Printf("Property \"%s\" mismatch during merge", strippedKey) + continue + } else if sourceList, ok := out[strippedKey].([]float64); ok { + if val, ok := v.([]float64); ok { + out[strippedKey] = append(sourceList, val...) continue } - } + log.Printf("Property \"%s\" mismatch during merge", strippedKey) + continue + } else if sourceList, ok := out[strippedKey].([]bool); ok { + if val, ok := v.([]bool); ok { + out[strippedKey] = append(sourceList, val...) + continue + } + + log.Printf("Property \"%s\" mismatch during merge", strippedKey) + continue + } else if sourceList, ok := out[strippedKey].([]string); ok { + if val, ok := v.([]string); ok { + out[strippedKey] = append(sourceList, val...) + continue + } + + log.Printf("Property \"%s\" mismatch during merge", strippedKey) + continue + } else { + out[strippedKey] = v + } } out[k] = v } @@ -291,37 +311,23 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { // MergeMapLists merges two lists of maps. If a prefix of * is set on a map key, // that key will be used to de-duplicate/merge with the source map -func MergeMapLists(a, b []any) []any { +func MergeMapLists(a, b []map[string]interface{}) []map[string]interface{} { out := a for j, mapEntry := range b { - mapEntry, ok := mapEntry.(map[string]any) - if !ok { - continue - } - var uniqueKey string var dedupValue interface{} for k, v := range mapEntry { if strings.HasPrefix(k, mergePrefix) { uniqueKey = k dedupValue = v - bj, ok := b[j].(map[string]any) - if !ok { - continue - } - bj[strings.TrimPrefix(uniqueKey, mergePrefix)] = v - delete(bj, uniqueKey) - break + b[j][strings.TrimPrefix(uniqueKey, mergePrefix)] = v + delete(b[j], uniqueKey) } } if len(uniqueKey) > 0 { strippedMergeKey := strings.TrimPrefix(uniqueKey, mergePrefix) for i, sourceMapEntry := range out { - sourceMapEntry, ok := sourceMapEntry.(map[string]any) - if !ok { - continue - } for k, v := range sourceMapEntry { if (k == strippedMergeKey || k == uniqueKey) && v == dedupValue { mergedMapEntry := MergeMaps(sourceMapEntry, mapEntry) diff --git a/pkg/chart/v2/loader/load_test.go b/pkg/chart/v2/loader/load_test.go index c4ae646f6..3ce9347fb 100644 --- a/pkg/chart/v2/loader/load_test.go +++ b/pkg/chart/v2/loader/load_test.go @@ -595,10 +595,282 @@ func TestMergeValuesV2(t *testing.T) { } equal = reflect.DeepEqual(testMap, expectedMap) if !equal { - t.Errorf("Expected a map with different keys to merge properly with another map. Expected: %v, got %v", expectedMap, testMap) + t.Errorf("Expected a map with different keys to merge properly with another map. Expected values: %v, got %v", expectedMap, testMap) } } +func TestMergeLists(t *testing.T) { + stringCommonMap := map[string]interface{}{ + "foo": "bar", + "baz": []string{ + "cool", + }, + } + stringOverrideMap := map[string]interface{}{ + "foo": "bar", + "\\*baz": []string{ + "stuff", + }, + } + stringComposedMap := map[string]interface{}{ + "foo": "bar", + "baz": []string{ + "cool", + "stuff", + }, + } + testMap := MergeMaps(stringCommonMap, stringOverrideMap) + equal := reflect.DeepEqual(testMap, stringComposedMap) + if !equal { + t.Errorf("Expected lists of a bool type to compose when specified. Expected values: %v, got %v", stringComposedMap, testMap) + } + + floatCommonMap := map[string]interface{}{ + "foo": "bar", + "baz": []float64{ + 1, + }, + } + floatOverrideMap := map[string]interface{}{ + "foo": "bar", + "\\*baz": []float64{ + 2, + }, + } + floatComposedMap := map[string]interface{}{ + "foo": "bar", + "baz": []float64{ + 1, + 2, + }, + } + testMap = MergeMaps(floatCommonMap, floatOverrideMap) + equal = reflect.DeepEqual(testMap, floatComposedMap) + if !equal { + t.Errorf("Expected lists of a bool type to compose when specified. Expected values: %v, got %v", floatComposedMap, testMap) + } + + boolCommonMap := map[string]interface{}{ + "foo": "bar", + "baz": []bool{ + true, + }, + } + boolOverrideMap := map[string]interface{}{ + "foo": "bar", + "\\*baz": []bool{ + false, + }, + } + boolComposedMap := map[string]interface{}{ + "foo": "bar", + "baz": []bool{ + true, + false, + }, + } + testMap = MergeMaps(boolCommonMap, boolOverrideMap) + equal = reflect.DeepEqual(testMap, boolComposedMap) + if !equal { + t.Errorf("Expected lists of a bool type to compose when specified. Expected values: %v, got %v", boolComposedMap, testMap) + } + + testMap = MergeMaps(stringCommonMap, floatOverrideMap) + equal =reflect.DeepEqual(testMap, stringCommonMap) + if !equal { + t.Errorf("Expected lists of non-matching types not to compose. Expected values: %v, got %v", stringCommonMap, testMap) + } + + nilOverrideMap := map[string]interface{}{ + "foo": "bar", + "\\*baz": nil, + } + nilComposedMap := map[string]interface{}{ + "foo": "bar", + } + testMap = MergeMaps(stringCommonMap, nilOverrideMap) + equal = reflect.DeepEqual(testMap, nilComposedMap) + if !equal { + t.Errorf("Expected nil on maps of composed lists to override values. Expected values: %v, got %v", nilOverrideMap, testMap) + } + + stringCommonMap = map[string]interface{}{ + "foo": "bar", + "baz": []string{ + "cool", + }, + "\\*baz": []string{ + "nifty", + }, + } + stringComposedMap = map[string]interface{}{ + "foo": "bar", + "baz": []string{ + "nifty", + "stuff", + }, + } + testMap = MergeMaps(stringCommonMap, stringOverrideMap) + equal = reflect.DeepEqual(testMap, stringComposedMap) + if !equal { + t.Errorf("Expected nil on maps of composed lists to override values. Expected values: %v, got %v", stringComposedMap, testMap) + } + + stringOverrideMap = map[string]interface{}{ + "foo": "bar", + "baz": []string{ + "things", + }, + "\\*baz": []string{ + "stuff", + }, + } + stringComposedMap = map[string]interface{}{ + "foo": "bar", + "baz": []string{ + "things", + }, + } + testMap = MergeMaps(stringCommonMap, stringOverrideMap) + equal = reflect.DeepEqual(testMap, stringComposedMap) + if !equal { + t.Errorf("Expected composed lists to default to old behavior in case of conflict. Expected values: %v, got %v", stringComposedMap, testMap) + } + + indexedCommonMap := map[string]interface{}{ + "foo": []map[string]interface{}{ + { + "name": "bar", + "qualities": []string{ + "cool", + }, + }, + { + "name": "baz", + "qualities": []string{ + "stuff", + }, + }, + }, + } + indexedOverrideMap := map[string]interface{}{ + "\\*foo": []map[string]interface{}{ + { + "name": "bar", + "qualities": []string{ + "nifty", + }, + }, + { + "name": "bash", + "qualities": []string{ + "things", + }, + }, + }, + } + normalOverrideMap := map[string]interface{}{ + "foo": []map[string]interface{}{ + { + "\\*name": "bar", + "qualities": []string{ + "nifty", + }, + }, + { + "name": "bash", + "qualities": []string{ + "things", + }, + }, + }, + } + indexedMergedMap := map[string]interface{}{ + "foo": []map[string]interface{}{ + { + "name": "bar", + "qualities": []string{ + "cool", + }, + }, + { + "name": "baz", + "qualities": []string{ + "stuff", + }, + }, + { + "name": "bar", + "qualities": []string{ + "nifty", + }, + }, + { + "name": "bash", + "qualities": []string{ + "things", + }, + }, + }, + } + testMap = MergeMaps(indexedCommonMap, indexedOverrideMap) + equal = reflect.DeepEqual(testMap, indexedMergedMap) + if !equal { + t.Errorf("Expected composed map with duplicate names. Expected values: %v, got %v", indexedMergedMap, testMap) + } + + testMap = MergeMaps(indexedCommonMap, normalOverrideMap) + equal = reflect.DeepEqual(testMap, normalOverrideMap) + if !equal { + t.Errorf("Expected ordinary lists to be fully overriden. Expected values: %v, got %v", indexedOverrideMap, testMap) + } + + indexedOverrideMap = map[string]interface{}{ + "\\*foo": []map[string]interface{}{ + { + "\\*name": "bar", + "qualities": []string{ + "nifty", + }, + }, + { + "name": "bash", + "qualities": []string{ + "things", + }, + }, + }, + } + indexedMergedMap = map[string]interface{}{ + "foo": []map[string]interface{}{ + { + "name": "bar", + "qualities": []string{ + "nifty", + }, + }, + { + "name": "baz", + "qualities": []string{ + "stuff", + }, + }, + { + "name": "bash", + "qualities": []string{ + "things", + }, + }, + }, + } + testMap = MergeMaps(indexedCommonMap, indexedOverrideMap) + equal = reflect.DeepEqual(testMap, indexedMergedMap) + if !equal { + t.Errorf("Expected indexed 'bar' name to be overriden. Expected values: %v, got %v", indexedMergedMap, testMap) + } + +} + func verifyChart(t *testing.T, c *chart.Chart) { t.Helper() if c.Name() == "" { From c223fd3adcffd2bda1cba129e2c0462f5c819638 Mon Sep 17 00:00:00 2001 From: James M Leddy Date: Tue, 23 Sep 2025 14:43:35 -0400 Subject: [PATCH 6/6] feat: Allow deeper merge of lists The initial implementation doesn't consider the type of the index. Instead, we should stipulate that the index is a scalar. Specifically a float or a string, since bool doesn't really make sense for this. Signed-off-by: James M Leddy --- pkg/chart/v2/loader/load.go | 48 ++++++--- pkg/chart/v2/loader/load_test.go | 162 ++++++++++++++++++++++++++++++- 2 files changed, 195 insertions(+), 15 deletions(-) diff --git a/pkg/chart/v2/loader/load.go b/pkg/chart/v2/loader/load.go index 7e80c35d9..6450dcecb 100644 --- a/pkg/chart/v2/loader/load.go +++ b/pkg/chart/v2/loader/load.go @@ -270,11 +270,11 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { if sourceList, ok := out[strippedKey].([]map[string]interface{}); ok { if val, ok := v.([]map[string]interface{}); ok { - out[strippedKey] = MergeMapLists(sourceList, val) + out[strippedKey] = mergeMapLists(sourceList, val) continue } - log.Printf("Property \"%s\" mismatch during merge", strippedKey) + log.Printf("Warning: Property \"%s\" mismatch during merge", strippedKey) continue } else if sourceList, ok := out[strippedKey].([]float64); ok { if val, ok := v.([]float64); ok { @@ -282,7 +282,7 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { continue } - log.Printf("Property \"%s\" mismatch during merge", strippedKey) + log.Printf("Warning: Property \"%s\" mismatch during merge", strippedKey) continue } else if sourceList, ok := out[strippedKey].([]bool); ok { if val, ok := v.([]bool); ok { @@ -290,7 +290,7 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { continue } - log.Printf("Property \"%s\" mismatch during merge", strippedKey) + log.Printf("Warning: Property \"%s\" mismatch during merge", strippedKey) continue } else if sourceList, ok := out[strippedKey].([]string); ok { if val, ok := v.([]string); ok { @@ -298,7 +298,7 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { continue } - log.Printf("Property \"%s\" mismatch during merge", strippedKey) + log.Printf("Warning: Property \"%s\" mismatch during merge", strippedKey) continue } else { out[strippedKey] = v @@ -309,27 +309,47 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { return out } -// MergeMapLists merges two lists of maps. If a prefix of * is set on a map key, +// mergeMapLists merges two lists of maps. If a prefix of * is set on a map key, // that key will be used to de-duplicate/merge with the source map -func MergeMapLists(a, b []map[string]interface{}) []map[string]interface{} { - out := a +func mergeMapLists(a, b []map[string]interface{}) []map[string]interface{} { + out := make([]map[string]interface{}, len(a)) + copy(out, a) +OuterList: for j, mapEntry := range b { var uniqueKey string var dedupValue interface{} + alreadyHasPrefix := false for k, v := range mapEntry { + switch v.(type) { + case float64: + break + case string: + break + default: + continue + } if strings.HasPrefix(k, mergePrefix) { - uniqueKey = k + if alreadyHasPrefix { + duplicateKey := "\\*" + uniqueKey + log.Printf("Warning: Can't index on multiple keys \"%s: %v\" and \"%s: %v\" during list merge", duplicateKey, dedupValue, k, v) + // undo prior step + b[j][duplicateKey] = dedupValue + delete(b[j], uniqueKey) + out = append(out, mapEntry) + continue OuterList + } + + alreadyHasPrefix = true + uniqueKey = strings.TrimPrefix(k, mergePrefix) dedupValue = v - b[j][strings.TrimPrefix(uniqueKey, mergePrefix)] = v - delete(b[j], uniqueKey) + b[j][uniqueKey] = v + delete(b[j], k) } } if len(uniqueKey) > 0 { - strippedMergeKey := strings.TrimPrefix(uniqueKey, mergePrefix) - for i, sourceMapEntry := range out { for k, v := range sourceMapEntry { - if (k == strippedMergeKey || k == uniqueKey) && v == dedupValue { + if k == uniqueKey && v == dedupValue { mergedMapEntry := MergeMaps(sourceMapEntry, mapEntry) out[i] = mergedMapEntry break diff --git a/pkg/chart/v2/loader/load_test.go b/pkg/chart/v2/loader/load_test.go index 3ce9347fb..864fadfdc 100644 --- a/pkg/chart/v2/loader/load_test.go +++ b/pkg/chart/v2/loader/load_test.go @@ -871,6 +871,167 @@ func TestMergeLists(t *testing.T) { } +func TestMergeListsDeep(t *testing.T) { + indexedCommonMap := map[string]interface{}{ + "groups": []map[string]interface{}{ + { + "name": "commonRules", + "id": float64(1331), + "rules": []map[string]interface{}{ + { + "name": "firstRule", + "expr": "commonFirstExpr", + "for": "3m", + }, + }, + }, + }, + } + indexedOverrideMap := map[string]interface{}{ + "\\*groups": []map[string]interface{}{ + { + "\\*name": "commonRules", + "\\*rules": []map[string]interface{}{ + { + "\\*name": "firstRule", + "for": "4m", + }, + { + "name": "uncommonRule", + "expr": "secondExpr", + }, + }, + }, + { + "name": "appRules", + "rules": []map[string]interface{}{ + { + "name": "appRule", + "expr": "appExpr", + "for": "4m", + }, + }, + }, + }, + } + indexedMergedMap := map[string]interface{}{ + "groups": []map[string]interface{}{ + { + "name": "commonRules", + "id": float64(1331), + "rules": []map[string]interface{}{ + { + "name": "firstRule", + "expr": "commonFirstExpr", + "for": "4m", + }, + { + "name": "uncommonRule", + "expr": "secondExpr", + }, + }, + }, + { + "name": "appRules", + "rules": []map[string]interface{}{ + { + "name": "appRule", + "expr": "appExpr", + "for": "4m", + }, + }, + }, + }, + } + testMap := MergeMaps(indexedCommonMap, indexedOverrideMap) + equal := reflect.DeepEqual(testMap, indexedMergedMap) + if !equal { + t.Errorf("Expected two level deep merged list. Expected values: %v, got %v", indexedMergedMap, testMap) + } + + indexedOverrideMap = map[string]interface{}{ + "\\*groups": []map[string]interface{}{ + { + "\\*id": float64(1331), + "\\*rules": []map[string]interface{}{ + { + "\\*name": "firstRule", + "for": "4m", + }, + { + "name": "uncommonRule", + "expr": "secondExpr", + }, + }, + }, + }, + } + indexedMergedMap = map[string]interface{}{ + "groups": []map[string]interface{}{ + { + "name": "commonRules", + "id": float64(1331), + "rules": []map[string]interface{}{ + { + "name": "firstRule", + "expr": "commonFirstExpr", + "for": "4m", + }, + { + "name": "uncommonRule", + "expr": "secondExpr", + }, + }, + }, + }, + } + testMap = MergeMaps(indexedCommonMap, indexedOverrideMap) + equal = reflect.DeepEqual(testMap, indexedMergedMap) + if !equal { + t.Errorf("Expected deep merged list on float64 key. Expected values: %v, got %v", indexedMergedMap, testMap) + } + + indexedOverrideMap = map[string]interface{}{ + "\\*groups": []map[string]interface{}{ + { + "\\*name": "commonRules", + "\\*rules": []map[string]interface{}{ + { + "\\*name": "firstRule", + "\\*for": "3m", + "expr": "OverrideFirstExpr", + }, + }, + }, + }, + } + indexedMergedMap = map[string]interface{}{ + "groups": []map[string]interface{}{ + { + "name": "commonRules", + "id": float64(1331), + "rules": []map[string]interface{}{ + { + "name": "firstRule", + "expr": "commonFirstExpr", + "for": "3m", + }, + { + "\\*name": "firstRule", + "\\*for": "3m", + "expr": "OverrideFirstExpr", + }, + }, + }, + }, + } + testMap = MergeMaps(indexedCommonMap, indexedOverrideMap) + equal = reflect.DeepEqual(testMap, indexedMergedMap) + if !equal { + t.Errorf("Expected composed list where we used two erroneously indicies. Expected values: %v, got %v", indexedMergedMap, testMap) + } +} + func verifyChart(t *testing.T, c *chart.Chart) { t.Helper() if c.Name() == "" { @@ -917,7 +1078,6 @@ func verifyChart(t *testing.T, c *chart.Chart) { t.Errorf("Expected %s version %s, got %s", dep.Name(), exp["version"], dep.Metadata.Version) } } - } func verifyDependencies(t *testing.T, c *chart.Chart) {