Skip to content

Commit

Permalink
Merge pull request #469 from runatlantis/mergeability
Browse files Browse the repository at this point in the history 
Get mergeable status *before* changing build status
  • Loading branch information
@lkysow
lkysow authored Feb 11, 2019
2 parents 6b07938 + 4e42958 commit f4e1472
Show file tree
Hide file tree
Showing 9 changed files with 105 additions and 179 deletions.
5 changes: 5 additions & 0 deletions server/events/command_context.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,9 @@ type CommandContext struct {
// User is the user that triggered this command.
User models.User
Log *logging.SimpleLogger
// PullMergeable is true if Pull is able to be merged. This is available in
// the CommandContext because we want to collect this information before we
// set our own build statuses which can affect mergeability if users have
// required the Atlantis status to be successful prior to merging.
PullMergeable bool
}
17 changes: 17 additions & 0 deletions server/events/command_runner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,23 @@ func (c *DefaultCommandRunner) RunCommentCommand(baseRepo models.Repo, maybeHead
if !c.validateCtxAndComment(ctx) {
return
}

if cmd.CommandName() == ApplyCommand {
// Get the mergeable status before we set any build statuses of our own.
// We do this here because when we set a "Pending" status, if users have
// required the Atlantis status checks to pass, then we've now changed
// the mergeability status of the pull request.
ctx.PullMergeable, err = c.VCSClient.PullIsMergeable(baseRepo, pull)
if err != nil {
// On error we continue the request with mergeable assumed false.
// We want to continue because not all apply's will need this status,
// only if they rely on the mergeability requirement.
ctx.PullMergeable = false
ctx.Log.Warn("unable to get mergeable status: %s. Continuing with mergeable assumed false", err)
}
ctx.Log.Info("pull request mergeable status: %t", ctx.PullMergeable)
}

if err = c.CommitStatusUpdater.Update(baseRepo, pull, models.PendingCommitStatus, cmd.CommandName()); err != nil {
ctx.Log.Warn("unable to update commit status: %s", err)
}
Expand Down
6 changes: 4 additions & 2 deletions server/events/models/models.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,10 @@ type ProjectCommandContext struct {
// If the pull request branch is from the same repository then HeadRepo will
// be the same as BaseRepo.
// See https://help.github.com/articles/about-pull-request-merges/.
HeadRepo Repo
Log *logging.SimpleLogger
HeadRepo Repo
Log *logging.SimpleLogger
// PullMergeable is true if the pull request for this project is able to be merged.
PullMergeable bool
Pull PullRequest
ProjectConfig *valid.Project
// RePlanCmd is the command that users should run to re-plan this project.
Expand Down
3 changes: 3 additions & 0 deletions server/events/project_command_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ func (p *DefaultProjectCommandBuilder) buildPlanAllCommands(ctx *CommandContext,
Verbose: verbose,
RePlanCmd: p.CommentBuilder.BuildPlanComment(mp.Path, DefaultWorkspace, "", commentFlags),
ApplyCmd: p.CommentBuilder.BuildApplyComment(mp.Path, DefaultWorkspace, ""),
PullMergeable: ctx.PullMergeable,
})
}
} else {
Expand Down Expand Up @@ -173,6 +174,7 @@ func (p *DefaultProjectCommandBuilder) buildPlanAllCommands(ctx *CommandContext,
Verbose: verbose,
RePlanCmd: p.CommentBuilder.BuildPlanComment(mp.Dir, mp.Workspace, mp.GetName(), commentFlags),
ApplyCmd: p.CommentBuilder.BuildApplyComment(mp.Dir, mp.Workspace, mp.GetName()),
PullMergeable: ctx.PullMergeable,
})
}
}
Expand Down Expand Up @@ -321,6 +323,7 @@ func (p *DefaultProjectCommandBuilder) buildProjectCommandCtx(ctx *CommandContex
GlobalConfig: globalCfg,
RePlanCmd: p.CommentBuilder.BuildPlanComment(repoRelDir, workspace, projectName, commentFlags),
ApplyCmd: p.CommentBuilder.BuildApplyComment(repoRelDir, workspace, projectName),
PullMergeable: ctx.PullMergeable,
}, nil
}

