From 2dfd94fae12cd3d4c8a4afa04df9ff130afe326f Mon Sep 17 00:00:00 2001 From: Nemanja Zeljkovic Date: Wed, 30 Jul 2025 14:22:51 +0200 Subject: [PATCH] Use mergo and add more tests Signed-off-by: Nemanja Zeljkovic --- pkg/yaml/append_processor.go | 83 ++-- pkg/yaml/append_processor_test.go | 364 ++++++++++++++++++ test/integration/run.sh | 1 + test/integration/test-cases/key_append.sh | 21 + .../test-cases/key_append/common.yaml | 8 + .../key_append/helmfile.yaml.gotmpl | 13 + .../test-cases/key_append/output.yaml | 7 + .../key_append/overrides/values.yaml.gotmpl | 9 + .../test-cases/key_append/values.yaml.gotmpl | 6 + 9 files changed, 483 insertions(+), 29 deletions(-) create mode 100644 test/integration/test-cases/key_append.sh create mode 100644 test/integration/test-cases/key_append/common.yaml create mode 100644 test/integration/test-cases/key_append/helmfile.yaml.gotmpl create mode 100644 test/integration/test-cases/key_append/output.yaml create mode 100644 test/integration/test-cases/key_append/overrides/values.yaml.gotmpl create mode 100644 test/integration/test-cases/key_append/values.yaml.gotmpl diff --git a/pkg/yaml/append_processor.go b/pkg/yaml/append_processor.go index 792e4e22..4431c840 100644 --- a/pkg/yaml/append_processor.go +++ b/pkg/yaml/append_processor.go @@ -2,6 +2,8 @@ package yaml import ( "strings" + + "dario.cat/mergo" ) type AppendProcessor struct{} @@ -14,40 +16,73 @@ func (ap *AppendProcessor) MergeWithAppend(dest, src map[string]any) error { convertToStringMapInPlace(dest) convertToStringMapInPlace(src) - for key, srcValue := range src { + appendMap := make(map[string]any) + regularMap := make(map[string]any) + + for key, value := range src { if IsAppendKey(key) { baseKey := GetBaseKey(key) - destValue, exists := dest[baseKey] - if exists { - if isSlice(srcValue) && isSlice(destValue) { - destSlice := destValue.([]any) - srcSlice := srcValue.([]any) - dest[baseKey] = append(destSlice, srcSlice...) - } else { - dest[baseKey] = srcValue - } - } else { - dest[baseKey] = srcValue - } - delete(src, key) + appendMap[baseKey] = value + } else { + regularMap[key] = value } } - for key, srcValue := range src { - if isMap(srcValue) { - srcMap := srcValue.(map[string]any) + if len(appendMap) > 0 { + for baseKey, appendValue := range appendMap { + destValue, exists := dest[baseKey] + if exists { + if _, ok := destValue.([]any); !ok { + dest[baseKey] = appendValue + delete(appendMap, baseKey) + } + } + } + + if len(appendMap) > 0 { + tempDest := make(map[string]any) + for k, v := range dest { + tempDest[k] = v + } + + if err := mergo.Merge(&tempDest, appendMap, mergo.WithAppendSlice, mergo.WithSliceDeepCopy); err != nil { + return err + } + + for k, v := range tempDest { + dest[k] = v + } + } + } + + for key, value := range regularMap { + if srcMap, ok := value.(map[string]any); ok { if destMap, ok := dest[key].(map[string]any); ok { if err := ap.MergeWithAppend(destMap, srcMap); err != nil { return err } dest[key] = destMap } else { - dest[key] = srcMap + newDestMap := make(map[string]any) + if err := ap.MergeWithAppend(newDestMap, srcMap); err != nil { + return err + } + dest[key] = newDestMap } } else { - dest[key] = srcValue + tempDest := make(map[string]any) + tempDest[key] = dest[key] + tempSrc := make(map[string]any) + tempSrc[key] = value + + if err := mergo.Merge(&tempDest, tempSrc, mergo.WithOverride); err != nil { + return err + } + + dest[key] = tempDest[key] } } + return nil } @@ -76,16 +111,6 @@ func convertToStringMapInPlace(v any) any { } } -func isSlice(value any) bool { - _, ok := value.([]any) - return ok -} - -func isMap(value any) bool { - _, ok := value.(map[string]any) - return ok -} - func IsAppendKey(key string) bool { return strings.HasSuffix(key, "+") } diff --git a/pkg/yaml/append_processor_test.go b/pkg/yaml/append_processor_test.go index cbae8bd4..5a206ebe 100644 --- a/pkg/yaml/append_processor_test.go +++ b/pkg/yaml/append_processor_test.go @@ -100,6 +100,38 @@ func TestAppendProcessor_MergeWithAppend(t *testing.T) { }, }, }, + { + name: "nested append with non-existent parent", + dest: map[string]any{}, + src: map[string]any{ + "kube-state-metrics": map[string]any{ + "prometheus": map[string]any{ + "monitor": map[string]any{ + "metricRelabelings+": []any{ + map[string]any{ + "action": "labeldrop", + "regex": "info_.*", + }, + }, + }, + }, + }, + }, + expected: map[string]any{ + "kube-state-metrics": map[string]any{ + "prometheus": map[string]any{ + "monitor": map[string]any{ + "metricRelabelings": []any{ + map[string]any{ + "action": "labeldrop", + "regex": "info_.*", + }, + }, + }, + }, + }, + }, + }, { name: "overwrite non-slice with append", dest: map[string]any{ @@ -120,17 +152,349 @@ func TestAppendProcessor_MergeWithAppend(t *testing.T) { }, }, }, + { + name: "type collision - []string vs []any", + dest: map[string]any{ + "values": []string{"a", "b"}, + }, + src: map[string]any{ + "values+": []any{"c", "d"}, + }, + expected: map[string]any{ + "values": []any{"c", "d"}, + }, + }, + { + name: "type collision - []int vs []any", + dest: map[string]any{ + "values": []int{1, 2}, + }, + src: map[string]any{ + "values+": []any{3, 4}, + }, + expected: map[string]any{ + "values": []any{3, 4}, + }, + }, + { + name: "append on map - should overwrite", + dest: map[string]any{ + "config": map[string]any{"key": "value"}, + }, + src: map[string]any{ + "config+": []any{"new", "values"}, + }, + expected: map[string]any{ + "config": []any{"new", "values"}, + }, + }, + { + name: "append on scalar - should overwrite", + dest: map[string]any{ + "version": "1.0.0", + }, + src: map[string]any{ + "version+": []any{"2.0.0", "3.0.0"}, + }, + expected: map[string]any{ + "version": []any{"2.0.0", "3.0.0"}, + }, + }, + { + name: "nil slice in destination", + dest: map[string]any{ + "values": nil, + }, + src: map[string]any{ + "values+": []any{"a", "b"}, + }, + expected: map[string]any{ + "values": []any{"a", "b"}, + }, + }, + { + name: "empty slice in destination", + dest: map[string]any{ + "values": []any{}, + }, + src: map[string]any{ + "values+": []any{"a", "b"}, + }, + expected: map[string]any{ + "values": []any{"a", "b"}, + }, + }, + { + name: "nil slice in source", + dest: map[string]any{ + "values": []any{"existing"}, + }, + src: map[string]any{ + "values+": nil, + }, + expected: map[string]any{ + "values": nil, + }, + }, + { + name: "mixed types in slices", + dest: map[string]any{ + "data": []any{"string", 42, true}, + }, + src: map[string]any{ + "data+": []any{"new", 100, false}, + }, + expected: map[string]any{ + "data": []any{"string", 42, true, "new", 100, false}, + }, + }, + { + name: "multiple append keys", + dest: map[string]any{ + "list1": []any{"a"}, + "list2": []any{"x"}, + }, + src: map[string]any{ + "list1+": []any{"b"}, + "list2+": []any{"y"}, + }, + expected: map[string]any{ + "list1": []any{"a", "b"}, + "list2": []any{"x", "y"}, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + srcCopy := make(map[string]any) + for k, v := range tt.src { + srcCopy[k] = v + } + err := processor.MergeWithAppend(tt.dest, tt.src) require.NoError(t, err) assert.Equal(t, tt.expected, tt.dest) + + assert.Equal(t, srcCopy, tt.src, "source map should not be mutated") }) } } +func TestAppendProcessor_EdgeCases(t *testing.T) { + processor := NewAppendProcessor() + + t.Run("empty maps", func(t *testing.T) { + dest := make(map[string]any) + src := make(map[string]any) + + err := processor.MergeWithAppend(dest, src) + require.NoError(t, err) + assert.Empty(t, dest) + }) + + t.Run("nil maps", func(t *testing.T) { + var dest map[string]any + var src map[string]any + + err := processor.MergeWithAppend(dest, src) + require.NoError(t, err) + assert.Nil(t, dest) + }) + + t.Run("deep nested with append", func(t *testing.T) { + dest := map[string]any{ + "level1": map[string]any{ + "level2": map[string]any{ + "level3": map[string]any{ + "values": []any{"deep"}, + }, + }, + }, + } + src := map[string]any{ + "level1": map[string]any{ + "level2": map[string]any{ + "level3": map[string]any{ + "values+": []any{"nested"}, + }, + }, + }, + } + expected := map[string]any{ + "level1": map[string]any{ + "level2": map[string]any{ + "level3": map[string]any{ + "values": []any{"deep", "nested"}, + }, + }, + }, + } + + err := processor.MergeWithAppend(dest, src) + require.NoError(t, err) + assert.Equal(t, expected, dest) + }) + + t.Run("complex nested structure", func(t *testing.T) { + dest := map[string]any{ + "config": map[string]any{ + "services": []any{ + map[string]any{"name": "service1"}, + }, + "settings": map[string]any{ + "timeout": 30, + }, + }, + } + src := map[string]any{ + "config": map[string]any{ + "services+": []any{ + map[string]any{"name": "service2"}, + }, + "settings": map[string]any{ + "retries": 3, + }, + }, + } + expected := map[string]any{ + "config": map[string]any{ + "services": []any{ + map[string]any{"name": "service1"}, + map[string]any{"name": "service2"}, + }, + "settings": map[string]any{ + "timeout": 30, + "retries": 3, + }, + }, + } + + err := processor.MergeWithAppend(dest, src) + require.NoError(t, err) + assert.Equal(t, expected, dest) + }) +} + +func TestAppendProcessor_TypeConversions(t *testing.T) { + processor := NewAppendProcessor() + + t.Run("map[any]any to map[string]any conversion", func(t *testing.T) { + dest := map[string]any{ + "values": []any{"a"}, + } + src := map[any]any{ + "values+": []any{"b"}, + } + + srcConverted := make(map[string]any) + for k, v := range src { + if ks, ok := k.(string); ok { + srcConverted[ks] = v + } + } + + err := processor.MergeWithAppend(dest, srcConverted) + require.NoError(t, err) + assert.Equal(t, []any{"a", "b"}, dest["values"]) + }) + + t.Run("mixed key types", func(t *testing.T) { + dest := map[string]any{ + "values": []any{"a"}, + } + src := map[any]any{ + "values+": []any{"b"}, + "other": "value", + 42: "number_key", + } + + srcConverted := make(map[string]any) + for k, v := range src { + if ks, ok := k.(string); ok { + srcConverted[ks] = v + } + } + + err := processor.MergeWithAppend(dest, srcConverted) + require.NoError(t, err) + assert.Equal(t, []any{"a", "b"}, dest["values"]) + assert.Equal(t, "value", dest["other"]) + assert.NotContains(t, dest, "42") + }) +} + +func TestAppendProcessor_PropertyBased(t *testing.T) { + processor := NewAppendProcessor() + + t.Run("idempotent regular merge", func(t *testing.T) { + dest := map[string]any{ + "key1": "value1", + "key2": []any{"a", "b"}, + } + src := map[string]any{ + "key1": "value1", + "key3": "value3", + } + + err := processor.MergeWithAppend(dest, src) + require.NoError(t, err) + + err = processor.MergeWithAppend(dest, src) + require.NoError(t, err) + + expected := map[string]any{ + "key1": "value1", + "key2": []any{"a", "b"}, + "key3": "value3", + } + assert.Equal(t, expected, dest) + }) + + t.Run("append is not idempotent", func(t *testing.T) { + dest := map[string]any{ + "values": []any{"a"}, + } + src := map[string]any{ + "values+": []any{"b"}, + } + + err := processor.MergeWithAppend(dest, src) + require.NoError(t, err) + assert.Equal(t, []any{"a", "b"}, dest["values"]) + + err = processor.MergeWithAppend(dest, src) + require.NoError(t, err) + assert.Equal(t, []any{"a", "b", "b"}, dest["values"]) + }) + + t.Run("merge is not commutative", func(t *testing.T) { + map1 := map[string]any{"a": 1, "b": 2} + map2 := map[string]any{"b": 3, "c": 4} + + result1 := make(map[string]any) + for k, v := range map1 { + result1[k] = v + } + err := processor.MergeWithAppend(result1, map2) + require.NoError(t, err) + + result2 := make(map[string]any) + for k, v := range map2 { + result2[k] = v + } + err = processor.MergeWithAppend(result2, map1) + require.NoError(t, err) + + assert.NotEqual(t, result1, result2) + + expected1 := map[string]any{"a": 1, "b": 3, "c": 4} + expected2 := map[string]any{"a": 1, "b": 2, "c": 4} + assert.Equal(t, expected1, result1) + assert.Equal(t, expected2, result2) + }) +} + func TestIsAppendKey(t *testing.T) { tests := []struct { key string diff --git a/test/integration/run.sh b/test/integration/run.sh index 1e501553..d7eca928 100755 --- a/test/integration/run.sh +++ b/test/integration/run.sh @@ -103,6 +103,7 @@ ${kubectl} create namespace ${test_ns} || fail "Could not create namespace ${tes . ${dir}/test-cases/issue-1749.sh . ${dir}/test-cases/issue-1893.sh . ${dir}/test-cases/state-values-set-cli-args-in-environments.sh +. ${dir}/test-cases/key_append.sh # ALL DONE ----------------------------------------------------------------------------------------------------------- diff --git a/test/integration/test-cases/key_append.sh b/test/integration/test-cases/key_append.sh new file mode 100644 index 00000000..cd0d92b2 --- /dev/null +++ b/test/integration/test-cases/key_append.sh @@ -0,0 +1,21 @@ +test_start "key append feature" + +key_append_case_input_dir="${cases_dir}/key_append" +key_append_expected_output="${key_append_case_input_dir}/output.yaml" + +key_append_tmp=$(mktemp -d) +key_append_values_file="${key_append_tmp}/key_append.values.yaml" +key_append_generated_metrics_file="${key_append_tmp}/key_append_generated_metrics.yaml" + +info "Testing key append functionality with nested structure" +config_file="helmfile.yaml.gotmpl" + +info "Running helmfile template for key append test" +${helmfile} -f ${key_append_case_input_dir}/${config_file} template > ${key_append_values_file} || fail "\"helmfile template\" shouldn't fail" + +info "Verifying that metricRelabelings+ is properly processed" +yq 'select(.metadata.name=="prometheus-monitoring-kube-state-metrics") | .spec.endpoints[].metricRelabelings' ${key_append_values_file} > ${key_append_generated_metrics_file} +./dyff between -bs ${key_append_expected_output} ${key_append_generated_metrics_file} || fail "\"helmfile template\" should be consistent" +echo code=$? + +test_pass "key append feature" diff --git a/test/integration/test-cases/key_append/common.yaml b/test/integration/test-cases/key_append/common.yaml new file mode 100644 index 00000000..0b2f39af --- /dev/null +++ b/test/integration/test-cases/key_append/common.yaml @@ -0,0 +1,8 @@ +templates: + applications-default: + missingFileHandler: Warn + valuesTemplate: + - values.yaml + - values.yaml.gotmpl + - overrides/values.yaml + - overrides/values.yaml.gotmpl diff --git a/test/integration/test-cases/key_append/helmfile.yaml.gotmpl b/test/integration/test-cases/key_append/helmfile.yaml.gotmpl new file mode 100644 index 00000000..c434d5aa --- /dev/null +++ b/test/integration/test-cases/key_append/helmfile.yaml.gotmpl @@ -0,0 +1,13 @@ +{{ readFile "common.yaml" }} + +repositories: +- name: prometheus-community + url: https://prometheus-community.github.io/helm-charts + +releases: +- name: prometheus-monitoring + chart: prometheus-community/kube-prometheus-stack + namespace: monitoring + forceNamespace: monitoring + inherit: + - template: applications-default diff --git a/test/integration/test-cases/key_append/output.yaml b/test/integration/test-cases/key_append/output.yaml new file mode 100644 index 00000000..f67c8d29 --- /dev/null +++ b/test/integration/test-cases/key_append/output.yaml @@ -0,0 +1,7 @@ +- action: labeldrop + regex: info_.* +- action: drop + regex: container_network_.* + sourceLabels: + - namespace + - __name__ diff --git a/test/integration/test-cases/key_append/overrides/values.yaml.gotmpl b/test/integration/test-cases/key_append/overrides/values.yaml.gotmpl new file mode 100644 index 00000000..60aa16a5 --- /dev/null +++ b/test/integration/test-cases/key_append/overrides/values.yaml.gotmpl @@ -0,0 +1,9 @@ +kube-state-metrics: + prometheus: + monitor: + metricRelabelings+: + - action: drop + regex: "container_network_.*" + sourceLabels: + - namespace + - __name__ diff --git a/test/integration/test-cases/key_append/values.yaml.gotmpl b/test/integration/test-cases/key_append/values.yaml.gotmpl new file mode 100644 index 00000000..463307bc --- /dev/null +++ b/test/integration/test-cases/key_append/values.yaml.gotmpl @@ -0,0 +1,6 @@ +kube-state-metrics: + prometheus: + monitor: + metricRelabelings: + - action: labeldrop + regex: "info_.*"