From 96c583a0e3a8d9eaca90b6127000921a4e6564dd Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Wed, 27 Nov 2019 10:46:29 -0800 Subject: [PATCH] Refactor branch diverged warning. - only check for divergence if using checkout strategy merge --- server/events/markdown_renderer.go | 4 +- server/events/markdown_renderer_test.go | 2 +- server/events/models/models.go | 4 +- server/events/working_dir.go | 72 ++++++++++++++++--------- server/events/working_dir_test.go | 54 ++++++++----------- 5 files changed, 74 insertions(+), 62 deletions(-) diff --git a/server/events/markdown_renderer.go b/server/events/markdown_renderer.go index 3e4b1d1871..6541868461 100644 --- a/server/events/markdown_renderer.go +++ b/server/events/markdown_renderer.go @@ -229,7 +229,7 @@ var planSuccessUnwrappedTmpl = template.Must(template.New("").Parse( "```diff\n" + "{{.TerraformOutput}}\n" + "```\n\n" + planNextSteps + - "{{ if .HasDiverged }}\n\n:warning: Master branch is ahead, it is recommended to pull new commits first.{{end}}")) + "{{ if .HasDiverged }}\n\n:warning: The branch we're merging into is ahead, it is recommended to pull new commits first.{{end}}")) var planSuccessWrappedTmpl = template.Must(template.New("").Parse( "
Show Output\n\n" + @@ -238,7 +238,7 @@ var planSuccessWrappedTmpl = template.Must(template.New("").Parse( "```\n\n" + planNextSteps + "\n" + "
" + - "{{ if .HasDiverged }}\n\n:warning: Master branch is ahead, it is recommended to pull new commits first.{{end}}")) + "{{ if .HasDiverged }}\n\n:warning: The branch we're merging into is ahead, it is recommended to pull new commits first.{{end}}")) // planNextSteps are instructions appended after successful plans as to what // to do next. diff --git a/server/events/markdown_renderer_test.go b/server/events/markdown_renderer_test.go index 19e33d9087..5e8924fba2 100644 --- a/server/events/markdown_renderer_test.go +++ b/server/events/markdown_renderer_test.go @@ -190,7 +190,7 @@ $$$ * :repeat: To **plan** this project again, comment: * $atlantis plan -d path -w workspace$ -:warning: Master branch is ahead, it is recommended to pull new commits first. +:warning: The branch we're merging into is ahead, it is recommended to pull new commits first. --- * :fast_forward: To **apply** all unapplied plans from this pull request, comment: diff --git a/server/events/models/models.go b/server/events/models/models.go index eea7467855..8e1ec53798 100644 --- a/server/events/models/models.go +++ b/server/events/models/models.go @@ -431,7 +431,9 @@ type PlanSuccess struct { RePlanCmd string // ApplyCmd is the command that users should run to apply this plan. ApplyCmd string - // Indicates if remote master has diverged + // HasDiverged is true if we're using the checkout merge strategy and the + // branch we're merging into has been updated since we cloned and merged + // it. HasDiverged bool } diff --git a/server/events/working_dir.go b/server/events/working_dir.go index aadb5031f7..b08c82b753 100644 --- a/server/events/working_dir.go +++ b/server/events/working_dir.go @@ -34,7 +34,9 @@ const workingDirPrefix = "repos" // WorkingDir handles the workspace on disk for running commands. type WorkingDir interface { // Clone git clones headRepo, checks out the branch and then returns the - // absolute path to the root of the cloned repo as well as if master branch has diverged. + // 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. Clone(log *logging.SimpleLogger, baseRepo models.Repo, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) // GetWorkingDir returns the path to the workspace for this repo and pull. // If workspace does not exist on disk, error will be of type os.IsNotExist. @@ -96,35 +98,13 @@ func (w *FileWorkspace) Clone( } currCommit := strings.Trim(string(outputRevParseCmd), "\n") - // Bring our remote refs up to date. - remoteUpdateCmd := exec.Command("git", "remote", "update") - remoteUpdateCmd.Dir = cloneDir - outputRemoteUpdate, err := remoteUpdateCmd.CombinedOutput() - if err != nil { - log.Warn("getting remote update failed: %s", string(outputRemoteUpdate)) - } - - // Check if remote master branch has diverged. - statusUnoCmd := exec.Command("git", "status", "--untracked-files=no") - statusUnoCmd.Dir = cloneDir - outputStatusUno, err := statusUnoCmd.CombinedOutput() - if err != nil { - log.Warn("getting repo status has failed: %s", string(outputStatusUno)) - } - status := strings.Trim(string(outputStatusUno), "\n") - hasDiverged := strings.Contains(status, "have diverged") - if hasDiverged { - log.Info("remote master branch is ahead and thereby has new commits, it is recommended to pull new commits") - } else { - log.Debug("remote master branch has no new commits") - } - // 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, hasDiverged, nil + return cloneDir, w.warnDiverged(log, cloneDir), nil } + 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. } @@ -133,6 +113,48 @@ func (w *FileWorkspace) Clone( return 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. +// This matters in the case of the merge checkout strategy because after +// cloning the repo and doing the merge, it's possible master was updated. +// Then users won't be getting the merge functionality they expected. +// 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.SimpleLogger, cloneDir string) bool { + if !w.CheckoutMerge { + // It only makes sense to warn that master has diverged if we're using + // the checkout merge strategy. If we're just checking out the branch, + // then it doesn't matter what's going on with master because we've + // decided to always run off the branch. + return false + } + + // Bring our remote refs up to date. + remoteUpdateCmd := exec.Command("git", "remote", "update") + remoteUpdateCmd.Dir = cloneDir + outputRemoteUpdate, err := remoteUpdateCmd.CombinedOutput() + if err != nil { + log.Warn("getting remote update failed: %s", string(outputRemoteUpdate)) + return false + } + + // Check if remote master branch has diverged. + statusUnoCmd := exec.Command("git", "status", "--untracked-files=no") + statusUnoCmd.Dir = cloneDir + outputStatusUno, err := statusUnoCmd.CombinedOutput() + if err != nil { + log.Warn("getting repo status has failed: %s", string(outputStatusUno)) + return false + } + hasDiverged := strings.Contains(string(outputStatusUno), "have diverged") + if hasDiverged { + log.Info("remote master branch is ahead and thereby has new commits, it is recommended to pull new commits") + } else { + log.Debug("remote master branch has no new commits") + } + return hasDiverged +} + func (w *FileWorkspace) forceClone(log *logging.SimpleLogger, cloneDir string, headRepo models.Repo, diff --git a/server/events/working_dir_test.go b/server/events/working_dir_test.go index eda205c43d..0058ef042c 100644 --- a/server/events/working_dir_test.go +++ b/server/events/working_dir_test.go @@ -27,14 +27,11 @@ func TestClone_NoneExisting(t *testing.T) { TestingOverrideHeadCloneURL: fmt.Sprintf("file://%s", repoDir), } - cloneDir, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ + cloneDir, _, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", }, "default") Ok(t, err) - // Check if master repo has not diverged - Equals(t, hasDiverged, false) - // Use rev-parse to verify at correct commit. actCommit := runCmd(t, cloneDir, "git", "rev-parse", "HEAD") Equals(t, expCommit, actCommit) @@ -84,9 +81,7 @@ func TestClone_CheckoutMergeNoneExisting(t *testing.T) { BaseBranch: "master", }, "default") Ok(t, err) - - // Check if master repo has not diverged - Equals(t, hasDiverged, false) + Equals(t, false, hasDiverged) // Check the commits. actBaseCommit := runCmd(t, cloneDir, "git", "rev-parse", "HEAD~1") @@ -134,9 +129,7 @@ func TestClone_CheckoutMergeNoReclone(t *testing.T) { BaseBranch: "master", }, "default") Ok(t, err) - - // Check if master repo has not diverged - Equals(t, hasDiverged, false) + Equals(t, false, hasDiverged) // Create a file that we can use to check if the repo was recloned. runCmd(t, dataDir, "touch", "repos/0/default/proof") @@ -147,9 +140,7 @@ func TestClone_CheckoutMergeNoReclone(t *testing.T) { BaseBranch: "master", }, "default") Ok(t, err) - - // Check if master repo has not diverged - Equals(t, hasDiverged, false) + Equals(t, false, hasDiverged) // Check that our proof file is still there, proving that we didn't reclone. _, err = os.Stat(filepath.Join(cloneDir, "proof")) @@ -186,9 +177,7 @@ func TestClone_CheckoutMergeNoRecloneFastForward(t *testing.T) { BaseBranch: "master", }, "default") Ok(t, err) - - // Check if master repo has not diverged - Equals(t, hasDiverged, false) + Equals(t, false, hasDiverged) // Create a file that we can use to check if the repo was recloned. runCmd(t, dataDir, "touch", "repos/0/default/proof") @@ -199,9 +188,7 @@ func TestClone_CheckoutMergeNoRecloneFastForward(t *testing.T) { BaseBranch: "master", }, "default") Ok(t, err) - - // Check if master repo has not diverged - Equals(t, hasDiverged, false) + Equals(t, false, hasDiverged) // Check that our proof file is still there, proving that we didn't reclone. _, err = os.Stat(filepath.Join(cloneDir, "proof")) @@ -238,14 +225,11 @@ func TestClone_CheckoutMergeConflict(t *testing.T) { TestingOverrideBaseCloneURL: overrideURL, } - _, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ + _, _, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", BaseBranch: "master", }, "default") - // Check if master repo has not diverged - Equals(t, hasDiverged, false) - ErrContains(t, "running git merge -q --no-ff -m atlantis-merge FETCH_HEAD", err) ErrContains(t, "Auto-merging file", err) ErrContains(t, "CONFLICT (add/add)", err) @@ -275,9 +259,7 @@ func TestClone_NoReclone(t *testing.T) { HeadBranch: "branch", }, "default") Ok(t, err) - - // Check if master repo has not diverged - Equals(t, hasDiverged, false) + Equals(t, false, hasDiverged) // Check that our proof file is still there. _, err = os.Stat(filepath.Join(cloneDir, "proof")) @@ -313,16 +295,15 @@ func TestClone_RecloneWrongCommit(t *testing.T) { HeadCommit: expCommit, }, "default") Ok(t, err) - - // Check if master repo has not diverged - Equals(t, hasDiverged, false) + Equals(t, false, hasDiverged) // Use rev-parse to verify at correct commit. actCommit := runCmd(t, cloneDir, "git", "rev-parse", "HEAD") Equals(t, expCommit, actCommit) } -// Test that if master (remote) repo has diverged, see issue 804 +// 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, cleanup := initRepo(t) @@ -374,15 +355,22 @@ func TestClone_MasterHasDiverged(t *testing.T) { DataDir: repoDir, CheckoutMerge: true, } - _, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "second-pr", BaseBranch: "master", }, "default") Ok(t, err) - - // Check if master repo is ahead and has diverged Equals(t, hasDiverged, true) + + // Run it again but without the checkout merge strategy. It should return + // false. + wd.CheckoutMerge = false + _, hasDiverged, err = wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ + HeadBranch: "second-pr", + BaseBranch: "master", + }, "default") + Ok(t, err) + Equals(t, hasDiverged, false) } func initRepo(t *testing.T) (string, func()) {