Skip to content

Commit

Permalink
fix: Do not unnecessarily update apply check if it doesn't exist yet (#…
Browse files Browse the repository at this point in the history 
…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 <[email protected]>
  • Loading branch information
@lukemassa @jamengual
lukemassa and jamengual authored Sep 25, 2023
1 parent c35ba0d commit fad6f0f
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 44 deletions.
38 changes: 21 additions & 17 deletions server/events/command_runner_internal_test.go
View file
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
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
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),
)
}
})
}
}

0 comments on commit fad6f0f

Please sign in to comment.