From e95ac7e5a23b118927d6d9e65ad0b885bf89b128 Mon Sep 17 00:00:00 2001 From: Yann Soubeyrand Date: Sat, 27 May 2023 21:57:59 +0200 Subject: [PATCH] fix(repo-server): completely clean up Git working directory (#3683) (#13001) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(util/git/client): make runCredentialedCmd() signature coherent with runCmd() one Signed-off-by: Yann Soubeyrand * 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 --------- Signed-off-by: Yann Soubeyrand --- test/e2e/fixture/app/actions.go | 6 +++++ test/e2e/fixture/fixture.go | 12 ++++++++++ test/e2e/git_submodule_test.go | 22 +++++++++++++++++++ .../testdata/git-submodule/submodule-pod.yaml | 4 +--- util/git/client.go | 21 +++++++++++------- 5 files changed, 54 insertions(+), 11 deletions(-) diff --git a/test/e2e/fixture/app/actions.go b/test/e2e/fixture/app/actions.go index 5baa5937ff40a2..c4e173ddf6336a 100644 --- 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 index dd54222ca31204..c5048ccb7cd10d 100644 --- 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 index 27476db84a1edc..525c13d4b35ef4 100644 --- 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 index fa3b92c2f5875f..134107da31cba9 100644 --- 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 index 51e98df746d86c..f7ff776cd4f571 100644 --- 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