From fbb12fe3b996f156c393c8caa3a7013dd11b819d Mon Sep 17 00:00:00 2001 From: chroju <4526869+chroju@users.noreply.github.com> Date: Fri, 4 Aug 2023 00:15:13 +0900 Subject: [PATCH] feat: set `atlantis/apply` check to `successful` if all plans are `No Changes` (#3378) * mod: rename updateCommitStatus func * feat: add PlannedNoChangesPlanStatus * Add skipApplyNoChanges option to PlanCommandRunner * Add skipApplyNoChanges option to ApplyCommandRunner * Add --skip-apply-no-changes flag * Fix typo Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com> * Rename --skip-apply-no-changes flag * Refactor updateCommitStatus functions * chore(docs): add detailed use case for the flag * test: add plan_command_runner set apply status * feat: set apply status to successful by default when result is 'No Changes' --------- Co-authored-by: chroju Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com> --- server/events/apply_command_runner.go | 2 +- server/events/command/project_result.go | 2 + server/events/command/project_result_test.go | 9 + server/events/command_runner_internal_test.go | 117 +++++++- server/events/models/models.go | 5 + server/events/plan_command_runner.go | 39 ++- server/events/plan_command_runner_test.go | 253 ++++++++++++++++++ 7 files changed, 412 insertions(+), 15 deletions(-) diff --git a/server/events/apply_command_runner.go b/server/events/apply_command_runner.go index c8189d034..06439aff0 100644 --- a/server/events/apply_command_runner.go +++ b/server/events/apply_command_runner.go @@ -199,7 +199,7 @@ func (a *ApplyCommandRunner) updateCommitStatus(ctx *command.Context, pullStatus var numErrored int status := models.SuccessCommitStatus - numSuccess = pullStatus.StatusCount(models.AppliedPlanStatus) + numSuccess = pullStatus.StatusCount(models.AppliedPlanStatus) + pullStatus.StatusCount(models.PlannedNoChangesPlanStatus) numErrored = pullStatus.StatusCount(models.ErroredApplyStatus) if numErrored > 0 { diff --git a/server/events/command/project_result.go b/server/events/command/project_result.go index 09a84e033..0d59c4e9a 100644 --- a/server/events/command/project_result.go +++ b/server/events/command/project_result.go @@ -57,6 +57,8 @@ func (p ProjectResult) PlanStatus() models.ProjectPlanStatus { return models.ErroredPlanStatus } else if p.Failure != "" { return models.ErroredPlanStatus + } else if p.PlanSuccess.NoChanges() { + return models.PlannedNoChangesPlanStatus } return models.PlannedPlanStatus case PolicyCheck, ApprovePolicies: diff --git a/server/events/command/project_result_test.go b/server/events/command/project_result_test.go index 250dc374b..dad68c2e5 100644 --- a/server/events/command/project_result_test.go +++ b/server/events/command/project_result_test.go @@ -79,6 +79,15 @@ func TestProjectResult_PlanStatus(t *testing.T) { }, expStatus: models.PlannedPlanStatus, }, + { + p: command.ProjectResult{ + Command: command.Plan, + PlanSuccess: &models.PlanSuccess{ + TerraformOutput: "No changes. Infrastructure is up-to-date.", + }, + }, + expStatus: models.PlannedNoChangesPlanStatus, + }, { p: command.ProjectResult{ Command: command.Apply, diff --git a/server/events/command_runner_internal_test.go b/server/events/command_runner_internal_test.go index d16e6def5..4cf3076a0 100644 --- a/server/events/command_runner_internal_test.go +++ b/server/events/command_runner_internal_test.go @@ -67,6 +67,22 @@ func TestApplyUpdateCommitStatus(t *testing.T) { expNumSuccess: 1, expNumTotal: 3, }, + "apply, one planned no changes": { + cmd: command.Apply, + pullStatus: models.PullStatus{ + Projects: []models.ProjectStatus{ + { + Status: models.AppliedPlanStatus, + }, + { + Status: models.PlannedNoChangesPlanStatus, + }, + }, + }, + expStatus: models.SuccessCommitStatus, + expNumSuccess: 2, + expNumTotal: 2, + }, } for name, c := range cases { @@ -86,7 +102,7 @@ func TestApplyUpdateCommitStatus(t *testing.T) { } } -func TestPlanUpdateCommitStatus(t *testing.T) { +func TestPlanUpdatePlanCommitStatus(t *testing.T) { cases := map[string]struct { cmd command.Name pullStatus models.PullStatus @@ -137,7 +153,104 @@ func TestPlanUpdateCommitStatus(t *testing.T) { cr := &PlanCommandRunner{ commitStatusUpdater: csu, } - cr.updateCommitStatus(&command.Context{}, c.pullStatus) + cr.updateCommitStatus(&command.Context{}, c.pullStatus, command.Plan) + Equals(t, models.Repo{}, csu.CalledRepo) + Equals(t, models.PullRequest{}, csu.CalledPull) + Equals(t, c.expStatus, csu.CalledStatus) + Equals(t, c.cmd, csu.CalledCommand) + Equals(t, c.expNumSuccess, csu.CalledNumSuccess) + Equals(t, c.expNumTotal, csu.CalledNumTotal) + }) + } +} + +func TestPlanUpdateApplyCommitStatus(t *testing.T) { + cases := map[string]struct { + cmd command.Name + pullStatus models.PullStatus + expStatus models.CommitStatus + expNumSuccess int + expNumTotal int + }{ + "all plans success with no changes": { + cmd: command.Apply, + pullStatus: models.PullStatus{ + Projects: []models.ProjectStatus{ + { + Status: models.PlannedNoChangesPlanStatus, + }, + { + Status: models.PlannedNoChangesPlanStatus, + }, + }, + }, + expStatus: models.SuccessCommitStatus, + expNumSuccess: 2, + expNumTotal: 2, + }, + "one plan, one plan success with no changes": { + cmd: command.Apply, + pullStatus: models.PullStatus{ + Projects: []models.ProjectStatus{ + { + Status: models.PlannedNoChangesPlanStatus, + }, + { + Status: models.PlannedPlanStatus, + }, + }, + }, + expStatus: models.PendingCommitStatus, + expNumSuccess: 1, + expNumTotal: 2, + }, + "one plan, one apply, one plan success with no changes": { + cmd: command.Apply, + pullStatus: models.PullStatus{ + Projects: []models.ProjectStatus{ + { + Status: models.PlannedNoChangesPlanStatus, + }, + { + Status: models.AppliedPlanStatus, + }, + { + Status: models.PlannedPlanStatus, + }, + }, + }, + expStatus: models.PendingCommitStatus, + expNumSuccess: 2, + expNumTotal: 3, + }, + "one apply error, one apply, one plan success with no changes": { + cmd: command.Apply, + pullStatus: models.PullStatus{ + Projects: []models.ProjectStatus{ + { + Status: models.PlannedNoChangesPlanStatus, + }, + { + Status: models.AppliedPlanStatus, + }, + { + Status: models.ErroredApplyStatus, + }, + }, + }, + expStatus: models.FailedCommitStatus, + expNumSuccess: 2, + expNumTotal: 3, + }, + } + + for name, c := range cases { + t.Run(name, func(t *testing.T) { + csu := &MockCSU{} + cr := &PlanCommandRunner{ + commitStatusUpdater: csu, + } + cr.updateCommitStatus(&command.Context{}, c.pullStatus, command.Apply) Equals(t, models.Repo{}, csu.CalledRepo) Equals(t, models.PullRequest{}, csu.CalledPull) Equals(t, c.expStatus, csu.CalledStatus) diff --git a/server/events/models/models.go b/server/events/models/models.go index 4d9ccc60a..492fc36fe 100644 --- a/server/events/models/models.go +++ b/server/events/models/models.go @@ -562,6 +562,9 @@ const ( // PlannedPlanStatus means that a plan has been successfully generated but // not yet applied. PlannedPlanStatus + // PlannedNoChangesPlanStatus means that a plan has been successfully + // generated with "No changes" and not yet applied. + PlannedNoChangesPlanStatus // ErroredApplyStatus means that a plan has been generated but there was an // error while applying it. ErroredApplyStatus @@ -586,6 +589,8 @@ func (p ProjectPlanStatus) String() string { return "plan_errored" case PlannedPlanStatus: return "planned" + case PlannedNoChangesPlanStatus: + return "planned_no_changes" case ErroredApplyStatus: return "apply_errored" case AppliedPlanStatus: diff --git a/server/events/plan_command_runner.go b/server/events/plan_command_runner.go index 8fbd1409e..9313f14d4 100644 --- a/server/events/plan_command_runner.go +++ b/server/events/plan_command_runner.go @@ -148,7 +148,8 @@ func (p *PlanCommandRunner) runAutoplan(ctx *command.Context) { ctx.Log.Err("writing results: %s", err) } - p.updateCommitStatus(ctx, pullStatus) + p.updateCommitStatus(ctx, pullStatus, command.Plan) + p.updateCommitStatus(ctx, pullStatus, command.Apply) // Check if there are any planned projects and if there are any errors or if plans are being deleted if len(policyCheckCmds) > 0 && @@ -218,7 +219,7 @@ func (p *PlanCommandRunner) run(ctx *command.Context, cmd *CommentCommand) { return } ctx.Log.Debug("resetting VCS status") - p.updateCommitStatus(ctx, *pullStatus) + p.updateCommitStatus(ctx, *pullStatus, command.Plan) } else { // With a generic plan, we set successful commit statuses // with 0/0 projects planned successfully because some users require @@ -272,7 +273,8 @@ func (p *PlanCommandRunner) run(ctx *command.Context, cmd *CommentCommand) { return } - p.updateCommitStatus(ctx, pullStatus) + p.updateCommitStatus(ctx, pullStatus, command.Plan) + p.updateCommitStatus(ctx, pullStatus, command.Apply) // Runs policy checks step after all plans are successful. // This step does not approve any policies that require approval. @@ -291,26 +293,39 @@ func (p *PlanCommandRunner) Run(ctx *command.Context, cmd *CommentCommand) { } } -func (p *PlanCommandRunner) updateCommitStatus(ctx *command.Context, pullStatus models.PullStatus) { +func (p *PlanCommandRunner) updateCommitStatus(ctx *command.Context, pullStatus models.PullStatus, commandName command.Name) { var numSuccess int var numErrored int status := models.SuccessCommitStatus - numErrored = pullStatus.StatusCount(models.ErroredPlanStatus) - // We consider anything that isn't a plan error as a plan success. - // For example, if there is an apply error, that means that at least a - // plan was generated successfully. - numSuccess = len(pullStatus.Projects) - numErrored + if commandName == command.Plan { + numErrored = pullStatus.StatusCount(models.ErroredPlanStatus) + // We consider anything that isn't a plan error as a plan success. + // For example, if there is an apply error, that means that at least a + // plan was generated successfully. + numSuccess = len(pullStatus.Projects) - numErrored - if numErrored > 0 { - status = models.FailedCommitStatus + if numErrored > 0 { + status = models.FailedCommitStatus + } + } else if commandName == command.Apply { + numSuccess = pullStatus.StatusCount(models.AppliedPlanStatus) + pullStatus.StatusCount(models.PlannedNoChangesPlanStatus) + numErrored = pullStatus.StatusCount(models.ErroredApplyStatus) + + if numErrored > 0 { + status = models.FailedCommitStatus + } else if numSuccess < len(pullStatus.Projects) { + // If there are plans that haven't been applied yet, we'll use a pending + // status. + status = models.PendingCommitStatus + } } if err := p.commitStatusUpdater.UpdateCombinedCount( ctx.Pull.BaseRepo, ctx.Pull, status, - command.Plan, + commandName, numSuccess, len(pullStatus.Projects), ); err != nil { diff --git a/server/events/plan_command_runner_test.go b/server/events/plan_command_runner_test.go index 1d4b44150..e388ef249 100644 --- a/server/events/plan_command_runner_test.go +++ b/server/events/plan_command_runner_test.go @@ -508,3 +508,256 @@ func TestPlanCommandRunner_ExecutionOrder(t *testing.T) { }) } } + +func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { + logger := logging.NewNoopLogger(t) + RegisterMockTestingT(t) + + cases := []struct { + Description string + ProjectContexts []command.ProjectContext + ProjectResults []command.ProjectResult + PrevPlanStored bool // stores a previous "No changes" plan in the backend + ExpVCSApplyStatusTotal int + ExpVCSApplyStatusSucc int + }{ + { + Description: "When planning with changes, set the 0/1 apply status", + ProjectContexts: []command.ProjectContext{ + { + CommandName: command.Plan, + RepoRelDir: "mydir", + }, + }, + ProjectResults: []command.ProjectResult{ + { + RepoRelDir: "mydir", + Command: command.Plan, + PlanSuccess: &models.PlanSuccess{ + TerraformOutput: "Plan: 0 to add, 0 to change, 1 to destroy.", + }, + }, + }, + ExpVCSApplyStatusTotal: 1, + ExpVCSApplyStatusSucc: 0, + }, + { + Description: "When planning with no changes, set the 1/1 apply status", + ProjectContexts: []command.ProjectContext{ + { + CommandName: command.Plan, + RepoRelDir: "mydir", + }, + }, + ProjectResults: []command.ProjectResult{ + { + RepoRelDir: "mydir", + Command: command.Plan, + PlanSuccess: &models.PlanSuccess{ + TerraformOutput: "No changes. Infrastructure is up-to-date.", + }, + }, + }, + ExpVCSApplyStatusTotal: 1, + ExpVCSApplyStatusSucc: 1, + }, + { + Description: "When planning with no changes and previous plan with no changes, set the 1/2 apply status", + ProjectContexts: []command.ProjectContext{ + { + CommandName: command.Plan, + RepoRelDir: "mydir", + }, + }, + ProjectResults: []command.ProjectResult{ + { + RepoRelDir: "mydir", + Command: command.Plan, + PlanSuccess: &models.PlanSuccess{ + TerraformOutput: "Plan: 0 to add, 0 to change, 1 to destroy.", + }, + }, + }, + PrevPlanStored: true, + ExpVCSApplyStatusTotal: 2, + ExpVCSApplyStatusSucc: 1, + }, + { + Description: "When planning with no changes and previous 'No changes' plan, set the 2/2 apply status", + ProjectContexts: []command.ProjectContext{ + { + CommandName: command.Plan, + RepoRelDir: "mydir", + }, + }, + ProjectResults: []command.ProjectResult{ + { + RepoRelDir: "mydir", + Command: command.Plan, + PlanSuccess: &models.PlanSuccess{ + TerraformOutput: "No changes. Infrastructure is up-to-date.", + }, + }, + }, + PrevPlanStored: true, + ExpVCSApplyStatusTotal: 2, + ExpVCSApplyStatusSucc: 2, + }, + { + Description: "When planning again with changes following a previous 'No changes' plan, set the 0/1 apply status", + ProjectContexts: []command.ProjectContext{ + { + CommandName: command.Plan, + RepoRelDir: "prevdir", + Workspace: "default", + }, + }, + ProjectResults: []command.ProjectResult{ + { + RepoRelDir: "prevdir", + Workspace: "default", + Command: command.Plan, + PlanSuccess: &models.PlanSuccess{ + TerraformOutput: "Plan: 0 to add, 0 to change, 1 to destroy.", + }, + }, + }, + PrevPlanStored: true, + ExpVCSApplyStatusTotal: 1, + ExpVCSApplyStatusSucc: 0, + }, + { + Description: "When planning again with changes following a previous 'No changes' plan, while another plan with 'No changes', set the 1/2 apply status.", + ProjectContexts: []command.ProjectContext{ + { + CommandName: command.Plan, + RepoRelDir: "prevdir", + Workspace: "default", + }, + { + CommandName: command.Plan, + RepoRelDir: "mydir", + }, + }, + ProjectResults: []command.ProjectResult{ + { + RepoRelDir: "prevdir", + Workspace: "default", + Command: command.Plan, + PlanSuccess: &models.PlanSuccess{ + TerraformOutput: "Plan: 0 to add, 0 to change, 1 to destroy.", + }, + }, + { + RepoRelDir: "mydir", + Command: command.Plan, + PlanSuccess: &models.PlanSuccess{ + TerraformOutput: "No changes. Infrastructure is up-to-date.", + }, + }, + }, + PrevPlanStored: true, + ExpVCSApplyStatusTotal: 2, + ExpVCSApplyStatusSucc: 1, + }, + { + Description: "When planning again with no changes following a previous 'No changes' plan, while another plan also with 'No changes', set the 2/2 apply status.", + ProjectContexts: []command.ProjectContext{ + { + CommandName: command.Plan, + RepoRelDir: "prevdir", + Workspace: "default", + }, + { + CommandName: command.Plan, + RepoRelDir: "mydir", + }, + }, + ProjectResults: []command.ProjectResult{ + { + RepoRelDir: "prevdir", + Workspace: "default", + Command: command.Plan, + PlanSuccess: &models.PlanSuccess{ + TerraformOutput: "No changes. Infrastructure is up-to-date.", + }, + }, + { + RepoRelDir: "mydir", + Command: command.Plan, + PlanSuccess: &models.PlanSuccess{ + TerraformOutput: "No changes. Infrastructure is up-to-date.", + }, + }, + }, + PrevPlanStored: true, + ExpVCSApplyStatusTotal: 2, + ExpVCSApplyStatusSucc: 2, + }, + } + + for _, c := range cases { + t.Run(c.Description, func(t *testing.T) { + // create an empty DB + tmp := t.TempDir() + db, err := db.New(tmp) + Ok(t, err) + + vcsClient := setup(t, func(tc *TestConfig) { + tc.backend = db + }) + + scopeNull, _, _ := metrics.NewLoggingScope(logger, "atlantis") + modelPull := models.PullRequest{BaseRepo: testdata.GithubRepo, State: models.OpenPullState, Num: testdata.Pull.Num} + + cmd := &events.CommentCommand{Name: command.Plan} + + ctx := &command.Context{ + User: testdata.User, + Log: logging.NewNoopLogger(t), + Scope: scopeNull, + Pull: modelPull, + HeadRepo: testdata.GithubRepo, + Trigger: command.CommentTrigger, + } + + if c.PrevPlanStored { + _, err = db.UpdatePullWithResults(modelPull, []command.ProjectResult{ + { + Command: command.Plan, + RepoRelDir: "prevdir", + Workspace: "default", + PlanSuccess: &models.PlanSuccess{ + TerraformOutput: "No changes. Your infrastructure matches the configuration.", + }, + }, + }) + Ok(t, err) + } + + When(projectCommandBuilder.BuildPlanCommands(ctx, cmd)).ThenReturn(c.ProjectContexts, nil) + + for i := range c.ProjectContexts { + When(projectCommandRunner.Plan(c.ProjectContexts[i])).ThenReturn(c.ProjectResults[i]) + } + + planCommandRunner.Run(ctx, cmd) + + vcsClient.VerifyWasCalledOnce().CreateComment(Any[models.Repo](), AnyInt(), AnyString(), AnyString()) + + ExpCommitStatus := models.SuccessCommitStatus + if c.ExpVCSApplyStatusSucc != c.ExpVCSApplyStatusTotal { + ExpCommitStatus = models.PendingCommitStatus + } + + commitUpdater.VerifyWasCalledOnce().UpdateCombinedCount( + Any[models.Repo](), + Any[models.PullRequest](), + Eq[models.CommitStatus](ExpCommitStatus), + Eq[command.Name](command.Apply), + Eq(c.ExpVCSApplyStatusSucc), + Eq(c.ExpVCSApplyStatusTotal), + ) + }) + } +}