simplified validation logic and added e2e tests

Signed-off-by: reggie-k <[email protected]>

cmd/argocd/commands/admin/repo.go

@@ -141,8 +141,12 @@ func NewGenRepoSpecCommand() *cobra.Command {
 				repoOpts.Repo.Password = cli.PromptPassword(repoOpts.Repo.Password)
 			}
 
-			cmdutil.ValidateBearerTokenAndPasswordCombo(repoOpts.Repo.BearerToken, repoOpts.Repo.Password)
-			cmdutil.ValidateBearerTokenForGitOnly(repoOpts.Repo.BearerToken, repoOpts.Repo.Type)
+			err := cmdutil.ValidateBearerTokenAndPasswordCombo(repoOpts.Repo.BearerToken, repoOpts.Repo.Password)
+			errors.CheckError(err)
+			err = cmdutil.ValidateBearerTokenForHTTPSRepoOnly(repoOpts.Repo.BearerToken, git.IsHTTPSURL(repoOpts.Repo.Repo))
+			errors.CheckError(err)
+			err = cmdutil.ValidateBearerTokenForGitOnly(repoOpts.Repo.BearerToken, repoOpts.Repo.Type)
+			errors.CheckError(err)
 
 			argoCDCM := &corev1.ConfigMap{
 				TypeMeta: metav1.TypeMeta{
@@ -161,7 +165,7 @@ func NewGenRepoSpecCommand() *cobra.Command {
 			settingsMgr := settings.NewSettingsManager(ctx, kubeClientset, ArgoCDNamespace)
 			argoDB := db.NewDB(ArgoCDNamespace, settingsMgr, kubeClientset)
 
-			_, err := argoDB.CreateRepository(ctx, &repoOpts.Repo)
+			_, err = argoDB.CreateRepository(ctx, &repoOpts.Repo)
 			errors.CheckError(err)
 
 			secret, err := kubeClientset.CoreV1().Secrets(ArgoCDNamespace).Get(ctx, db.RepoURLToSecretName(repoSecretPrefix, repoOpts.Repo.Repo, repoOpts.Repo.Project), metav1.GetOptions{})

cmd/argocd/commands/repo.go

@@ -197,8 +197,12 @@ func NewRepoAddCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command {
 				repoOpts.Repo.Password = cli.PromptPassword(repoOpts.Repo.Password)
 			}
 
-			cmdutil.ValidateBearerTokenAndPasswordCombo(repoOpts.Repo.BearerToken, repoOpts.Repo.Password)
-			cmdutil.ValidateBearerTokenForGitOnly(repoOpts.Repo.BearerToken, repoOpts.Repo.Type)
+			err := cmdutil.ValidateBearerTokenAndPasswordCombo(repoOpts.Repo.BearerToken, repoOpts.Repo.Password)
+			errors.CheckError(err)
+			err = cmdutil.ValidateBearerTokenForGitOnly(repoOpts.Repo.BearerToken, repoOpts.Repo.Type)
+			errors.CheckError(err)
+			err = cmdutil.ValidateBearerTokenForHTTPSRepoOnly(repoOpts.Repo.BearerToken, git.IsHTTPSURL(repoOpts.Repo.Repo))
+			errors.CheckError(err)
 
 			// We let the server check access to the repository before adding it. If
 			// it is a private repo, but we cannot access with with the credentials
@@ -229,7 +233,7 @@ func NewRepoAddCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command {
 				ForceHttpBasicAuth:         repoOpts.Repo.ForceHttpBasicAuth,
 				UseAzureWorkloadIdentity:   repoOpts.Repo.UseAzureWorkloadIdentity,
 			}
-			_, err := repoIf.ValidateAccess(ctx, &repoAccessReq)
+			_, err = repoIf.ValidateAccess(ctx, &repoAccessReq)
 			errors.CheckError(err)
 
 			repoCreateReq := repositorypkg.RepoCreateRequest{

cmd/argocd/commands/repocreds.go

@@ -160,17 +160,18 @@ func NewRepoCredsAddCommand(clientOpts *argocdclient.ClientOptions) *cobra.Comma
 			conn, repoIf := headless.NewClientOrDie(clientOpts, c).NewRepoCredsClientOrDie()
 			defer io.Close(conn)
 
-			// Specifying bearerToken is only valid for HTTPS repositories
-			cmdutil.ValidateBearerTokenForHTTPSRepoOnly(repo.BearerToken, git.IsHTTPSURL(repo.URL))
-
 			// If the user set a username, but didn't supply password via --password,
 			// then we prompt for it
 			if repo.Username != "" && repo.Password == "" {
 				repo.Password = cli.PromptPassword(repo.Password)
 			}
 
-			cmdutil.ValidateBearerTokenAndPasswordCombo(repo.BearerToken, repo.Password)
-			cmdutil.ValidateBearerTokenForGitOnly(repo.BearerToken, repo.Type)
+			err := cmdutil.ValidateBearerTokenAndPasswordCombo(repo.BearerToken, repo.Password)
+			errors.CheckError(err)
+			err = cmdutil.ValidateBearerTokenForGitOnly(repo.BearerToken, repo.Type)
+			errors.CheckError(err)
+			err = cmdutil.ValidateBearerTokenForHTTPSRepoOnly(repo.BearerToken, git.IsHTTPSURL(repo.URL))
+			errors.CheckError(err)
 
 			repoCreateReq := repocredspkg.RepoCredsCreateRequest{
 				Creds:  &repo,

cmd/util/common.go

@@ -2,37 +2,38 @@ package util
 
 import (
 	stderrors "errors"
-
-	"github.com/argoproj/argo-cd/v3/util/errors"
 )
 
 var (
 	LogFormat string
 	LogLevel  string
 )
 
-func ValidateBearerTokenForHTTPSRepoOnly(bearerToken string, isHTTPS bool) {
+func ValidateBearerTokenForHTTPSRepoOnly(bearerToken string, isHTTPS bool) error {
 	// Bearer token is only valid for HTTPS repositories
 	if bearerToken != "" {
 		if !isHTTPS {
 			err := stderrors.New("--bearer-token is only supported for HTTPS repositories")
-			errors.CheckError(err)
+			return err
 		}
 	}
+	return nil
 }
 
-func ValidateBearerTokenForGitOnly(bearerToken string, repoType string) {
+func ValidateBearerTokenForGitOnly(bearerToken string, repoType string) error {
 	// Bearer token is only valid for Git repositories
 	if bearerToken != "" && repoType != "git" {
 		err := stderrors.New("--bearer-token is only supported for Git repositories")
-		errors.CheckError(err)
+		return err
 	}
+	return nil
 }
 
-func ValidateBearerTokenAndPasswordCombo(bearerToken string, password string) {
+func ValidateBearerTokenAndPasswordCombo(bearerToken string, password string) error {
 	// Either the password or the bearer token must be set, but not both
 	if bearerToken != "" && password != "" {
 		err := stderrors.New("only --bearer-token or --password is allowed, not both")
-		errors.CheckError(err)
+		return err
 	}
+	return nil
 }

cmd/util/common_test.go

@@ -1,13 +1,9 @@
 package util
 
 import (
-	"bytes"
-	"errors"
-	"os"
-	"os/exec"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestValidateBearerTokenAndPasswordCombo(t *testing.T) {
@@ -47,10 +43,12 @@ func TestValidateBearerTokenAndPasswordCombo(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			// if tt.expectError {
-			runCmdAndCheckError(t, tt.expectError, "TestValidateBearerTokenAndPasswordCombo", tt.errorMsg, func() {
-				ValidateBearerTokenAndPasswordCombo(tt.bearerToken, tt.password)
-			})
+			err := ValidateBearerTokenAndPasswordCombo(tt.bearerToken, tt.password)
+			if tt.expectError {
+				require.ErrorContains(t, err, tt.errorMsg)
+			} else {
+				require.NoError(t, err)
+			}
 		})
 	}
 }
@@ -99,9 +97,12 @@ func TestValidateBearerTokenForGitOnly(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			runCmdAndCheckError(t, tt.expectError, "TestValidateBearerTokenForGitOnly", tt.errorMsg, func() {
-				ValidateBearerTokenForGitOnly(tt.bearerToken, tt.repoType)
-			})
+			err := ValidateBearerTokenForGitOnly(tt.bearerToken, tt.repoType)
+			if tt.expectError {
+				require.ErrorContains(t, err, tt.errorMsg)
+			} else {
+				require.NoError(t, err)
+			}
 		})
 	}
 }
@@ -143,38 +144,12 @@ func TestValidateBearerTokenForHTTPSRepoOnly(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			runCmdAndCheckError(t, tt.expectError, "TestValidateBearerTokenForHTTPSRepoOnly", tt.errorMsg, func() {
-				ValidateBearerTokenForHTTPSRepoOnly(tt.bearerToken, tt.isHTTPS)
-			})
+			err := ValidateBearerTokenForHTTPSRepoOnly(tt.bearerToken, tt.isHTTPS)
+			if tt.expectError {
+				require.ErrorContains(t, err, tt.errorMsg)
+			} else {
+				require.NoError(t, err)
+			}
 		})
 	}
 }
-
-func runCmdAndCheckError(t *testing.T, expectError bool, testName, errorMsg string, validationFunc func()) {
-	t.Helper()
-	// All CLI commands do not return an error upon failure.
-	// Instead, the errors.CheckError(err) in each CLI command performs non-zero code system exit.
-	// So in order to test the commands, we need to run the command in a separate process and capture it's error message.
-	// https://stackoverflow.com/a/33404435
-	// TODO: consider whether to change all the CLI commands to return an error instead of performing a non-zero code system exit.
-	if expectError {
-		if os.Getenv("BE_CRASHER") == "1" {
-			validationFunc()
-			return
-		}
-		cmd := exec.Command(os.Args[0], "-test.run="+testName)
-		cmd.Env = append(os.Environ(), "BE_CRASHER=1")
-		var stderr bytes.Buffer
-		cmd.Stderr = &stderr
-		err := cmd.Run()
-		var exitErr *exec.ExitError
-		if errors.As(err, &exitErr) && !exitErr.Success() {
-			t.Log(stderr.String())
-			assert.Contains(t, stderr.String(), errorMsg)
-			return
-		}
-		t.Fatalf("process ran with err %v, want exit status 1", err)
-	} else {
-		validationFunc()
-	}
-}

test/e2e/repo_management_test.go

@@ -177,3 +177,19 @@ func TestAddHelmRepoInsecureSkipVerify(t *testing.T) {
 		assert.True(t, exists)
 	})
 }
+
+func TestFailOnPrivateRepoCreationWithPasswordAndBearerToken(t *testing.T) {
+	app.Given(t).And(func() {
+		repoUrl := fixture.RepoURL(fixture.RepoURLTypeFile)
+		_, err := fixture.RunCli("repo", "add", repoUrl, "--password", "test", "--bearer-token", "test")
+		require.ErrorContains(t, err, "only --bearer-token or --password is allowed, not both")
+	})
+}
+
+func TestCreatePrivateRepoWithBearerToken(t *testing.T) {
+	app.Given(t).And(func() {
+		repoUrl := fixture.RepoURL(fixture.RepoURLTypeFile)
+		_, err := fixture.RunCli("repo", "add", repoUrl, "--bearer-token", "test")
+		require.NoError(t, err)
+	})
+}