Expand Down
11 changes: 3 additions & 8 deletions server/events/project_command_runner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ type DefaultProjectCommandRunner struct {
ApplyStepRunner StepRunner
RunStepRunner StepRunner
PullApprovedChecker runtime.PullApprovedChecker
PullMergeableChecker runtime.PullMergeableChecker
WorkingDir WorkingDir
Webhooks WebhooksSender
WorkingDirLocker WorkingDirLocker
Expand Down Expand Up @@ -144,7 +143,7 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx models.ProjectCommandContext) (
return nil, "", cloneErr
}
projAbsPath := filepath.Join(repoDir, ctx.RepoRelDir)
if _, err := os.Stat(projAbsPath); os.IsNotExist(err) {
if _, err = os.Stat(projAbsPath); os.IsNotExist(err) {
return nil, "", DirNotExistErr{RepoRelDir: ctx.RepoRelDir}
}

Expand Down Expand Up @@ -209,7 +208,7 @@ func (p *DefaultProjectCommandRunner) doApply(ctx models.ProjectCommandContext)
return "", "", err
}
absPath := filepath.Join(repoDir, ctx.RepoRelDir)
if _, err := os.Stat(absPath); os.IsNotExist(err) {
if _, err = os.Stat(absPath); os.IsNotExist(err) {
return "", "", DirNotExistErr{RepoRelDir: ctx.RepoRelDir}
}

Expand Down Expand Up @@ -238,11 +237,7 @@ func (p *DefaultProjectCommandRunner) doApply(ctx models.ProjectCommandContext)
return "", "Pull request must be approved before running apply.", nil
}
case raw.MergeableApplyRequirement:
mergeable, err := p.PullMergeableChecker.PullIsMergeable(ctx.BaseRepo, ctx.Pull) // nolint: vetshadow
if err != nil {
return "", "", errors.Wrap(err, "checking if pull request is mergeable")
}
if !mergeable {
if !ctx.PullMergeable {
return "", "Pull request must be mergeable before running apply.", nil
}
}
Expand Down
124 changes: 73 additions & 51 deletions server/events/project_command_runner_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,17 +134,16 @@ func TestDefaultProjectCommandRunner_Plan(t *testing.T) {
mockLocker := mocks.NewMockProjectLocker()

runner := events.DefaultProjectCommandRunner{
Locker: mockLocker,
LockURLGenerator: mockURLGenerator{},
InitStepRunner: mockInit,
PlanStepRunner: mockPlan,
ApplyStepRunner: mockApply,
RunStepRunner: mockRun,
PullApprovedChecker: nil,
PullMergeableChecker: nil,
WorkingDir: mockWorkingDir,
Webhooks: nil,
WorkingDirLocker: events.NewDefaultWorkingDirLocker(),
Locker: mockLocker,
LockURLGenerator: mockURLGenerator{},
InitStepRunner: mockInit,
PlanStepRunner: mockPlan,
ApplyStepRunner: mockApply,
RunStepRunner: mockRun,
PullApprovedChecker: nil,
WorkingDir: mockWorkingDir,
Webhooks: nil,
WorkingDirLocker: events.NewDefaultWorkingDirLocker(),
}

repoDir, cleanup := TempDir(t)
Expand Down Expand Up @@ -236,37 +235,37 @@ func TestDefaultProjectCommandRunner_ApplyNotApproved(t *testing.T) {
func TestDefaultProjectCommandRunner_ApplyNotMergeable(t *testing.T) {
RegisterMockTestingT(t)
mockWorkingDir := mocks.NewMockWorkingDir()
mockMergeable := mocks2.NewMockPullMergeableChecker()
runner := &events.DefaultProjectCommandRunner{
WorkingDir: mockWorkingDir,
PullMergeableChecker: mockMergeable,
WorkingDirLocker: events.NewDefaultWorkingDirLocker(),
RequireMergeableOverride: true,
}
ctx := models.ProjectCommandContext{}
tmp, cleanup := TempDir(t)
defer cleanup()
When(mockWorkingDir.GetWorkingDir(ctx.BaseRepo, ctx.Pull, ctx.Workspace)).ThenReturn(tmp, nil)
When(mockMergeable.PullIsMergeable(ctx.BaseRepo, ctx.Pull)).ThenReturn(false, nil)

res := runner.Apply(ctx)
Equals(t, "Pull request must be mergeable before running apply.", res.Failure)
}

func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
cases := []struct {
description string
projCfg *valid.Project
globalCfg *valid.Config
expSteps []string
expOut string
description string
projCfg *valid.Project
globalCfg *valid.Config
expSteps []string
expOut string
expFailure string
pullMergeable bool
}{
{
description: "use defaults",
projCfg: nil,
globalCfg: nil,
expSteps: []string{"apply"},
expOut: "apply",
description: "use defaults",
projCfg: nil,
globalCfg: nil,
expSteps: []string{"apply"},
expOut: "apply",
pullMergeable: true,
},
{
description: "no workflow, use defaults",
Expand All @@ -281,8 +280,9 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
},
},
},
expSteps: []string{"apply"},
expOut: "apply",
expSteps: []string{"apply"},
expOut: "apply",
pullMergeable: true,
},
{
description: "no workflow, approval required, use defaults",
Expand All @@ -299,8 +299,9 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
},
},
},
expSteps: []string{"approve", "apply"},
expOut: "apply",
expSteps: []string{"approve", "apply"},
expOut: "apply",
pullMergeable: true,
},
{
description: "no workflow, mergeable required, use defaults",
Expand All @@ -317,8 +318,29 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
},
},
},
expSteps: []string{"mergeable", "apply"},
expOut: "apply",
expSteps: []string{"apply"},
expOut: "apply",
pullMergeable: true,
},
{
description: "no workflow, mergeable required, pull not mergeable",
projCfg: &valid.Project{
Dir: ".",
ApplyRequirements: []string{"mergeable"},
},
globalCfg: &valid.Config{
Version: 2,
Projects: []valid.Project{
{
Dir: ".",
ApplyRequirements: []string{"mergeable"},
},
},
},
expSteps: []string{""},
expOut: "",
expFailure: "Pull request must be mergeable before running apply.",
pullMergeable: false,
},
{
description: "no workflow, mergeable and approved required, use defaults",
Expand All @@ -335,8 +357,9 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
},
},
},
expSteps: []string{"mergeable", "approved", "apply"},
expOut: "apply",
expSteps: []string{"approved", "apply"},
expOut: "apply",
pullMergeable: true,
},
{
description: "workflow without apply stage set",
Expand All @@ -359,8 +382,9 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
},
},
},
expSteps: []string{"apply"},
expOut: "apply",
expSteps: []string{"apply"},
expOut: "apply",
pullMergeable: true,
},
{
description: "workflow with custom apply stage",
Expand Down Expand Up @@ -396,8 +420,9 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
},
},
},
expSteps: []string{"run", "apply", "plan", "init"},
expOut: "run\napply\nplan\ninit",
expSteps: []string{"run", "apply", "plan", "init"},
expOut: "run\napply\nplan\ninit",
pullMergeable: true,
},
}

