fix(repo-server): completely clean up Git working directory (#3683) (#…

…13001) * refactor(util/git/client): make runCredentialedCmd() signature coherent with runCmd() one Signed-off-by: Yann Soubeyrand <[email protected]> * fix(repo-server): completely clean up Git working directory In some cases, for example when a Git submodule wasn’t present anymore in a Git revision, the repo-server didn’t completely clean up its Git working directory. Fixes #3683 Signed-off-by: Yann Soubeyrand <[email protected]> --------- Signed-off-by: Yann Soubeyrand <[email protected]>
argoproj · May 27, 2023 · 41e91d5 · 41e91d5
1 parent 4f6f1cc
commit 41e91d5
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 11 deletions.
diff --git a/test/e2e/fixture/app/actions.go b/test/e2e/fixture/app/actions.go
@@ -76,6 +76,12 @@ func (a *Actions) AddTag(name string) *Actions {
 	return a
 }
 
+func (a *Actions) RemoveSubmodule() *Actions {
+	a.context.t.Helper()
+	fixture.RemoveSubmodule()
+	return a
+}
+
 func (a *Actions) CreateFromPartialFile(data string, flags ...string) *Actions {
 	a.context.t.Helper()
 	tmpFile, err := os.CreateTemp("", "")

diff --git a/test/e2e/fixture/fixture.go b/test/e2e/fixture/fixture.go
@@ -855,6 +855,18 @@ func CreateSubmoduleRepos(repoType string) {
 	CheckError(os.Setenv("GIT_ALLOW_PROTOCOL", oldEnv))
 }
 
+func RemoveSubmodule() {
+	log.Info("removing submodule")
+
+	FailOnErr(Run(submoduleParentDirectory(), "git", "rm", "submodule/test"))
+	FailOnErr(Run(submoduleParentDirectory(), "touch", "submodule/.gitkeep"))
+	FailOnErr(Run(submoduleParentDirectory(), "git", "add", "submodule/.gitkeep"))
+	FailOnErr(Run(submoduleParentDirectory(), "git", "commit", "-m", "remove submodule"))
+	if IsRemote() {
+		FailOnErr(Run(submoduleParentDirectory(), "git", "push", "-f", "origin", "master"))
+	}
+}
+
 // RestartRepoServer performs a restart of the repo server deployment and waits
 // until the rollout has completed.
 func RestartRepoServer() {

diff --git a/test/e2e/git_submodule_test.go b/test/e2e/git_submodule_test.go
@@ -40,3 +40,25 @@ func TestGitSubmoduleHTTPSSupport(t *testing.T) {
 		Expect(SyncStatusIs(SyncStatusCodeSynced)).
 		Expect(Pod(func(p v1.Pod) bool { return p.Name == "pod-in-submodule" }))
 }
+
+func TestGitSubmoduleRemovalSupport(t *testing.T) {
+	Given(t).
+		RepoURLType(fixture.RepoURLTypeSSHSubmoduleParent).
+		Path("submodule").
+		Recurse().
+		CustomSSHKnownHostsAdded().
+		SubmoduleSSHRepoURLAdded(true).
+		When().
+		CreateFromFile(func(app *Application) {}).
+		Sync().
+		Then().
+		Expect(SyncStatusIs(SyncStatusCodeSynced)).
+		Expect(Pod(func(p v1.Pod) bool { return p.Name == "pod-in-submodule" })).
+		When().
+		RemoveSubmodule().
+		Refresh(RefreshTypeNormal).
+		Sync().
+		Then().
+		Expect(SyncStatusIs(SyncStatusCodeSynced)).
+		Expect(NotPod(func(p v1.Pod) bool { return p.Name == "pod-in-submodule" }))
+}
diff --git a/test/e2e/testdata/git-submodule/submodule-pod.yaml b/test/e2e/testdata/git-submodule/submodule-pod.yaml
@@ -7,6 +7,4 @@ spec:
     - name: main
       image: quay.io/argoprojlabs/argocd-e2e-container:0.1
       imagePullPolicy: IfNotPresent
-      command:
-        - "true"
-  restartPolicy: Never
+  terminationGracePeriodSeconds: 0
diff --git a/util/git/client.go b/util/git/client.go
@@ -334,9 +334,9 @@ func (m *nativeGitClient) IsLFSEnabled() bool {
 func (m *nativeGitClient) fetch(revision string) error {
 	var err error
 	if revision != "" {
-		err = m.runCredentialedCmd("git", "fetch", "origin", revision, "--tags", "--force", "--prune")
+		err = m.runCredentialedCmd("fetch", "origin", revision, "--tags", "--force", "--prune")
 	} else {
-		err = m.runCredentialedCmd("git", "fetch", "origin", "--tags", "--force", "--prune")
+		err = m.runCredentialedCmd("fetch", "origin", "--tags", "--force", "--prune")
 	}
 	return err
 }
@@ -354,7 +354,7 @@ func (m *nativeGitClient) Fetch(revision string) error {
 	if err == nil && m.IsLFSEnabled() {
 		largeFiles, err := m.LsLargeFiles()
 		if err == nil && len(largeFiles) > 0 {
-			err = m.runCredentialedCmd("git", "lfs", "fetch", "--all")
+			err = m.runCredentialedCmd("lfs", "fetch", "--all")
 			if err != nil {
 				return err
 			}
@@ -387,10 +387,10 @@ func (m *nativeGitClient) LsLargeFiles() ([]string, error) {
 
 // Submodule embed other repositories into this repository
 func (m *nativeGitClient) Submodule() error {
-	if err := m.runCredentialedCmd("git", "submodule", "sync", "--recursive"); err != nil {
+	if err := m.runCredentialedCmd("submodule", "sync", "--recursive"); err != nil {
 		return err
 	}
-	if err := m.runCredentialedCmd("git", "submodule", "update", "--init", "--recursive"); err != nil {
+	if err := m.runCredentialedCmd("submodule", "update", "--init", "--recursive"); err != nil {
 		return err
 	}
 	return nil
@@ -424,7 +424,12 @@ func (m *nativeGitClient) Checkout(revision string, submoduleEnabled bool) error
 			}
 		}
 	}
-	if _, err := m.runCmd("clean", "-fdx"); err != nil {
+	// NOTE
+	// The double “f” in the arguments is not a typo: the first “f” tells
+	// `git clean` to delete untracked files and directories, and the second “f”
+	// tells it to clean untractked nested Git repositories (for example a
+	// submodule which has since been removed).
+	if _, err := m.runCmd("clean", "-ffdx"); err != nil {
 		return err
 	}
 	return nil
@@ -632,7 +637,7 @@ func (m *nativeGitClient) runCmd(args ...string) (string, error) {
 
 // runCredentialedCmd is a convenience function to run a git command with username/password credentials
 // nolint:unparam
-func (m *nativeGitClient) runCredentialedCmd(command string, args ...string) error {
+func (m *nativeGitClient) runCredentialedCmd(args ...string) error {
 	closer, environ, err := m.creds.Environ()
 	if err != nil {
 		return err
@@ -647,7 +652,7 @@ func (m *nativeGitClient) runCredentialedCmd(command string, args ...string) err
 		}
 	}
 
-	cmd := exec.Command(command, args...)
+	cmd := exec.Command("git", args...)
 	cmd.Env = append(cmd.Env, environ...)
 	_, err = m.runCmdOutput(cmd)
 	return err