From 2bb1442aa8f2c58e018e486a403bb1e9abda0c4a Mon Sep 17 00:00:00 2001 From: Finn Arne Gangstad Date: Fri, 3 Mar 2023 10:24:07 +0100 Subject: [PATCH 1/4] merge again if base branch has been updated The current "merge" checkout strategy is unsafe. It merges the PR and the base branch without holding the directory lock(s), so there is a potentially very long window where another PR can be applied and be unexpectedly reverted later. This happens occasionally if a PR causes plans in multiple directories, but is almost _guaranteed_ to happen if the initial plan has to wait until a lock is freed up and a manual "atlantis plan" command is given. Instead of printing a warning when this happens, we now merge again if necessary while holding the lock. Plans are then guaranteed to only be made when merged with the base branch for each directory being planned, and applying later should be safe even if the base branch sees further updates. This fixes/affects #804, #867, #979 --- server/events/working_dir.go | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/server/events/working_dir.go b/server/events/working_dir.go index aab1fb83e8..1a65c588b6 100644 --- a/server/events/working_dir.go +++ b/server/events/working_dir.go @@ -117,11 +117,15 @@ func (w *FileWorkspace) Clone( // We're prefix matching here because BitBucket doesn't give us the full // commit, only a 12 character prefix. if strings.HasPrefix(currCommit, p.HeadCommit) { - log.Debug("repo is at correct commit %q so will not re-clone", p.HeadCommit) - return cloneDir, w.warnDiverged(log, p, headRepo, cloneDir), nil + if w.CheckoutMerge && w.recheckDiverged(log, p, headRepo, cloneDir) { + log.Info("base branch has been updated, using merge strategy and will clone again") + } else { + log.Debug("repo is at correct commit %q so will not re-clone", p.HeadCommit) + return cloneDir, false, nil + } + } else { + log.Debug("repo was already cloned but is not at correct commit, wanted %q got %q", p.HeadCommit, currCommit) } - - log.Debug("repo was already cloned but is not at correct commit, wanted %q got %q", p.HeadCommit, currCommit) // We'll fall through to re-clone. } @@ -129,14 +133,14 @@ func (w *FileWorkspace) Clone( return cloneDir, false, w.forceClone(log, cloneDir, headRepo, p) } -// warnDiverged returns true if we should warn the user that the branch we're -// merging into has diverged from what we currently have checked out. +// recheckDiverged returns true if the branch we're merging into has diverged +// from what we currently have checked out. // This matters in the case of the merge checkout strategy because after -// cloning the repo and doing the merge, it's possible main was updated. -// Then users won't be getting the merge functionality they expected. +// cloning the repo and doing the merge, it's possible main was updated +// and we have to perform a new merge. // If there are any errors we return false since we prefer things to succeed // vs. stopping the plan/apply. -func (w *FileWorkspace) warnDiverged(log logging.SimpleLogging, p models.PullRequest, headRepo models.Repo, cloneDir string) bool { +func (w *FileWorkspace) recheckDiverged(log logging.SimpleLogging, p models.PullRequest, headRepo models.Repo, cloneDir string) bool { if !w.CheckoutMerge { // It only makes sense to warn that main has diverged if we're using // the checkout merge strategy. If we're just checking out the branch, @@ -174,13 +178,7 @@ func (w *FileWorkspace) warnDiverged(log logging.SimpleLogging, p models.PullReq } } - hasDiverged := w.HasDiverged(log, cloneDir) - if hasDiverged { - log.Info("remote main branch is ahead and thereby has new commits, it is recommended to pull new commits") - } else { - log.Debug("remote main branch has no new commits") - } - return hasDiverged + return w.HasDiverged(log, cloneDir) } func (w *FileWorkspace) HasDiverged(log logging.SimpleLogging, cloneDir string) bool { From 66a67712b72ee7735b11dd299782697fcca42f4e Mon Sep 17 00:00:00 2001 From: Finn Arne Gangstad Date: Fri, 3 Mar 2023 15:53:37 +0100 Subject: [PATCH 2/4] Remove diverged test that no longer applies --- server/events/working_dir_test.go | 76 ------------------------------- 1 file changed, 76 deletions(-) diff --git a/server/events/working_dir_test.go b/server/events/working_dir_test.go index 458329140b..2cff1396e7 100644 --- a/server/events/working_dir_test.go +++ b/server/events/working_dir_test.go @@ -411,82 +411,6 @@ func TestClone_RecloneWrongCommit(t *testing.T) { Equals(t, expCommit, actCommit) } -// Test that if the branch we're merging into has diverged and we're using -// checkout-strategy=merge, we warn the user (see #804). -func TestClone_MasterHasDiverged(t *testing.T) { - // Initialize the git repo. - repoDir := initRepo(t) - - // Simulate first PR. - runCmd(t, repoDir, "git", "checkout", "-b", "first-pr") - runCmd(t, repoDir, "touch", "file1") - runCmd(t, repoDir, "git", "add", "file1") - runCmd(t, repoDir, "git", "commit", "-m", "file1") - - // Atlantis checkout first PR. - firstPRDir := repoDir + "/first-pr" - runCmd(t, repoDir, "mkdir", "-p", "first-pr") - runCmd(t, firstPRDir, "git", "clone", "--branch", "main", "--single-branch", repoDir, ".") - runCmd(t, firstPRDir, "git", "remote", "add", "head", repoDir) - runCmd(t, firstPRDir, "git", "fetch", "head", "+refs/heads/first-pr") - runCmd(t, firstPRDir, "git", "config", "--local", "user.email", "atlantisbot@runatlantis.io") - runCmd(t, firstPRDir, "git", "config", "--local", "user.name", "atlantisbot") - runCmd(t, firstPRDir, "git", "config", "--local", "commit.gpgsign", "false") - runCmd(t, firstPRDir, "git", "merge", "-q", "--no-ff", "-m", "atlantis-merge", "FETCH_HEAD") - - // Simulate second PR. - runCmd(t, repoDir, "git", "checkout", "main") - runCmd(t, repoDir, "git", "checkout", "-b", "second-pr") - runCmd(t, repoDir, "touch", "file2") - runCmd(t, repoDir, "git", "add", "file2") - runCmd(t, repoDir, "git", "commit", "-m", "file2") - - // Atlantis checkout second PR. - secondPRDir := repoDir + "/second-pr" - runCmd(t, repoDir, "mkdir", "-p", "second-pr") - runCmd(t, secondPRDir, "git", "clone", "--branch", "main", "--single-branch", repoDir, ".") - runCmd(t, secondPRDir, "git", "remote", "add", "head", repoDir) - runCmd(t, secondPRDir, "git", "fetch", "head", "+refs/heads/second-pr") - runCmd(t, secondPRDir, "git", "config", "--local", "user.email", "atlantisbot@runatlantis.io") - runCmd(t, secondPRDir, "git", "config", "--local", "user.name", "atlantisbot") - runCmd(t, secondPRDir, "git", "config", "--local", "commit.gpgsign", "false") - runCmd(t, secondPRDir, "git", "merge", "-q", "--no-ff", "-m", "atlantis-merge", "FETCH_HEAD") - - // Merge first PR - runCmd(t, repoDir, "git", "checkout", "main") - runCmd(t, repoDir, "git", "merge", "first-pr") - - // Copy the second-pr repo to our data dir which has diverged remote main - runCmd(t, repoDir, "mkdir", "-p", "repos/0/") - runCmd(t, repoDir, "cp", "-R", secondPRDir, "repos/0/default") - - // Run the clone. - wd := &events.FileWorkspace{ - DataDir: repoDir, - CheckoutMerge: true, - CheckoutDepth: 50, - GpgNoSigningEnabled: true, - } - _, hasDiverged, err := wd.Clone(logging.NewNoopLogger(t), models.Repo{CloneURL: repoDir}, models.PullRequest{ - BaseRepo: models.Repo{CloneURL: repoDir}, - HeadBranch: "second-pr", - BaseBranch: "main", - }, "default") - Ok(t, err) - Equals(t, hasDiverged, true) - - // Run it again but without the checkout merge strategy. It should return - // false. - wd.CheckoutMerge = false - _, hasDiverged, err = wd.Clone(logging.NewNoopLogger(t), models.Repo{}, models.PullRequest{ - BaseRepo: models.Repo{}, - HeadBranch: "second-pr", - BaseBranch: "main", - }, "default") - Ok(t, err) - Equals(t, hasDiverged, false) -} - func TestHasDiverged_MasterHasDiverged(t *testing.T) { // Initialize the git repo. repoDir := initRepo(t) From f4c0f2c4d254dfe4433affb446befd464f283a64 Mon Sep 17 00:00:00 2001 From: Finn Arne Gangstad Date: Fri, 24 Mar 2023 10:28:41 +0100 Subject: [PATCH 3/4] Reinstate TestClone_MasterHasDiverged test and make it pass --- server/events/working_dir.go | 7 +-- server/events/working_dir_test.go | 76 +++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 3 deletions(-) diff --git a/server/events/working_dir.go b/server/events/working_dir.go index 2166ac3c0e..fa4087d6ad 100644 --- a/server/events/working_dir.go +++ b/server/events/working_dir.go @@ -79,8 +79,7 @@ type FileWorkspace struct { // Clone git clones headRepo, checks out the branch and then returns the absolute // path to the root of the cloned repo. It also returns -// a boolean indicating if we should warn users that the branch we're -// merging into has been updated since we cloned it. +// a boolean indicating whether we had to merge with upstream again. // If the repo already exists and is at // the right commit it does nothing. This is to support running commands in // multiple dirs of the same repo without deleting existing plans. @@ -90,6 +89,7 @@ func (w *FileWorkspace) Clone( p models.PullRequest, workspace string) (string, bool, error) { cloneDir := w.cloneDir(p.BaseRepo, p, workspace) + hasDiverged := false // If the directory already exists, check if it's at the right commit. // If so, then we do nothing. @@ -119,6 +119,7 @@ func (w *FileWorkspace) Clone( if strings.HasPrefix(currCommit, p.HeadCommit) { if w.CheckoutMerge && w.recheckDiverged(log, p, headRepo, cloneDir) { log.Info("base branch has been updated, using merge strategy and will clone again") + hasDiverged = true } else { log.Debug("repo is at correct commit %q so will not re-clone", p.HeadCommit) return cloneDir, false, nil @@ -130,7 +131,7 @@ func (w *FileWorkspace) Clone( } // Otherwise we clone the repo. - return cloneDir, false, w.forceClone(log, cloneDir, headRepo, p) + return cloneDir, hasDiverged, w.forceClone(log, cloneDir, headRepo, p) } // recheckDiverged returns true if the branch we're merging into has diverged diff --git a/server/events/working_dir_test.go b/server/events/working_dir_test.go index 2cff1396e7..458329140b 100644 --- a/server/events/working_dir_test.go +++ b/server/events/working_dir_test.go @@ -411,6 +411,82 @@ func TestClone_RecloneWrongCommit(t *testing.T) { Equals(t, expCommit, actCommit) } +// Test that if the branch we're merging into has diverged and we're using +// checkout-strategy=merge, we warn the user (see #804). +func TestClone_MasterHasDiverged(t *testing.T) { + // Initialize the git repo. + repoDir := initRepo(t) + + // Simulate first PR. + runCmd(t, repoDir, "git", "checkout", "-b", "first-pr") + runCmd(t, repoDir, "touch", "file1") + runCmd(t, repoDir, "git", "add", "file1") + runCmd(t, repoDir, "git", "commit", "-m", "file1") + + // Atlantis checkout first PR. + firstPRDir := repoDir + "/first-pr" + runCmd(t, repoDir, "mkdir", "-p", "first-pr") + runCmd(t, firstPRDir, "git", "clone", "--branch", "main", "--single-branch", repoDir, ".") + runCmd(t, firstPRDir, "git", "remote", "add", "head", repoDir) + runCmd(t, firstPRDir, "git", "fetch", "head", "+refs/heads/first-pr") + runCmd(t, firstPRDir, "git", "config", "--local", "user.email", "atlantisbot@runatlantis.io") + runCmd(t, firstPRDir, "git", "config", "--local", "user.name", "atlantisbot") + runCmd(t, firstPRDir, "git", "config", "--local", "commit.gpgsign", "false") + runCmd(t, firstPRDir, "git", "merge", "-q", "--no-ff", "-m", "atlantis-merge", "FETCH_HEAD") + + // Simulate second PR. + runCmd(t, repoDir, "git", "checkout", "main") + runCmd(t, repoDir, "git", "checkout", "-b", "second-pr") + runCmd(t, repoDir, "touch", "file2") + runCmd(t, repoDir, "git", "add", "file2") + runCmd(t, repoDir, "git", "commit", "-m", "file2") + + // Atlantis checkout second PR. + secondPRDir := repoDir + "/second-pr" + runCmd(t, repoDir, "mkdir", "-p", "second-pr") + runCmd(t, secondPRDir, "git", "clone", "--branch", "main", "--single-branch", repoDir, ".") + runCmd(t, secondPRDir, "git", "remote", "add", "head", repoDir) + runCmd(t, secondPRDir, "git", "fetch", "head", "+refs/heads/second-pr") + runCmd(t, secondPRDir, "git", "config", "--local", "user.email", "atlantisbot@runatlantis.io") + runCmd(t, secondPRDir, "git", "config", "--local", "user.name", "atlantisbot") + runCmd(t, secondPRDir, "git", "config", "--local", "commit.gpgsign", "false") + runCmd(t, secondPRDir, "git", "merge", "-q", "--no-ff", "-m", "atlantis-merge", "FETCH_HEAD") + + // Merge first PR + runCmd(t, repoDir, "git", "checkout", "main") + runCmd(t, repoDir, "git", "merge", "first-pr") + + // Copy the second-pr repo to our data dir which has diverged remote main + runCmd(t, repoDir, "mkdir", "-p", "repos/0/") + runCmd(t, repoDir, "cp", "-R", secondPRDir, "repos/0/default") + + // Run the clone. + wd := &events.FileWorkspace{ + DataDir: repoDir, + CheckoutMerge: true, + CheckoutDepth: 50, + GpgNoSigningEnabled: true, + } + _, hasDiverged, err := wd.Clone(logging.NewNoopLogger(t), models.Repo{CloneURL: repoDir}, models.PullRequest{ + BaseRepo: models.Repo{CloneURL: repoDir}, + HeadBranch: "second-pr", + BaseBranch: "main", + }, "default") + Ok(t, err) + Equals(t, hasDiverged, true) + + // Run it again but without the checkout merge strategy. It should return + // false. + wd.CheckoutMerge = false + _, hasDiverged, err = wd.Clone(logging.NewNoopLogger(t), models.Repo{}, models.PullRequest{ + BaseRepo: models.Repo{}, + HeadBranch: "second-pr", + BaseBranch: "main", + }, "default") + Ok(t, err) + Equals(t, hasDiverged, false) +} + func TestHasDiverged_MasterHasDiverged(t *testing.T) { // Initialize the git repo. repoDir := initRepo(t) From 50b33883b5173d4fb3542fac2119570e2a6642ea Mon Sep 17 00:00:00 2001 From: Finn Arne Gangstad Date: Fri, 24 Mar 2023 15:34:45 +0100 Subject: [PATCH 4/4] Extend TestClone_MasterHasDiverged to test new merging functionality We now verify that the first Clone with CheckoutMerge=true with a diverged base branch atually clones and merges again. --- server/events/working_dir_test.go | 33 +++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/server/events/working_dir_test.go b/server/events/working_dir_test.go index 458329140b..2622c67f9a 100644 --- a/server/events/working_dir_test.go +++ b/server/events/working_dir_test.go @@ -412,7 +412,8 @@ func TestClone_RecloneWrongCommit(t *testing.T) { } // Test that if the branch we're merging into has diverged and we're using -// checkout-strategy=merge, we warn the user (see #804). +// checkout-strategy=merge, we actually merge the branch. +// Also check that we do not merge if we are not using the merge strategy. func TestClone_MasterHasDiverged(t *testing.T) { // Initialize the git repo. repoDir := initRepo(t) @@ -463,28 +464,40 @@ func TestClone_MasterHasDiverged(t *testing.T) { // Run the clone. wd := &events.FileWorkspace{ DataDir: repoDir, - CheckoutMerge: true, + CheckoutMerge: false, CheckoutDepth: 50, GpgNoSigningEnabled: true, } - _, hasDiverged, err := wd.Clone(logging.NewNoopLogger(t), models.Repo{CloneURL: repoDir}, models.PullRequest{ + + // Run the clone without the checkout merge strategy. It should return + // false for hasDiverged + _, hasDiverged, err := wd.Clone(logging.NewNoopLogger(t), models.Repo{}, models.PullRequest{ + BaseRepo: models.Repo{}, + HeadBranch: "second-pr", + BaseBranch: "main", + }, "default") + Ok(t, err) + Assert(t, hasDiverged == false, "Clone with CheckoutMerge=false should not merge") + + wd.CheckoutMerge = true + // Run the clone twice with the merge strategy, the first run should + // return true for hasDiverged, subsequent runs should + // return false since the first call is supposed to merge. + _, hasDiverged, err = wd.Clone(logging.NewNoopLogger(t), models.Repo{CloneURL: repoDir}, models.PullRequest{ BaseRepo: models.Repo{CloneURL: repoDir}, HeadBranch: "second-pr", BaseBranch: "main", }, "default") Ok(t, err) - Equals(t, hasDiverged, true) + Assert(t, hasDiverged == true, "First clone with CheckoutMerge=true with diverged base should have merged") - // Run it again but without the checkout merge strategy. It should return - // false. - wd.CheckoutMerge = false - _, hasDiverged, err = wd.Clone(logging.NewNoopLogger(t), models.Repo{}, models.PullRequest{ - BaseRepo: models.Repo{}, + _, hasDiverged, err = wd.Clone(logging.NewNoopLogger(t), models.Repo{CloneURL: repoDir}, models.PullRequest{ + BaseRepo: models.Repo{CloneURL: repoDir}, HeadBranch: "second-pr", BaseBranch: "main", }, "default") Ok(t, err) - Equals(t, hasDiverged, false) + Assert(t, hasDiverged == false, "Second clone with CheckoutMerge=true and initially diverged base should not merge again") } func TestHasDiverged_MasterHasDiverged(t *testing.T) {