Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Detects if master branch has diverged and warn to pull new commits #815

Closed
wants to merge 7 commits into from
Closed

Detects if master branch has diverged and warn to pull new commits #815

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
87cd01a
Detects if master branch has diverged and warn to pull new commits in…
MRinalducci Oct 26, 2019
ebec1de
Changed pull warning message
MRinalducci Oct 26, 2019
c55fb30
Changed pull warning message
MRinalducci Oct 27, 2019
2a5635e
Added render and clone tests for master branch has diverged
picmrinalducci Oct 28, 2019
4b15b0c
Solved merge conflicts
MRinalducci Oct 30, 2019
d8b59eb
Added tests for all clones
MRinalducci Oct 31, 2019
1981a67
Make code clearer
MRinalducci Nov 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down Expand Up @@ -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=
Expand Down
7 changes: 5 additions & 2 deletions server/events/markdown_renderer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 is ahead, it is recommended to pull new commits first.{{end}}"))

var planSuccessWrappedTmpl = template.Must(template.New("").Parse(
"<details><summary>Show Output</summary>\n\n" +
"```diff\n" +
"{{.TerraformOutput}}\n" +
"```\n\n" +
planNextSteps + "\n" +
"</details>"))
"</details>" +
"{{ 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.
Expand Down
9 changes: 5 additions & 4 deletions server/events/mock_workingdir_test.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 5 additions & 4 deletions server/events/mocks/mock_working_dir.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions server/events/models/models.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions server/events/project_command_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
3 changes: 2 additions & 1 deletion server/events/project_command_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
}

Expand Down
5 changes: 3 additions & 2 deletions server/events/project_command_runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
48 changes: 36 additions & 12 deletions server/events/working_dir.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
MRinalducci marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down Expand Up @@ -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.
Expand All @@ -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()
MRinalducci marked this conversation as resolved.
Show resolved Hide resolved
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
MRinalducci marked this conversation as resolved.
Show resolved Hide resolved
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
MRinalducci marked this conversation as resolved.
Show resolved Hide resolved
statusUnoCmd := exec.Command("git", "status", "-uno")
MRinalducci marked this conversation as resolved.
Show resolved Hide resolved
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we skip this section if any of the above commands fail? Maybe pull it into its own function so you can return early.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it like this because I don't want to impact the clone function if for any reason it cannot detected that master branch has diverged as it displays only a warning in the comment of the plan.

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, 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.
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
18 changes: 9 additions & 9 deletions server/events/working_dir_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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")
Expand Down