From eb67cc6f5ab1bd594f713c28ff2fc6b61cec0356 Mon Sep 17 00:00:00 2001 From: Luke Massa Date: Mon, 25 Sep 2023 12:01:16 -0400 Subject: [PATCH] fix: Do not unnecessarily update apply check if it doesn't exist yet (#3747) * Do not unnecessarily create apply pipeline if it doesn't exist yet * Updates * Fix remaining * Fix test logic * Cleanup more tests * Fix test --------- Co-authored-by: PePe Amengual --- server/events/command_runner_internal_test.go | 38 +++++++------ server/events/plan_command_runner.go | 5 +- server/events/plan_command_runner_test.go | 55 +++++++++++-------- 3 files changed, 54 insertions(+), 44 deletions(-) diff --git a/server/events/command_runner_internal_test.go b/server/events/command_runner_internal_test.go index 4cf3076a0f..f80c718802 100644 --- a/server/events/command_runner_internal_test.go +++ b/server/events/command_runner_internal_test.go @@ -166,11 +166,12 @@ func TestPlanUpdatePlanCommitStatus(t *testing.T) { func TestPlanUpdateApplyCommitStatus(t *testing.T) { cases := map[string]struct { - cmd command.Name - pullStatus models.PullStatus - expStatus models.CommitStatus - expNumSuccess int - expNumTotal int + cmd command.Name + pullStatus models.PullStatus + expStatus models.CommitStatus + doNotCallUpdateApply bool // In certain situations, we don't expect updateCommitStatus to call the underlying commitStatusUpdater code at all + expNumSuccess int + expNumTotal int }{ "all plans success with no changes": { cmd: command.Apply, @@ -200,9 +201,7 @@ func TestPlanUpdateApplyCommitStatus(t *testing.T) { }, }, }, - expStatus: models.PendingCommitStatus, - expNumSuccess: 1, - expNumTotal: 2, + doNotCallUpdateApply: true, }, "one plan, one apply, one plan success with no changes": { cmd: command.Apply, @@ -219,9 +218,7 @@ func TestPlanUpdateApplyCommitStatus(t *testing.T) { }, }, }, - expStatus: models.PendingCommitStatus, - expNumSuccess: 2, - expNumTotal: 3, + doNotCallUpdateApply: true, }, "one apply error, one apply, one plan success with no changes": { cmd: command.Apply, @@ -251,12 +248,17 @@ func TestPlanUpdateApplyCommitStatus(t *testing.T) { 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) - Equals(t, c.cmd, csu.CalledCommand) - Equals(t, c.expNumSuccess, csu.CalledNumSuccess) - Equals(t, c.expNumTotal, csu.CalledNumTotal) + if c.doNotCallUpdateApply { + Equals(t, csu.Called, false) + } else { + Equals(t, csu.Called, true) + 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) + } }) } } @@ -268,9 +270,11 @@ type MockCSU struct { CalledCommand command.Name CalledNumSuccess int CalledNumTotal int + Called bool } func (m *MockCSU) UpdateCombinedCount(repo models.Repo, pull models.PullRequest, status models.CommitStatus, command command.Name, numSuccess int, numTotal int) error { + m.Called = true m.CalledRepo = repo m.CalledPull = pull m.CalledStatus = status diff --git a/server/events/plan_command_runner.go b/server/events/plan_command_runner.go index 9313f14d4c..8563cee13e 100644 --- a/server/events/plan_command_runner.go +++ b/server/events/plan_command_runner.go @@ -315,9 +315,8 @@ func (p *PlanCommandRunner) updateCommitStatus(ctx *command.Context, pullStatus 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 there are plans that haven't been applied yet, no need to update the status + return } } diff --git a/server/events/plan_command_runner_test.go b/server/events/plan_command_runner_test.go index e765f434c5..8cc9f6c851 100644 --- a/server/events/plan_command_runner_test.go +++ b/server/events/plan_command_runner_test.go @@ -517,11 +517,12 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { ProjectContexts []command.ProjectContext ProjectResults []command.ProjectResult PrevPlanStored bool // stores a previous "No changes" plan in the backend + DoNotUpdateApply bool // certain circumtances we want to skip the call to update apply ExpVCSApplyStatusTotal int ExpVCSApplyStatusSucc int }{ { - Description: "When planning with changes, set the 0/1 apply status", + Description: "When planning with changes, do not change the apply status", ProjectContexts: []command.ProjectContext{ { CommandName: command.Plan, @@ -537,8 +538,7 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { }, }, }, - ExpVCSApplyStatusTotal: 1, - ExpVCSApplyStatusSucc: 0, + DoNotUpdateApply: true, }, { Description: "When planning with no changes, set the 1/1 apply status", @@ -561,7 +561,7 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { ExpVCSApplyStatusSucc: 1, }, { - Description: "When planning with no changes and previous plan with no changes, set the 1/2 apply status", + Description: "When planning with no changes and previous plan with no changes do not set the apply status", ProjectContexts: []command.ProjectContext{ { CommandName: command.Plan, @@ -577,9 +577,8 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { }, }, }, - PrevPlanStored: true, - ExpVCSApplyStatusTotal: 2, - ExpVCSApplyStatusSucc: 1, + DoNotUpdateApply: true, + PrevPlanStored: true, }, { Description: "When planning with no changes and previous 'No changes' plan, set the 2/2 apply status", @@ -603,7 +602,7 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { ExpVCSApplyStatusSucc: 2, }, { - Description: "When planning again with changes following a previous 'No changes' plan, set the 0/1 apply status", + Description: "When planning again with changes following a previous 'No changes' plan do not set the apply status", ProjectContexts: []command.ProjectContext{ { CommandName: command.Plan, @@ -621,12 +620,11 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { }, }, }, - PrevPlanStored: true, - ExpVCSApplyStatusTotal: 1, - ExpVCSApplyStatusSucc: 0, + DoNotUpdateApply: true, + PrevPlanStored: true, }, { - Description: "When planning again with changes following a previous 'No changes' plan, while another plan with 'No changes', set the 1/2 apply status.", + Description: "When planning again with changes following a previous 'No changes' plan, while another plan with 'No changes' do not set the apply status.", ProjectContexts: []command.ProjectContext{ { CommandName: command.Plan, @@ -655,9 +653,8 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { }, }, }, - PrevPlanStored: true, - ExpVCSApplyStatusTotal: 2, - ExpVCSApplyStatusSucc: 1, + DoNotUpdateApply: true, + PrevPlanStored: true, }, { 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.", @@ -748,15 +745,25 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { 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), - ) + if c.DoNotUpdateApply { + commitUpdater.VerifyWasCalled(Never()).UpdateCombinedCount( + Any[models.Repo](), + Any[models.PullRequest](), + Any[models.CommitStatus](), + Eq[command.Name](command.Apply), + AnyInt(), + AnyInt(), + ) + } else { + commitUpdater.VerifyWasCalledOnce().UpdateCombinedCount( + Any[models.Repo](), + Any[models.PullRequest](), + Eq[models.CommitStatus](ExpCommitStatus), + Eq[command.Name](command.Apply), + Eq(c.ExpVCSApplyStatusSucc), + Eq(c.ExpVCSApplyStatusTotal), + ) + } }) } }