From 87cd01a12cfce82ea1c738964d620e52394d5ddb Mon Sep 17 00:00:00 2001 From: Marco Rinalducci Date: Sat, 26 Oct 2019 17:13:06 +0200 Subject: [PATCH 1/7] Detects if master branch has diverged and warn to pull new commits in comment --- go.mod | 1 + go.sum | 3 ++ server/events/markdown_renderer.go | 7 ++- server/events/mock_workingdir_test.go | 9 ++-- server/events/mocks/mock_working_dir.go | 9 ++-- server/events/models/models.go | 2 + server/events/project_command_builder.go | 4 +- server/events/project_command_runner.go | 3 +- server/events/project_command_runner_test.go | 5 +- server/events/working_dir.go | 48 +++++++++++++++----- server/events/working_dir_test.go | 18 ++++---- 11 files changed, 73 insertions(+), 36 deletions(-) diff --git a/go.mod b/go.mod index f755f24803..cb8dd42851 100644 --- a/go.mod +++ b/go.mod @@ -63,6 +63,7 @@ require ( gopkg.in/fsnotify.v1 v1.4.7 // indirect gopkg.in/go-playground/assert.v1 v1.2.1 // indirect gopkg.in/go-playground/validator.v9 v9.20.2 + gopkg.in/russross/blackfriday.v2 v2.0.0 gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect gopkg.in/yaml.v2 v2.2.2 gotest.tools v2.2.0+incompatible // indirect diff --git a/go.sum b/go.sum index 749019644b..8ba2204b2e 100644 --- a/go.sum +++ b/go.sum @@ -211,6 +211,7 @@ github.com/shurcooL/issuesapp v0.0.0-20180602232740-048589ce2241/go.mod h1:NPpHK github.com/shurcooL/notifications v0.0.0-20181007000457-627ab5aea122/go.mod h1:b5uSkrEVM1jQUspwbixRBhaIjIzL2xazXp6kntxYle0= github.com/shurcooL/octicon v0.0.0-20181028054416-fa4f57f9efb2/go.mod h1:eWdoE5JD4R5UVWDucdOPg1g2fqQRq78IQa9zlOV1vpQ= github.com/shurcooL/reactions v0.0.0-20181006231557-f2e0b4ca5b82/go.mod h1:TCR1lToEk4d2s07G3XGfz2QrgHXg4RJBvjrOozvoWfk= +github.com/shurcooL/sanitized_anchor_name v0.0.0-20170918181015-86672fcb3f95 h1:/vdW8Cb7EXrkqWGufVMES1OH2sU9gKVb2n9/1y5NMBY= github.com/shurcooL/sanitized_anchor_name v0.0.0-20170918181015-86672fcb3f95/go.mod h1:1NzhyTcUVG4SuEtjjoZeVRXNmyL/1OwPU0+IJeTBvfc= github.com/shurcooL/users v0.0.0-20180125191416-49c67e49c537/go.mod h1:QJTqeLYEDaXHZDBsXlPCDqdhQuJkuw4NOtaxYe3xii4= github.com/shurcooL/webdavfs v0.0.0-20170829043945-18c3829fa133/go.mod h1:hKmq5kWdCj2z2KEozexVbfEZIWiTjhE0+UjmZgPqehw= @@ -341,6 +342,8 @@ gopkg.in/go-playground/assert.v1 v1.2.1/go.mod h1:9RXL0bg/zibRAgZUYszZSwO/z8Y/a8 gopkg.in/go-playground/validator.v9 v9.20.2 h1:6AVDyt8bk0FDiSYSeWivUfzqEjHyVSCMRkpTr6ZCIgk= gopkg.in/go-playground/validator.v9 v9.20.2/go.mod h1:+c9/zcJMFNgbLvly1L1V+PpxWdVbfP1avr/N00E2vyQ= gopkg.in/inf.v0 v0.9.1/go.mod h1:cWUDdTG/fYaXco+Dcufb5Vnc6Gp2YChqWtbxRZE0mXw= +gopkg.in/russross/blackfriday.v2 v2.0.0 h1:+FlnIV8DSQnT7NZ43hcVKcdJdzZoeCmJj4Ql8gq5keA= +gopkg.in/russross/blackfriday.v2 v2.0.0/go.mod h1:6sSBNz/GtOm/pJTuh5UmBK2ZHfmnxGbl2NZg1UliSOI= gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 h1:uRGJdciOHaEIrze2W8Q3AKkepLTh2hOroT7a+7czfdQ= gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7/go.mod h1:dt/ZhP58zS4L8KSrWDmTeBkI65Dw0HsyUHuEVlX15mw= gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= diff --git a/server/events/markdown_renderer.go b/server/events/markdown_renderer.go index c6620c4944..91a1c42b85 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: Master branch has new commits, it is recommended to pull new commits{{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: Master branch has new commits, it is recommended to pull new commits{{end}}")) // planNextSteps are instructions appended after successful plans as to what // to do next. 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..eea7467855 100644 --- a/server/events/models/models.go +++ b/server/events/models/models.go @@ -431,6 +431,8 @@ 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 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 bedd17098f..68f5946490 100644 --- a/server/events/project_command_builder.go +++ b/server/events/project_command_builder.go @@ -109,7 +109,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 } @@ -172,7 +172,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..b5e9ab77bf 100644 --- a/server/events/working_dir.go +++ b/server/events/working_dir.go @@ -35,7 +35,7 @@ const workingDirPrefix = "repos" 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) + 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 +70,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,17 +89,41 @@ func (w *FileWorkspace) Clone( } revParseCmd := exec.Command("git", "rev-parse", pullHead) // #nosec revParseCmd.Dir = cloneDir - output, 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)) + outputRevParseCmd, errRevParseCmd := revParseCmd.CombinedOutput() + if errRevParseCmd != nil { + 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 w.forceClone(log, cloneDir, headRepo, p) } - currCommit := strings.Trim(string(output), "\n") + currCommit := strings.Trim(string(outputRevParseCmd), "\n") + + // We're bring our remote refs up to date + remoteUpdateCmd := exec.Command("git", "remote", "update") + remoteUpdateCmd.Dir = cloneDir + outputRemoteUpdate, errRemoteUpdate := remoteUpdateCmd.CombinedOutput() + if errRemoteUpdate != nil { + log.Warn("getting remote update failed: %s", string(outputRemoteUpdate)) + } + + // We're checking if remote master branch has diverged + statusUnoCmd := exec.Command("git", "status", "-uno") + statusUnoCmd.Dir = cloneDir + outputStatusUno, errStatusUno := statusUnoCmd.CombinedOutput() + if errStatusUno != 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 has new commits, you have 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, nil + return cloneDir, hasDiverged, 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. @@ -112,17 +136,17 @@ func (w *FileWorkspace) Clone( func (w *FileWorkspace) forceClone(log *logging.SimpleLogger, cloneDir string, headRepo models.Repo, - p models.PullRequest) (string, error) { + p models.PullRequest) (string, bool, error) { err := os.RemoveAll(cloneDir) if err != nil { - return "", errors.Wrapf(err, "deleting dir %q before cloning", cloneDir) + return "", false, 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 "", false, errors.Wrap(err, "creating new workspace") } // During testing, we mock some of this out. @@ -184,11 +208,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 "", false, fmt.Errorf("running %s: %s: %s", cmdStr, sanitizedOutput, sanitizedErrMsg) } log.Debug("ran: %s. Output: %s", cmdStr, strings.TrimSuffix(sanitizedOutput, "\n")) } - return cloneDir, nil + return cloneDir, false, 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..55720b5f0e 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,7 +76,7 @@ func TestClone_CheckoutMergeNoneExisting(t *testing.T) { TestingOverrideBaseCloneURL: overrideURL, } - cloneDir, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ + cloneDir, _, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", BaseBranch: "master", }, "default") @@ -123,7 +123,7 @@ func TestClone_CheckoutMergeNoReclone(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") @@ -133,7 +133,7 @@ func TestClone_CheckoutMergeNoReclone(t *testing.T) { 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, _, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", BaseBranch: "master", }, "default") @@ -169,7 +169,7 @@ func TestClone_CheckoutMergeNoRecloneFastForward(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") @@ -179,7 +179,7 @@ func TestClone_CheckoutMergeNoRecloneFastForward(t *testing.T) { 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, _, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", BaseBranch: "master", }, "default") @@ -220,7 +220,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,7 +250,7 @@ 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, _, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", }, "default") Ok(t, err) @@ -284,7 +284,7 @@ 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, _, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{ HeadBranch: "branch", HeadCommit: expCommit, }, "default") From ebec1de8866ece0045125a167479a7d1888dcd11 Mon Sep 17 00:00:00 2001 From: Marco Rinalducci Date: Sun, 27 Oct 2019 00:13:42 +0200 Subject: [PATCH 2/7] Changed pull warning message --- server/events/markdown_renderer.go | 4 ++-- server/events/working_dir.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/server/events/markdown_renderer.go b/server/events/markdown_renderer.go index 91a1c42b85..c39d038798 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 has new commits, it is recommended to pull new commits{{end}}")) + "{{ if .HasDiverged }}\n\n :warning: Master branch is ahead and thereby has new commits, it is recommended to pull new commits.{{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 has new commits, it is recommended to pull new commits{{end}}")) + "{{ if .HasDiverged }}\n\n :warning: Master branch is ahead and thereby has new commits, it is recommended to pull new commits.{{end}}")) // planNextSteps are instructions appended after successful plans as to what // to do next. diff --git a/server/events/working_dir.go b/server/events/working_dir.go index b5e9ab77bf..0a94ab588d 100644 --- a/server/events/working_dir.go +++ b/server/events/working_dir.go @@ -114,7 +114,7 @@ func (w *FileWorkspace) Clone( status := strings.Trim(string(outputStatusUno), "\n") hasDiverged := strings.Contains(status, "have diverged") if hasDiverged { - log.Info("remote master branch has new commits, you have to pull new commits") + 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") } From c55fb30e842efde8f02ffbb9fb0a2852594bb925 Mon Sep 17 00:00:00 2001 From: Marco Rinalducci Date: Sun, 27 Oct 2019 09:59:32 +0100 Subject: [PATCH 3/7] Changed pull warning message --- server/events/markdown_renderer.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/events/markdown_renderer.go b/server/events/markdown_renderer.go index c39d038798..9391bed015 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 and thereby has new commits, it is recommended to pull new commits.{{end}}")) + "{{ if .HasDiverged }}\n\n :warning: Master branch 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 and thereby has new commits, it is recommended to pull new commits.{{end}}")) + "{{ if .HasDiverged }}\n\n :warning: Master branch is ahead, it is recommended to pull new commits first.{{end}}")) // planNextSteps are instructions appended after successful plans as to what // to do next. From 2a5635e97a4392dce7f9773cacf2758ff0c8761e Mon Sep 17 00:00:00 2001 From: Marco RINALDUCCI Date: Mon, 28 Oct 2019 19:09:58 +0100 Subject: [PATCH 4/7] Added render and clone tests for master branch has diverged --- server/events/markdown_renderer.go | 4 +- server/events/markdown_renderer_test.go | 36 ++++++++++++++ server/events/working_dir_test.go | 63 +++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 2 deletions(-) diff --git a/server/events/markdown_renderer.go b/server/events/markdown_renderer.go index 9391bed015..3e4b1d1871 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: Master branch 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: Master branch 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..19e33d9087 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: Master branch 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/working_dir_test.go b/server/events/working_dir_test.go index 55720b5f0e..6790538386 100644 --- a/server/events/working_dir_test.go +++ b/server/events/working_dir_test.go @@ -295,6 +295,69 @@ func TestClone_RecloneWrongCommit(t *testing.T) { Equals(t, expCommit, actCommit) } +// Test that if master (remote) repo has diverged, see issue 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) + + // Check if master repo is ahead and has diverged + Equals(t, hasDiverged, true) +} + func initRepo(t *testing.T) (string, func()) { repoDir, cleanup := TempDir(t) runCmd(t, repoDir, "git", "init") From 4b15b0c878a4516d596ee241ff94953674e416e4 Mon Sep 17 00:00:00 2001 From: Marco RINALDUCCI Date: Wed, 30 Oct 2019 20:08:02 +0100 Subject: [PATCH 5/7] Solved merge conflicts --- go.mod | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/go.mod b/go.mod index cb8dd42851..200bd34ed7 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module github.com/runatlantis/atlantis go 1.13 require ( + git.apache.org/thrift.git v0.12.0 // indirect github.com/Masterminds/semver v1.4.2 // indirect github.com/Masterminds/sprig v2.15.0+incompatible github.com/aokoli/goutils v1.0.1 // indirect @@ -17,18 +18,16 @@ require ( github.com/go-ozzo/ozzo-validation v0.0.0-20170913164239-85dcd8368eba github.com/go-playground/locales v0.12.1 // indirect github.com/go-playground/universal-translator v0.16.0 // indirect - github.com/go-test/deep v1.0.1 + github.com/go-test/deep v1.0.3 github.com/google/go-github/v28 v28.0.0 github.com/google/uuid v0.0.0-20161128191214-064e2069ce9c // indirect github.com/gorilla/context v0.0.0-20160226214623-1ea25387ff6f // indirect github.com/gorilla/mux v1.6.2 - github.com/hashicorp/errwrap v0.0.0-20141028054710-7554cd9344ce // indirect - github.com/hashicorp/go-getter v1.2.0 - github.com/hashicorp/go-multierror v0.0.0-20170622060955-83588e72410a - github.com/hashicorp/go-version v1.1.0 - github.com/hashicorp/golang-lru v0.5.1 // indirect + github.com/grpc-ecosystem/grpc-gateway v1.6.2 // indirect + github.com/hashicorp/go-getter v1.4.0 + github.com/hashicorp/go-version v1.2.0 github.com/hashicorp/hcl v0.0.0-20170914154624-68e816d1c783 // indirect - github.com/hpcloud/tail v1.0.0 // indirect + github.com/hashicorp/terraform-config-inspect v0.0.0-20190821133035-82a99dc22ef4 github.com/huandu/xstrings v1.0.0 // indirect github.com/imdario/mergo v0.3.5 // indirect github.com/inconshreveable/mousetrap v1.0.0 // indirect @@ -37,34 +36,32 @@ require ( github.com/microcosm-cc/bluemonday v1.0.1 github.com/mitchellh/colorstring v0.0.0-20150917214807-8631ce90f286 github.com/mitchellh/go-homedir v1.0.0 + github.com/mitchellh/gox v1.0.1 // indirect github.com/mitchellh/mapstructure v0.0.0-20170523030023-d0303fe80992 // indirect github.com/mohae/deepcopy v0.0.0-20170603005431-491d3605edfb github.com/nlopes/slack v0.1.0 github.com/onsi/ginkgo v1.9.0 // indirect - github.com/onsi/gomega v1.2.0 // indirect + github.com/openzipkin/zipkin-go v0.1.3 // indirect github.com/pelletier/go-buffruneio v0.2.0 // indirect github.com/pelletier/go-toml v1.0.0 // indirect github.com/petergtz/pegomock v2.5.0+incompatible github.com/pkg/errors v0.8.0 + github.com/prometheus/client_golang v0.9.3-0.20190127221311-3c4408c8b829 // indirect github.com/spf13/afero v0.0.0-20170901052352-ee1bd8ee15a1 // indirect github.com/spf13/cast v1.1.0 // indirect github.com/spf13/cobra v0.0.0-20170905172051-b78744579491 github.com/spf13/jwalterweatherman v0.0.0-20170901151539-12bd96e66386 // indirect - github.com/spf13/pflag v1.0.0 + github.com/spf13/pflag v1.0.3 github.com/spf13/viper v1.0.0 github.com/ulikunitz/xz v0.5.6 // indirect github.com/urfave/cli v1.20.0 github.com/urfave/negroni v0.2.0 github.com/xanzy/go-gitlab v0.20.2-0.20190819195750-b1d195859ad0 - go.opencensus.io v0.19.1 // indirect - golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2 - golang.org/x/sys v0.0.0-20190312061237-fead79001313 // indirect - google.golang.org/genproto v0.0.0-20190307195333-5fe7a883aa19 // indirect - gopkg.in/fsnotify.v1 v1.4.7 // indirect + golang.org/x/build v0.0.0-20190111050920-041ab4dc3f9d // indirect + golang.org/x/crypto v0.0.0-20190605123033-f99c8df09eb5 gopkg.in/go-playground/assert.v1 v1.2.1 // indirect gopkg.in/go-playground/validator.v9 v9.20.2 gopkg.in/russross/blackfriday.v2 v2.0.0 - gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect gopkg.in/yaml.v2 v2.2.2 gotest.tools v2.2.0+incompatible // indirect ) From d8b59ebc2dbb02397df9084da6ceb31fab72cdc6 Mon Sep 17 00:00:00 2001 From: Marco RINALDUCCI Date: Thu, 31 Oct 2019 10:52:37 +0100 Subject: [PATCH 6/7] Added tests for all clones --- server/events/working_dir_test.go | 45 ++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/server/events/working_dir_test.go b/server/events/working_dir_test.go index 6790538386..eda205c43d 100644 --- a/server/events/working_dir_test.go +++ b/server/events/working_dir_test.go @@ -27,11 +27,14 @@ func TestClone_NoneExisting(t *testing.T) { 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) + // 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) @@ -76,12 +79,15 @@ 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) + // Check if master repo has not diverged + Equals(t, hasDiverged, false) + // Check the commits. actBaseCommit := runCmd(t, cloneDir, "git", "rev-parse", "HEAD~1") actHeadCommit := runCmd(t, cloneDir, "git", "rev-parse", "HEAD^2") @@ -123,22 +129,28 @@ 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) + // Check if master repo has not diverged + Equals(t, hasDiverged, false) + // 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) + // Check if master repo has not diverged + Equals(t, hasDiverged, false) + // Check that our proof file is still there, proving that we didn't reclone. _, err = os.Stat(filepath.Join(cloneDir, "proof")) Ok(t, err) @@ -169,22 +181,28 @@ 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) + // Check if master repo has not diverged + Equals(t, hasDiverged, false) + // 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) + // Check if master repo has not diverged + Equals(t, hasDiverged, false) + // Check that our proof file is still there, proving that we didn't reclone. _, err = os.Stat(filepath.Join(cloneDir, "proof")) Ok(t, err) @@ -220,11 +238,14 @@ func TestClone_CheckoutMergeConflict(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") + // 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) @@ -250,11 +271,14 @@ 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) + // Check if master repo has not diverged + Equals(t, hasDiverged, false) + // Check that our proof file is still there. _, err = os.Stat(filepath.Join(cloneDir, "proof")) Ok(t, err) @@ -284,12 +308,15 @@ 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) + // 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) From 1981a6735c0e6d2a53aa36cc44a6dbdcb667557c Mon Sep 17 00:00:00 2001 From: Marco Rinalducci Date: Tue, 12 Nov 2019 22:02:12 +0100 Subject: [PATCH 7/7] Make code clearer --- server/events/working_dir.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/server/events/working_dir.go b/server/events/working_dir.go index 0a94ab588d..aadb5031f7 100644 --- a/server/events/working_dir.go +++ b/server/events/working_dir.go @@ -34,7 +34,7 @@ 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. + // absolute path to the root of the cloned repo as well as if master branch has diverged. 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. @@ -89,26 +89,26 @@ func (w *FileWorkspace) Clone( } revParseCmd := exec.Command("git", "rev-parse", pullHead) // #nosec revParseCmd.Dir = cloneDir - outputRevParseCmd, errRevParseCmd := revParseCmd.CombinedOutput() - if errRevParseCmd != nil { + 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(outputRevParseCmd)) return w.forceClone(log, cloneDir, headRepo, p) } currCommit := strings.Trim(string(outputRevParseCmd), "\n") - // We're bring our remote refs up to date + // Bring our remote refs up to date. remoteUpdateCmd := exec.Command("git", "remote", "update") remoteUpdateCmd.Dir = cloneDir - outputRemoteUpdate, errRemoteUpdate := remoteUpdateCmd.CombinedOutput() - if errRemoteUpdate != nil { + outputRemoteUpdate, err := remoteUpdateCmd.CombinedOutput() + if err != nil { log.Warn("getting remote update failed: %s", string(outputRemoteUpdate)) } - // We're checking if remote master branch has diverged - statusUnoCmd := exec.Command("git", "status", "-uno") + // Check if remote master branch has diverged. + statusUnoCmd := exec.Command("git", "status", "--untracked-files=no") statusUnoCmd.Dir = cloneDir - outputStatusUno, errStatusUno := statusUnoCmd.CombinedOutput() - if errStatusUno != nil { + outputStatusUno, err := statusUnoCmd.CombinedOutput() + if err != nil { log.Warn("getting repo status has failed: %s", string(outputStatusUno)) } status := strings.Trim(string(outputStatusUno), "\n")