From 4f21adef41a9dfad16ac6c1d7d7f53bc67e8a79d Mon Sep 17 00:00:00 2001
From: Cheng Fang <cfang@redhat.com>
Date: Fri, 18 Oct 2024 02:25:32 -0400
Subject: [PATCH] fix: Git write back to helm values is incorrect during the
 first pass and corrupts existing data (#885)

Signed-off-by: Cheng Fang <cfang@redhat.com>
---
 pkg/argocd/update.go      | 10 +++++++++-
 pkg/argocd/update_test.go | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/pkg/argocd/update.go b/pkg/argocd/update.go
index d239514b..723d6978 100644
--- a/pkg/argocd/update.go
+++ b/pkg/argocd/update.go
@@ -559,7 +559,7 @@ func setHelmValue(currentValues *yaml.MapSlice, key string, value interface{}) e
 	keys := strings.Split(key, ".")
 	current := currentValues
 	var parent *yaml.MapSlice
-	var parentIdx int
+	parentIdx := -1
 
 	for i, k := range keys {
 		if idx, found := findHelmValuesKey(*current, k); found {
@@ -590,11 +590,19 @@ func setHelmValue(currentValues *yaml.MapSlice, key string, value interface{}) e
 			if parent == nil {
 				*currentValues = newParent
 			} else {
+				// if parentIdx has not been set (parent element is also new), set it to the last element
+				if parentIdx == -1 {
+					parentIdx = len(*parent) - 1
+					if parentIdx < 0 {
+						parentIdx = 0
+					}
+				}
 				(*parent)[parentIdx].Value = newParent
 			}
 
 			parent = &newParent
 			current = &newCurrent
+			parentIdx = -1
 		}
 	}
 
diff --git a/pkg/argocd/update_test.go b/pkg/argocd/update_test.go
index 5d026fc2..416de5d3 100644
--- a/pkg/argocd/update_test.go
+++ b/pkg/argocd/update_test.go
@@ -1500,6 +1500,20 @@ replicas: 1
 		require.NoError(t, err)
 		assert.NotEmpty(t, yaml)
 		assert.Equal(t, strings.TrimSpace(strings.ReplaceAll(expected, "\t", "  ")), strings.TrimSpace(string(yaml)))
+
+		// when image.spec.foo fields are missing in the target helm value file,
+		// they should be auto created without corrupting any other pre-existing elements.
+		originalData = []byte("test-value1: one")
+		expected = `
+test-value1: one
+image:
+  spec:
+    foo: nginx:v1.0.0
+`
+		yaml, err = marshalParamsOverride(&app, originalData)
+		require.NoError(t, err)
+		assert.NotEmpty(t, yaml)
+		assert.Equal(t, strings.TrimSpace(strings.ReplaceAll(expected, "\t", "  ")), strings.TrimSpace(string(yaml)))
 	})
 
 	t.Run("Valid Helm source with Helm values file with multiple images", func(t *testing.T) {
@@ -1588,6 +1602,25 @@ replicas: 1
 		require.NoError(t, err)
 		assert.NotEmpty(t, yaml)
 		assert.Equal(t, strings.TrimSpace(strings.ReplaceAll(expected, "\t", "  ")), strings.TrimSpace(string(yaml)))
+
+		// when nginx.* and redis.* fields are missing in the target helm value file,
+		// they should be auto created without corrupting any other pre-existing elements.
+		originalData = []byte("test-value1: one")
+		expected = `
+test-value1: one
+nginx:
+  image:
+    tag: v1.0.0
+    name: nginx
+redis:
+  image:
+    tag: v1.0.0
+    name: redis
+`
+		yaml, err = marshalParamsOverride(&app, originalData)
+		require.NoError(t, err)
+		assert.NotEmpty(t, yaml)
+		assert.Equal(t, strings.TrimSpace(strings.ReplaceAll(expected, "\t", "  ")), strings.TrimSpace(string(yaml)))
 	})
 
 	t.Run("Valid Helm source with Helm values file with multiple aliases", func(t *testing.T) {
@@ -1695,6 +1728,7 @@ replicas: 1
 
 	t.Run("Failed to setValue image parameter name", func(t *testing.T) {
 		expected := `
+test-value1: one
 image:
   name: nginx
   tag: v1.0.0
@@ -1743,6 +1777,7 @@ replicas: 1
 		}
 
 		originalData := []byte(`
+test-value1: one
 image:
   name: nginx
 replicas: 1