Use mergo and add more tests

Signed-off-by: Nemanja Zeljkovic <nocturo@gmail.com>

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]
+			appendMap[baseKey] = value
-			if exists {
+		} else {
-				if isSlice(srcValue) && isSlice(destValue) {
+			regularMap[key] = value
-					destSlice := destValue.([]any)
-					srcSlice := srcValue.([]any)
-					dest[baseKey] = append(destSlice, srcSlice...)
-				} else {
-					dest[baseKey] = srcValue
-				}
-			} else {
-				dest[baseKey] = srcValue
-			}
-			delete(src, key)
 		}
 	}
 
-	for key, srcValue := range src {
+	if len(appendMap) > 0 {
-		if isMap(srcValue) {
+		for baseKey, appendValue := range appendMap {
-			srcMap := srcValue.(map[string]any)
+			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, "+")
 }

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

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

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" 

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

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

test/integration/test-cases/key_append/output.yaml

@@ -0,0 +1,7 @@
+- action: labeldrop
+  regex: info_.*
+- action: drop
+  regex: container_network_.*
+  sourceLabels:
+    - namespace
+    - __name__

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__

test/integration/test-cases/key_append/values.yaml.gotmpl

@@ -0,0 +1,6 @@
+kube-state-metrics:
+  prometheus:
+    monitor:
+      metricRelabelings:
+        - action: labeldrop
+          regex: "info_.*"