Get mergeable status *before* changing build status

server/events/command_context.go

@@ -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
 }

server/events/command_runner.go

@@ -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)
 	}

server/events/models/models.go

@@ -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.

server/events/project_command_builder.go

@@ -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 {
@@ -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,
 			})
 		}
 	}
@@ -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
 }
 

server/events/project_command_runner.go

@@ -83,7 +83,6 @@ type DefaultProjectCommandRunner struct {
 	ApplyStepRunner          StepRunner
 	RunStepRunner            StepRunner
 	PullApprovedChecker      runtime.PullApprovedChecker
-	PullMergeableChecker     runtime.PullMergeableChecker
 	WorkingDir               WorkingDir
 	Webhooks                 WebhooksSender
 	WorkingDirLocker         WorkingDirLocker
@@ -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}
 	}
 
@@ -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}
 	}
 
@@ -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
 			}
 		}

server/events/project_command_runner_test.go

@@ -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)
@@ -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",
@@ -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",
@@ -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",
@@ -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",
@@ -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",
@@ -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",
@@ -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,
 		},
 	}
 
@@ -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()
@@ -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":