diff --git a/server/events/markdown_renderer.go b/server/events/markdown_renderer.go index c6620c4944..6541868461 100644 --- a/server/events/markdown_renderer.go +++ b/server/events/markdown_renderer.go @@ -228,14 +228,17 @@ var multiProjectApplyTmpl = template.Must(template.New("").Funcs(sprig.TxtFuncMa var planSuccessUnwrappedTmpl = template.Must(template.New("").Parse( "```diff\n" + "{{.TerraformOutput}}\n" + - "```\n\n" + planNextSteps)) + "```\n\n" + planNextSteps + + "{{ 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" + "```diff\n" + "{{.TerraformOutput}}\n" + "```\n\n" + planNextSteps + "\n" + - "
")) + "" + + "{{ 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 38a1ae4384..5e8924fba2 100644 --- a/server/events/markdown_renderer_test.go +++ b/server/events/markdown_renderer_test.go @@ -156,6 +156,42 @@ $$$ * :repeat: To **plan** this project again, comment: * $atlantis plan -d path -w workspace$ +--- +* :fast_forward: To **apply** all unapplied plans from this pull request, comment: + * $atlantis apply$ +`, + }, + { + "single successful plan with master ahead", + models.PlanCommand, + []models.ProjectResult{ + { + PlanSuccess: &models.PlanSuccess{ + TerraformOutput: "terraform-output", + LockURL: "lock-url", + RePlanCmd: "atlantis plan -d path -w workspace", + ApplyCmd: "atlantis apply -d path -w workspace", + HasDiverged: true, + }, + Workspace: "workspace", + RepoRelDir: "path", + }, + }, + models.Github, + `Ran Plan for dir: $path$ workspace: $workspace$ + +$$$diff +terraform-output +$$$ + +* :arrow_forward: To **apply** this plan, comment: + * $atlantis apply -d path -w workspace$ +* :put_litter_in_its_place: To **delete** this plan click [here](lock-url) +* :repeat: To **plan** this project again, comment: + * $atlantis plan -d path -w workspace$ + +: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: * $atlantis apply$ diff --git a/server/events/mock_workingdir_test.go b/server/events/mock_workingdir_test.go index c0b745f724..8a992f3ae9 100644 --- a/server/events/mock_workingdir_test.go +++ b/server/events/mock_workingdir_test.go @@ -4,11 +4,12 @@ package events import ( + "reflect" + "time" + pegomock "github.com/petergtz/pegomock" models "github.com/runatlantis/atlantis/server/events/models" logging "github.com/runatlantis/atlantis/server/logging" - "reflect" - "time" ) type MockWorkingDir struct { @@ -26,7 +27,7 @@ func NewMockWorkingDir(options ...pegomock.Option) *MockWorkingDir { func (mock *MockWorkingDir) SetFailHandler(fh pegomock.FailHandler) { mock.fail = fh } func (mock *MockWorkingDir) FailHandler() pegomock.FailHandler { return mock.fail } -func (mock *MockWorkingDir) Clone(log *logging.SimpleLogger, baseRepo models.Repo, headRepo models.Repo, p models.PullRequest, workspace string) (string, error) { +func (mock *MockWorkingDir) Clone(log *logging.SimpleLogger, baseRepo models.Repo, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) { if mock == nil { panic("mock must not be nil. Use myMock := NewMockWorkingDir().") } @@ -42,7 +43,7 @@ func (mock *MockWorkingDir) Clone(log *logging.SimpleLogger, baseRepo models.Rep ret1 = result[1].(error) } } - return ret0, ret1 + return ret0, false, ret1 } func (mock *MockWorkingDir) GetWorkingDir(r models.Repo, p models.PullRequest, workspace string) (string, error) { diff --git a/server/events/mocks/mock_working_dir.go b/server/events/mocks/mock_working_dir.go index fbe85ae1f7..a7a5a2911d 100644 --- a/server/events/mocks/mock_working_dir.go +++ b/server/events/mocks/mock_working_dir.go @@ -4,11 +4,12 @@ package mocks import ( + "reflect" + "time" + pegomock "github.com/petergtz/pegomock" models "github.com/runatlantis/atlantis/server/events/models" logging "github.com/runatlantis/atlantis/server/logging" - "reflect" - "time" ) type MockWorkingDir struct { @@ -26,7 +27,7 @@ func NewMockWorkingDir(options ...pegomock.Option) *MockWorkingDir { func (mock *MockWorkingDir) SetFailHandler(fh pegomock.FailHandler) { mock.fail = fh } func (mock *MockWorkingDir) FailHandler() pegomock.FailHandler { return mock.fail } -func (mock *MockWorkingDir) Clone(log *logging.SimpleLogger, baseRepo models.Repo, headRepo models.Repo, p models.PullRequest, workspace string) (string, error) { +func (mock *MockWorkingDir) Clone(log *logging.SimpleLogger, baseRepo models.Repo, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) { if mock == nil { panic("mock must not be nil. Use myMock := NewMockWorkingDir().") } @@ -42,7 +43,7 @@ func (mock *MockWorkingDir) Clone(log *logging.SimpleLogger, baseRepo models.Rep ret1 = result[1].(error) } } - return ret0, ret1 + return ret0, false, ret1 } func (mock *MockWorkingDir) GetWorkingDir(r models.Repo, p models.PullRequest, workspace string) (string, error) { diff --git a/server/events/models/models.go b/server/events/models/models.go index 00497a52fb..8e1ec53798 100644 --- a/server/events/models/models.go +++ b/server/events/models/models.go @@ -431,6 +431,10 @@ type PlanSuccess struct { RePlanCmd string // ApplyCmd is the command that users should run to apply this plan. ApplyCmd string + // 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 } // PullStatus is the current status of a pull request that is in progress. diff --git a/server/events/project_command_builder.go b/server/events/project_command_builder.go index 78eba22271..f88085da43 100644 --- a/server/events/project_command_builder.go +++ b/server/events/project_command_builder.go @@ -113,7 +113,7 @@ func (p *DefaultProjectCommandBuilder) buildPlanAllCommands(ctx *CommandContext, } ctx.Log.Debug("%d files were modified in this pull request", len(modifiedFiles)) - repoDir, err := p.WorkingDir.Clone(ctx.Log, ctx.BaseRepo, ctx.HeadRepo, ctx.Pull, workspace) + repoDir, _, err := p.WorkingDir.Clone(ctx.Log, ctx.BaseRepo, ctx.HeadRepo, ctx.Pull, workspace) if err != nil { return nil, err } @@ -176,7 +176,7 @@ func (p *DefaultProjectCommandBuilder) buildProjectPlanCommand(ctx *CommandConte defer unlockFn() ctx.Log.Debug("cloning repository") - repoDir, err := p.WorkingDir.Clone(ctx.Log, ctx.BaseRepo, ctx.HeadRepo, ctx.Pull, workspace) + repoDir, _, err := p.WorkingDir.Clone(ctx.Log, ctx.BaseRepo, ctx.HeadRepo, ctx.Pull, workspace) if err != nil { return pcc, err } diff --git a/server/events/project_command_runner.go b/server/events/project_command_runner.go index 3bd813052a..a5efbbd372 100644 --- a/server/events/project_command_runner.go +++ b/server/events/project_command_runner.go @@ -151,7 +151,7 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx models.ProjectCommandContext) ( defer unlockFn() // Clone is idempotent so okay to run even if the repo was already cloned. - repoDir, cloneErr := p.WorkingDir.Clone(ctx.Log, ctx.BaseRepo, ctx.HeadRepo, ctx.Pull, ctx.Workspace) + repoDir, hasDiverged, cloneErr := p.WorkingDir.Clone(ctx.Log, ctx.BaseRepo, ctx.HeadRepo, ctx.Pull, ctx.Workspace) if cloneErr != nil { if unlockErr := lockAttempt.UnlockFn(); unlockErr != nil { ctx.Log.Err("error unlocking state after plan error: %v", unlockErr) @@ -176,6 +176,7 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx models.ProjectCommandContext) ( TerraformOutput: strings.Join(outputs, "\n"), RePlanCmd: ctx.RePlanCmd, ApplyCmd: ctx.ApplyCmd, + HasDiverged: hasDiverged, }, "", nil } diff --git a/server/events/project_command_runner_test.go b/server/events/project_command_runner_test.go index 04be59abbf..9729d8f9a8 100644 --- a/server/events/project_command_runner_test.go +++ b/server/events/project_command_runner_test.go @@ -14,11 +14,12 @@ package events_test import ( - "github.com/hashicorp/go-version" - "github.com/runatlantis/atlantis/server/events/runtime" "os" "testing" + "github.com/hashicorp/go-version" + "github.com/runatlantis/atlantis/server/events/runtime" + . "github.com/petergtz/pegomock" "github.com/runatlantis/atlantis/server/events" "github.com/runatlantis/atlantis/server/events/mocks" diff --git a/server/events/working_dir.go b/server/events/working_dir.go index 01bb682e45..7563278f4b 100644 --- a/server/events/working_dir.go +++ b/server/events/working_dir.go @@ -34,8 +34,10 @@ 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. - Clone(log *logging.SimpleLogger, baseRepo models.Repo, headRepo models.Repo, p models.PullRequest, workspace string) (string, error) + // 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. GetWorkingDir(r models.Repo, p models.PullRequest, workspace string) (string, error) @@ -70,7 +72,7 @@ func (w *FileWorkspace) Clone( baseRepo models.Repo, headRepo models.Repo, p models.PullRequest, - workspace string) (string, error) { + workspace string) (string, bool, error) { cloneDir := w.cloneDir(baseRepo, p, workspace) // If the directory already exists, check if it's at the right commit. @@ -89,40 +91,84 @@ func (w *FileWorkspace) Clone( } revParseCmd := exec.Command("git", "rev-parse", pullHead) // #nosec revParseCmd.Dir = cloneDir - output, err := revParseCmd.CombinedOutput() + outputRevParseCmd, err := revParseCmd.CombinedOutput() if err != nil { - log.Warn("will re-clone repo, could not determine if was at correct commit: %s: %s: %s", strings.Join(revParseCmd.Args, " "), err, string(output)) - return w.forceClone(log, cloneDir, headRepo, p) + log.Warn("will re-clone repo, could not determine if was at correct commit: %s: %s: %s", strings.Join(revParseCmd.Args, " "), err, string(outputRevParseCmd)) + return cloneDir, false, w.forceClone(log, cloneDir, headRepo, p) } - currCommit := strings.Trim(string(output), "\n") + currCommit := strings.Trim(string(outputRevParseCmd), "\n") + // 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, 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. } // Otherwise we clone the repo. - return w.forceClone(log, cloneDir, headRepo, p) + 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. +// 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, - p models.PullRequest) (string, error) { + p models.PullRequest) error { err := os.RemoveAll(cloneDir) if err != nil { - return "", errors.Wrapf(err, "deleting dir %q before cloning", cloneDir) + return errors.Wrapf(err, "deleting dir %q before cloning", cloneDir) } // Create the directory and parents if necessary. log.Info("creating dir %q", cloneDir) if err := os.MkdirAll(cloneDir, 0700); err != nil { - return "", errors.Wrap(err, "creating new workspace") + return errors.Wrap(err, "creating new workspace") } // During testing, we mock some of this out. @@ -184,11 +230,11 @@ func (w *FileWorkspace) forceClone(log *logging.SimpleLogger, sanitizedOutput := w.sanitizeGitCredentials(string(output), p.BaseRepo, headRepo) if err != nil { sanitizedErrMsg := w.sanitizeGitCredentials(err.Error(), p.BaseRepo, headRepo) - return "", fmt.Errorf("running %s: %s: %s", cmdStr, sanitizedOutput, sanitizedErrMsg) + return fmt.Errorf("running %s: %s: %s", cmdStr, sanitizedOutput, sanitizedErrMsg) } log.Debug("ran: %s. Output: %s", cmdStr, strings.TrimSuffix(sanitizedOutput, "\n")) } - return cloneDir, nil + return nil } // GetWorkingDir returns the path to the workspace for this repo and pull. diff --git a/server/events/working_dir_test.go b/server/events/working_dir_test.go index f7786e2ce6..0058ef042c 100644 --- a/server/events/working_dir_test.go +++ b/server/events/working_dir_test.go @@ -27,7 +27,7 @@ func TestClone_NoneExisting(t *testing.T) { TestingOverrideHeadCloneURL: fmt.Sprintf("file://%s", repoDir), } - cloneDir, 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) @@ -76,11 +76,12 @@ func TestClone_CheckoutMergeNoneExisting(t *testing.T) { TestingOverrideBaseCloneURL: overrideURL, } - cloneDir, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ + cloneDir, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", BaseBranch: "master", }, "default") Ok(t, err) + Equals(t, false, hasDiverged) // Check the commits. actBaseCommit := runCmd(t, cloneDir, "git", "rev-parse", "HEAD~1") @@ -123,21 +124,23 @@ func TestClone_CheckoutMergeNoReclone(t *testing.T) { TestingOverrideBaseCloneURL: overrideURL, } - _, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ + _, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", BaseBranch: "master", }, "default") Ok(t, err) + 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") // Now run the clone again. - cloneDir, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ + cloneDir, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", BaseBranch: "master", }, "default") Ok(t, err) + 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")) @@ -169,21 +172,23 @@ func TestClone_CheckoutMergeNoRecloneFastForward(t *testing.T) { TestingOverrideBaseCloneURL: overrideURL, } - _, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ + _, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", BaseBranch: "master", }, "default") Ok(t, err) + 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") // Now run the clone again. - cloneDir, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ + cloneDir, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", BaseBranch: "master", }, "default") Ok(t, err) + 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")) @@ -220,7 +225,7 @@ func TestClone_CheckoutMergeConflict(t *testing.T) { TestingOverrideBaseCloneURL: overrideURL, } - _, 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") @@ -250,10 +255,11 @@ func TestClone_NoReclone(t *testing.T) { CheckoutMerge: false, TestingOverrideHeadCloneURL: fmt.Sprintf("file://%s", repoDir), } - cloneDir, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ + cloneDir, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", }, "default") Ok(t, err) + Equals(t, false, hasDiverged) // Check that our proof file is still there. _, err = os.Stat(filepath.Join(cloneDir, "proof")) @@ -284,17 +290,89 @@ func TestClone_RecloneWrongCommit(t *testing.T) { CheckoutMerge: false, TestingOverrideHeadCloneURL: fmt.Sprintf("file://%s", repoDir), } - cloneDir, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ + cloneDir, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", HeadCommit: expCommit, }, "default") Ok(t, err) + 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 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) + defer cleanup() + + // 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", "master", "--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", "merge", "-q", "--no-ff", "-m", "atlantis-merge", "FETCH_HEAD") + + // Simulate second PR. + runCmd(t, repoDir, "git", "checkout", "master") + 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", "master", "--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", "merge", "-q", "--no-ff", "-m", "atlantis-merge", "FETCH_HEAD") + + // Merge first PR + runCmd(t, repoDir, "git", "checkout", "master") + runCmd(t, repoDir, "git", "merge", "first-pr") + + // Copy the second-pr repo to our data dir which has diverged remote master + 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, + } + _, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ + HeadBranch: "second-pr", + BaseBranch: "master", + }, "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(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()) { repoDir, cleanup := TempDir(t) runCmd(t, repoDir, "git", "init")