From 366f484608ef174a79eedaaab27b9e5507848052 Mon Sep 17 00:00:00 2001 From: Luke Massa Date: Wed, 6 Sep 2023 21:44:16 -0400 Subject: [PATCH 1/6] Do not unnecessarily create apply pipeline if it doesn't exist yet --- server/events/plan_command_runner.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) 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 } } From 5d2e19a7a31c1ed83051600f6798d3766873501d Mon Sep 17 00:00:00 2001 From: Luke Massa Date: Thu, 7 Sep 2023 00:56:03 -0400 Subject: [PATCH 2/6] Updates --- server/events/plan_command_runner_test.go | 38 +++++++++++++++-------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/server/events/plan_command_runner_test.go b/server/events/plan_command_runner_test.go index e765f434c5..555d9d9c44 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,6 +538,7 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { }, }, }, + DoNotUpdateApply: true, ExpVCSApplyStatusTotal: 1, ExpVCSApplyStatusSucc: 0, }, @@ -561,7 +563,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, @@ -603,7 +605,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, @@ -626,7 +628,7 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { 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.", + 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, @@ -748,15 +750,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(Times(0)).UpdateCombinedCount( + Any[models.Repo](), + Any[models.PullRequest](), + Eq[models.CommitStatus](ExpCommitStatus), + 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), + ) + } }) } } From f9e34c166b008499e4a356c2df4b50e4e78f3adb Mon Sep 17 00:00:00 2001 From: Luke Massa Date: Thu, 7 Sep 2023 01:06:08 -0400 Subject: [PATCH 3/6] Fix remaining --- server/events/command_runner_internal_test.go | 8 ++++---- server/events/plan_command_runner_test.go | 3 +++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/server/events/command_runner_internal_test.go b/server/events/command_runner_internal_test.go index 4cf3076a0f..6540167a5a 100644 --- a/server/events/command_runner_internal_test.go +++ b/server/events/command_runner_internal_test.go @@ -201,8 +201,8 @@ func TestPlanUpdateApplyCommitStatus(t *testing.T) { }, }, expStatus: models.PendingCommitStatus, - expNumSuccess: 1, - expNumTotal: 2, + expNumSuccess: 0, + expNumTotal: 0, }, "one plan, one apply, one plan success with no changes": { cmd: command.Apply, @@ -220,8 +220,8 @@ func TestPlanUpdateApplyCommitStatus(t *testing.T) { }, }, expStatus: models.PendingCommitStatus, - expNumSuccess: 2, - expNumTotal: 3, + expNumSuccess: 0, + expNumTotal: 0, }, "one apply error, one apply, one plan success with no changes": { cmd: command.Apply, diff --git a/server/events/plan_command_runner_test.go b/server/events/plan_command_runner_test.go index 555d9d9c44..7a4e77e7d6 100644 --- a/server/events/plan_command_runner_test.go +++ b/server/events/plan_command_runner_test.go @@ -579,6 +579,7 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { }, }, }, + DoNotUpdateApply: true, PrevPlanStored: true, ExpVCSApplyStatusTotal: 2, ExpVCSApplyStatusSucc: 1, @@ -623,6 +624,7 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { }, }, }, + DoNotUpdateApply: true, PrevPlanStored: true, ExpVCSApplyStatusTotal: 1, ExpVCSApplyStatusSucc: 0, @@ -657,6 +659,7 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { }, }, }, + DoNotUpdateApply: true, PrevPlanStored: true, ExpVCSApplyStatusTotal: 2, ExpVCSApplyStatusSucc: 1, From 31a6b18f382111ab226ea0ec329d2defcf84f985 Mon Sep 17 00:00:00 2001 From: Luke Massa Date: Thu, 7 Sep 2023 10:31:36 -0400 Subject: [PATCH 4/6] Fix test logic --- server/events/command_runner_internal_test.go | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/server/events/command_runner_internal_test.go b/server/events/command_runner_internal_test.go index 6540167a5a..17cd96b824 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: 0, - expNumTotal: 0, + doNotCallUpdateApply: true, }, "one plan, one apply, one plan success with no changes": { cmd: command.Apply, @@ -219,9 +218,8 @@ func TestPlanUpdateApplyCommitStatus(t *testing.T) { }, }, }, - expStatus: models.PendingCommitStatus, - expNumSuccess: 0, - expNumTotal: 0, + expStatus: models.PendingCommitStatus, + doNotCallUpdateApply: true, }, "one apply error, one apply, one plan success with no changes": { cmd: command.Apply, @@ -251,12 +249,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 +271,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 From 0ec25612cd1899dd7f289bfb210fe1f9005dae72 Mon Sep 17 00:00:00 2001 From: Luke Massa Date: Fri, 8 Sep 2023 13:26:01 -0400 Subject: [PATCH 5/6] Cleanup more tests --- server/events/command_runner_internal_test.go | 1 - server/events/plan_command_runner_test.go | 26 +++++++------------ 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/server/events/command_runner_internal_test.go b/server/events/command_runner_internal_test.go index 17cd96b824..f80c718802 100644 --- a/server/events/command_runner_internal_test.go +++ b/server/events/command_runner_internal_test.go @@ -218,7 +218,6 @@ func TestPlanUpdateApplyCommitStatus(t *testing.T) { }, }, }, - expStatus: models.PendingCommitStatus, doNotCallUpdateApply: true, }, "one apply error, one apply, one plan success with no changes": { diff --git a/server/events/plan_command_runner_test.go b/server/events/plan_command_runner_test.go index 7a4e77e7d6..e5a339afd4 100644 --- a/server/events/plan_command_runner_test.go +++ b/server/events/plan_command_runner_test.go @@ -538,9 +538,7 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { }, }, }, - DoNotUpdateApply: true, - ExpVCSApplyStatusTotal: 1, - ExpVCSApplyStatusSucc: 0, + DoNotUpdateApply: true, }, { Description: "When planning with no changes, set the 1/1 apply status", @@ -579,10 +577,8 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { }, }, }, - DoNotUpdateApply: true, - 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", @@ -624,10 +620,8 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { }, }, }, - DoNotUpdateApply: true, - 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' do not set the apply status.", @@ -659,10 +653,8 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { }, }, }, - DoNotUpdateApply: true, - 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.", @@ -754,10 +746,10 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { ExpCommitStatus = models.PendingCommitStatus } if c.DoNotUpdateApply { - commitUpdater.VerifyWasCalled(Times(0)).UpdateCombinedCount( + commitUpdater.VerifyWasCalled(Times(1)).UpdateCombinedCount( Any[models.Repo](), Any[models.PullRequest](), - Eq[models.CommitStatus](ExpCommitStatus), + Any[models.CommitStatus](), Eq[command.Name](command.Apply), AnyInt(), AnyInt(), From 882658e7f4ca95ab809a05e5169d00fe07b14030 Mon Sep 17 00:00:00 2001 From: Luke Massa Date: Fri, 8 Sep 2023 14:40:04 -0400 Subject: [PATCH 6/6] Fix test --- server/events/plan_command_runner_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/events/plan_command_runner_test.go b/server/events/plan_command_runner_test.go index e5a339afd4..8cc9f6c851 100644 --- a/server/events/plan_command_runner_test.go +++ b/server/events/plan_command_runner_test.go @@ -746,7 +746,7 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { ExpCommitStatus = models.PendingCommitStatus } if c.DoNotUpdateApply { - commitUpdater.VerifyWasCalled(Times(1)).UpdateCombinedCount( + commitUpdater.VerifyWasCalled(Never()).UpdateCombinedCount( Any[models.Repo](), Any[models.PullRequest](), Any[models.CommitStatus](),