Expand All @@ -409,23 +434,21 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
mockApply := mocks.NewMockStepRunner()
mockRun := mocks.NewMockStepRunner()
mockApproved := mocks2.NewMockPullApprovedChecker()
mockMergeable := mocks2.NewMockPullMergeableChecker()
mockWorkingDir := mocks.NewMockWorkingDir()
mockLocker := mocks.NewMockProjectLocker()
mockSender := mocks.NewMockWebhooksSender()

runner := events.DefaultProjectCommandRunner{
Locker: mockLocker,
LockURLGenerator: mockURLGenerator{},
InitStepRunner: mockInit,
PlanStepRunner: mockPlan,
ApplyStepRunner: mockApply,
RunStepRunner: mockRun,
PullApprovedChecker: mockApproved,
PullMergeableChecker: mockMergeable,
WorkingDir: mockWorkingDir,
Webhooks: mockSender,
WorkingDirLocker: events.NewDefaultWorkingDirLocker(),
Locker: mockLocker,
LockURLGenerator: mockURLGenerator{},
InitStepRunner: mockInit,
PlanStepRunner: mockPlan,
ApplyStepRunner: mockApply,
RunStepRunner: mockRun,
PullApprovedChecker: mockApproved,
WorkingDir: mockWorkingDir,
Webhooks: mockSender,
WorkingDirLocker: events.NewDefaultWorkingDirLocker(),
}
repoDir, cleanup := TempDir(t)
defer cleanup()
Expand All @@ -441,23 +464,22 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
Workspace: "default",
GlobalConfig: c.globalCfg,
RepoRelDir: ".",
PullMergeable: c.pullMergeable,
}
When(mockInit.Run(ctx, nil, repoDir)).ThenReturn("init", nil)
When(mockPlan.Run(ctx, nil, repoDir)).ThenReturn("plan", nil)
When(mockApply.Run(ctx, nil, repoDir)).ThenReturn("apply", nil)
When(mockRun.Run(ctx, nil, repoDir)).ThenReturn("run", nil)
When(mockApproved.PullIsApproved(ctx.BaseRepo, ctx.Pull)).ThenReturn(true, nil)
When(mockMergeable.PullIsMergeable(ctx.BaseRepo, ctx.Pull)).ThenReturn(true, nil)

res := runner.Apply(ctx)
Equals(t, c.expOut, res.ApplySuccess)
Equals(t, c.expFailure, res.Failure)

for _, step := range c.expSteps {
switch step {
case "approved":
mockApproved.VerifyWasCalledOnce().PullIsApproved(ctx.BaseRepo, ctx.Pull)
case "mergeable":
mockMergeable.VerifyWasCalledOnce().PullIsMergeable(ctx.BaseRepo, ctx.Pull)
case "init":
mockInit.VerifyWasCalledOnce().Run(ctx, nil, repoDir)
case "plan":
Expand Down
Loading

0 comments on commit f4e1472

Please sign in to comment.