Add test for appendUniqueArgs and added logic for ExtraRepoCommandArgs

argoproj-labs · Jan 8, 2025 · d587ba6 · d587ba6
1 parent 33b914b
commit d587ba6
Show file tree

Hide file tree

Showing 4 changed files with 121 additions and 59 deletions.
diff --git a/controllers/argocd/deployment.go b/controllers/argocd/deployment.go
@@ -256,12 +256,8 @@ func getArgoRepoCommand(cr *argoproj.ArgoCD, useTLSForRedis bool) []string {
 	// *** NOTE ***
 	// Do Not add any new default command line arguments below this.
 	extraArgs := cr.Spec.Repo.ExtraRepoCommandArgs
-	err := isMergable(extraArgs, cmd)
-	if err != nil {
-		return cmd
-	}
+	cmd = appendUniqueArgs(cmd, extraArgs)
 
-	cmd = append(cmd, extraArgs...)
 	return cmd
 }
 
@@ -343,59 +339,6 @@ func isMergable(extraArgs []string, cmd []string) error {
 	return nil
 }
 
-// appendUniqueArgs appends extraArgs to cmd while ignoring any duplicate flags.
-func appendUniqueArgs(cmd []string, extraArgs []string) []string {
-	// Parse cmd into a map to track flags and their indices
-	existingArgs := make(map[string]int) // Map to track index of each flag in cmd
-	for i := 0; i < len(cmd); i++ {
-		arg := cmd[i]
-		if strings.HasPrefix(arg, "--") {
-			// Check if the next item is a value (not another flag)
-			if i+1 < len(cmd) && !strings.HasPrefix(cmd[i+1], "--") {
-				existingArgs[arg] = i
-				i++ // Skip the value
-			} else {
-				existingArgs[arg] = i
-			}
-		}
-	}
-
-	// Iterate over extraArgs to append or override existing flags
-	for i := 0; i < len(extraArgs); i++ {
-		arg := extraArgs[i]
-		if strings.HasPrefix(arg, "--") {
-			if index, exists := existingArgs[arg]; exists {
-				// If the flag exists, check if we need to override its value
-				if i+1 < len(extraArgs) && !strings.HasPrefix(extraArgs[i+1], "--") {
-					// If the flag has a value in extraArgs,update it in cmd
-					if index+1 < len(cmd) && !strings.HasPrefix(cmd[index+1], "--") {
-						cmd[index+1] = extraArgs[i+1] // Override the existing value
-					} else {
-						// Insert the new value if it didn't previously have one
-						cmd = append(cmd[:index+1], append([]string{extraArgs[i+1]}, cmd[index+1:]...)...)
-					}
-					i++ // Skip the value in extraArgs
-				}
-			} else {
-				// Append the new flag and its value if not present
-				cmd = append(cmd, arg)
-				if i+1 < len(extraArgs) && !strings.HasPrefix(extraArgs[i+1], "--") {
-					cmd = append(cmd, extraArgs[i+1])
-					existingArgs[arg] = len(cmd) - 2 // Update index tracking
-					i++                              // Skip the value
-				} else {
-					existingArgs[arg] = len(cmd) - 1 // Update index tracking
-				}
-			}
-		} else {
-			// Append non-flag arguments directly
-			cmd = append(cmd, arg)
-		}
-	}
-
-	return cmd
-}
-
 // getDexServerAddress will return the Dex server address.
 func getDexServerAddress(cr *argoproj.ArgoCD) string {
 	return fmt.Sprintf("https://%s", fqdnServiceRef("dex-server", common.ArgoCDDefaultDexHTTPPort, cr))

diff --git a/controllers/argocd/deployment_test.go b/controllers/argocd/deployment_test.go
@@ -2226,6 +2226,17 @@ func TestArgoCDRepoServerDeploymentCommand(t *testing.T) {
 		"foo.scv.cluster.local:6379",
 	}
 
+	wantCmd := []string{
+		"uid_entrypoint.sh",
+		"argocd-repo-server",
+		"--redis",
+		"foo.scv.cluster.local:6379",
+		"--loglevel",
+		"info",
+		"--logformat",
+		"text",
+	}
+
 	assert.NoError(t, r.reconcileRepoDeployment(a, false))
 	assert.NoError(t, r.Client.Get(
 		context.TODO(),
@@ -2235,7 +2246,7 @@ func TestArgoCDRepoServerDeploymentCommand(t *testing.T) {
 		},
 		deployment))
 
-	assert.Equal(t, baseCommand, deployment.Spec.Template.Spec.Containers[0].Command)
+	assert.Equal(t, wantCmd, deployment.Spec.Template.Spec.Containers[0].Command)
 
 	// Remove all the command arguments that were added.
 	a.Spec.Repo.ExtraRepoCommandArgs = []string{}

diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go
@@ -1659,3 +1659,56 @@ func getApplicationSetHTTPServerHost(cr *argoproj.ArgoCD) (string, error) {
 	}
 	return host, nil
 }
