Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Do not unnecessarily update apply check if it doesn't exist yet #3747

Merged
merged 8 commits into from
Sep 25, 2023
Merged

fix: Do not unnecessarily update apply check if it doesn't exist yet #3747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
366f484
Do not unnecessarily create apply pipeline if it doesn't exist yet
lukemassa Sep 7, 2023
5d2e19a
Updates
lukemassa Sep 7, 2023
f9e34c1
Fix remaining
lukemassa Sep 7, 2023
31a6b18
Fix test logic
lukemassa Sep 7, 2023
0ec2561
Cleanup more tests
lukemassa Sep 8, 2023
882658e
Fix test
lukemassa Sep 8, 2023
c24d6a5
Merge branch 'main' into do_not_create_unnecessarily_create_apply_pip…
jamengual Sep 21, 2023
d9cbf8b
Merge branch 'main' into do_not_create_unnecessarily_create_apply_pip…
jamengual Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 21 additions & 17 deletions server/events/command_runner_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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)
}
})
}
}
Expand All @@ -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
Expand Down
5 changes: 2 additions & 3 deletions server/events/plan_command_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down
55 changes: 31 additions & 24 deletions server/events/plan_command_runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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",
Expand All @@ -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,
Expand All @@ -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",
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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.",
Expand Down Expand Up @@ -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),
)
}
})
}
}