+
+// appendUniqueArgs appends extraArgs to cmd while ignoring any duplicate flags.
+func appendUniqueArgs(cmd []string, extraArgs []string) []string {
+	// Parse cmd into a map to track flags and their indices
+	existingArgs := make(map[string]int) // Map to track index of each flag in cmd
+	for i := 0; i < len(cmd); i++ {
+		arg := cmd[i]
+		if strings.HasPrefix(arg, "--") {
+			// Check if the next item is a value (not another flag)
+			if i+1 < len(cmd) && !strings.HasPrefix(cmd[i+1], "--") {
+				existingArgs[arg] = i
+				i++ // Skip the value
+			} else {
+				existingArgs[arg] = i
+			}
+		}
+	}
+
+	// Iterate over extraArgs to append or override existing flags
+	for i := 0; i < len(extraArgs); i++ {
+		arg := extraArgs[i]
+		if strings.HasPrefix(arg, "--") {
+			if index, exists := existingArgs[arg]; exists {
+				// If the flag exists, check if we need to override its value
+				if i+1 < len(extraArgs) && !strings.HasPrefix(extraArgs[i+1], "--") {
+					// If the flag has a value in extraArgs,update it in cmd
+					if index+1 < len(cmd) && !strings.HasPrefix(cmd[index+1], "--") {
+						cmd[index+1] = extraArgs[i+1] // Override the existing value
+					} else {
+						// Insert the new value if it didn't previously have one
+						cmd = append(cmd[:index+1], append([]string{extraArgs[i+1]}, cmd[index+1:]...)...)
+					}
+					i++ // Skip the value in extraArgs
+				}
+			} else {
+				// Append the new flag and its value if not present
+				cmd = append(cmd, arg)
+				if i+1 < len(extraArgs) && !strings.HasPrefix(extraArgs[i+1], "--") {
+					cmd = append(cmd, extraArgs[i+1])
+					existingArgs[arg] = len(cmd) - 2 // Update index tracking
+					i++                              // Skip the value
+				} else {
+					existingArgs[arg] = len(cmd) - 1 // Update index tracking
+				}
+			}
+		} else {
+			// Append non-flag arguments directly
+			cmd = append(cmd, arg)
+		}
+	}
+
+	return cmd
+}
diff --git a/controllers/argocd/util_test.go b/controllers/argocd/util_test.go
@@ -1099,3 +1099,58 @@ func TestReconcileArgoCD_reconcileDexOAuthClientSecret(t *testing.T) {
 	}
 	assert.True(t, tokenExists, "Dex is enabled but unable to create oauth client secret")
 }
+
+func TestAppendUniqueArgs(t *testing.T) {
+	tests := []struct {
+		name      string
+		cmd       []string
+		extraArgs []string
+		want      []string
+	}{
+		{
+			name:      "append new flags and values",
+			cmd:       []string{"--foo", "bar"},
+			extraArgs: []string{"--baz", "qux"},
+			want:      []string{"--foo", "bar", "--baz", "qux"},
+		},
+		{
+			name:      "override existing flag value",
+			cmd:       []string{"--foo", "bar"},
+			extraArgs: []string{"--foo", "baz"},
+			want:      []string{"--foo", "baz"},
+		},
+		{
+			name:      "add flag without value",
+			cmd:       []string{"--foo", "bar"},
+			extraArgs: []string{"--baz"},
+			want:      []string{"--foo", "bar", "--baz"},
+		},
+		{
+			name:      "override flag with no value to have a value",
+			cmd:       []string{"--foo"},
+			extraArgs: []string{"--foo", "baz"},
+			want:      []string{"--foo", "baz"},
+		},
+		{
+			name:      "append non-flag arguments",
+			cmd:       []string{"--foo", "bar"},
+			extraArgs: []string{"extra", "--baz", "qux"},
+			want:      []string{"--foo", "bar", "extra", "--baz", "qux"},
+		},
+		{
+			name:      "ignore duplicate non-flag arguments",
+			cmd:       []string{"arg1", "arg2"},
+			extraArgs: []string{"arg2", "arg3"},
+			want:      []string{"arg1", "arg2", "arg2", "arg3"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := appendUniqueArgs(tt.cmd, tt.extraArgs)
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("appendUniqueArgs() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}