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

feat: PR check status to show summary e.g. Plan: 1 to add, 0 to change, 1 to destroy #2699

Closed
wants to merge 3 commits into from
+18 −10
Closed

feat: PR check status to show summary e.g. Plan: 1 to add, 0 to change, 1 to destroy #2699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b2b2a4b
feat: Update plan GitHub Status summary
albertorm95 Nov 20, 2022
dc4b540
feat: revert result changes for Plan and Apply functions
albertorm95 Nov 20, 2022
d6d8977
docs: Add CommandResult description
albertorm95 Nov 20, 2022
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
2 changes: 2 additions & 0 deletions server/events/command/project_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ const (
// be executed for a project.
type ProjectContext struct {
CommandName Name
// CommandResult represent the Terraform outputs after the command execution.
CommandResult ProjectResult
Copy link
Contributor

@krrrr38 krrrr38 Dec 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProjectContext is used for command execution context. So you should not use ProjectContext as a mutable variable like ctx.CommandResult = result.

In this case, it is better to add result ProjectResult arg into SetJobURLWithStatus like SetJobURLWithStatus(ctx command.ProjectContext, cmdName command.Name, status models.CommitStatus, projectResult ProjectResult) error

Copy link
Contributor

@krrrr38 krrrr38 Dec 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently TestProjectOutputWrapper is failed by following reasons.

  • test mock following calls which use same ctx.
	mockJobURLSetter.VerifyWasCalled(Once()).SetJobURLWithStatus(ctx, c.CommandName, models.PendingCommitStatus)
	mockJobURLSetter.VerifyWasCalled(Once()).SetJobURLWithStatus(ctx, c.CommandName, expCommitStatus)
  • actual invocations are used different ctx which has different CommandResult.
       	Actual interactions with this mock were:
        	SetJobURLWithStatus(command.ProjectContext{CommandName:0, CommandResult:command.ProjectResult{Command:0, RepoRelDir:"", Workspace:"", Error:error(nil), Failure:"", PlanSuccess:(*models.PlanSuccess)(nil), PolicyCheckSuccess:(*models.PolicyCheckSuccess)(nil), ApplySuccess:"", VersionSuccess:"", ProjectName:""}, ApplyCmd:"", ApplyRequirements:[]string(nil), AutomergeEnabled:false, ParallelApplyEnabled:false, ParallelPlanEnabled:false, ParallelPolicyCheckEnabled:false, AutoplanEnabled:false, BaseRepo:models.Repo{FullName:"", Owner:"", Name:"", CloneURL:"", SanitizedCloneURL:"", VCSHost:models.VCSHost{Hostname:"", Type:0}}, EscapedCommentArgs:[]string(nil), HeadRepo:models.Repo{FullName:"", Owner:"", Name:"", CloneURL:"", SanitizedCloneURL:"", VCSHost:models.VCSHost{Hostname:"", Type:0}}, Log:(*logging.StructuredLogger)(0xc000636b00), Scope:tally.Scope(nil), PullReqStatus:models.PullReqStatus{ApprovalStatus:models.ApprovalStatus{IsApproved:false, ApprovedBy:"", Date:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC)}, Mergeable:false}, ProjectPlanStatus:0, Pull:models.PullRequest{Num:0, HeadCommit:"", URL:"", HeadBranch:"", BaseBranch:"", Author:"", State:0, BaseRepo:models.Repo{FullName:"", Owner:"", Name:"", CloneURL:"", SanitizedCloneURL:"", VCSHost:models.VCSHost{Hostname:"", Type:0}}}, ProjectName:"", RepoConfigVersion:0, RePlanCmd:"", RepoRelDir:".", Steps:[]valid.Step{valid.Step{StepName:"plan", ExtraArgs:[]string(nil), RunCommand:"", EnvVarName:"", EnvVarValue:""}}, TerraformVersion:(*version.Version)(nil), User:models.User{Username:""}, Verbose:false, Workspace:"default", PolicySets:valid.PolicySets{Version:(*version.Version)(nil), Owners:valid.PolicyOwners{Users:[]string(nil)}, PolicySets:[]valid.PolicySet(nil)}, DeleteSourceBranchOnMerge:false, RepoLocking:false, JobID:"", ExecutionOrderGroup:0}, 1, 0)
        	SetJobURLWithStatus(command.ProjectContext{CommandName:0, CommandResult:command.ProjectResult{Command:0, RepoRelDir:"", Workspace:"", Error:error(nil), Failure:"", PlanSuccess:(*models.PlanSuccess)(0xc0000ae960), PolicyCheckSuccess:(*models.PolicyCheckSuccess)(nil), ApplySuccess:"exists", VersionSuccess:"", ProjectName:""}, ApplyCmd:"", ApplyRequirements:[]string(nil), AutomergeEnabled:false, ParallelApplyEnabled:false, ParallelPlanEnabled:false, ParallelPolicyCheckEnabled:false, AutoplanEnabled:false, BaseRepo:models.Repo{FullName:"", Owner:"", Name:"", CloneURL:"", SanitizedCloneURL:"", VCSHost:models.VCSHost{Hostname:"", Type:0}}, EscapedCommentArgs:[]string(nil), HeadRepo:models.Repo{FullName:"", Owner:"", Name:"", CloneURL:"", SanitizedCloneURL:"", VCSHost:models.VCSHost{Hostname:"", Type:0}}, Log:(*logging.StructuredLogger)(0xc000636b00), Scope:tally.Scope(nil), PullReqStatus:models.PullReqStatus{ApprovalStatus:models.ApprovalStatus{IsApproved:false, ApprovedBy:"", Date:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC)}, Mergeable:false}, ProjectPlanStatus:0, Pull:models.PullRequest{Num:0, HeadCommit:"", URL:"", HeadBranch:"", BaseBranch:"", Author:"", State:0, BaseRepo:models.Repo{FullName:"", Owner:"", Name:"", CloneURL:"", SanitizedCloneURL:"", VCSHost:models.VCSHost{Hostname:"", Type:0}}}, ProjectName:"", RepoConfigVersion:0, RePlanCmd:"", RepoRelDir:".", Steps:[]valid.Step{valid.Step{StepName:"plan", ExtraArgs:[]string(nil), RunCommand:"", EnvVarName:"", EnvVarValue:""}}, TerraformVersion:(*version.Version)(nil), User:models.User{Username:""}, Verbose:false, Workspace:"default", PolicySets:valid.PolicySets{Version:(*version.Version)(nil), Owners:valid.PolicyOwners{Users:[]string(nil)}, PolicySets:[]valid.PolicySet(nil)}, DeleteSourceBranchOnMerge:false, RepoLocking:false, JobID:"", ExecutionOrderGroup:0}, 1, 1)

Mutable variables are difficult to maintain it. I recommend you to change like above comment. @albertorm95

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! @albertorm95 could you find a way to make these tests work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried with that solution, but it gets very complex.

Update SetJobURLWithStatus means to also update UpdateProject which is in a lot of places and makes me to update a lot of test cases, I have no experience with those mocks 😢

cc: @krrrr38 @nitrocode

// ApplyCmd is the command that users should run to apply this plan. If
// this is an apply then this will be empty.
ApplyCmd string
Expand Down
25 changes: 15 additions & 10 deletions server/events/commit_status_updater.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,13 @@ func (d *DefaultCommitStatusUpdater) UpdateCombined(repo models.Repo, pull model
var descripWords string
switch status {
case models.PendingCommitStatus:
descripWords = "in progress..."
descripWords = genProjectStatusDescription(cmdName.String(), "in progress...")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @albertorm95 !

Could you also add test(s) for this to verify the descripWords ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can create one, just that I don't understand my current test fail :(

--- FAIL: TestProjectOutputWrapper

case models.FailedCommitStatus:
descripWords = "failed."
descripWords = genProjectStatusDescription(cmdName.String(), "failed.")
case models.SuccessCommitStatus:
descripWords = "succeeded."
descripWords = genProjectStatusDescription(cmdName.String(), "succeeded.")
}
descrip := fmt.Sprintf("%s %s", cases.Title(language.English).String(cmdName.String()), descripWords)
return d.Client.UpdateStatus(repo, pull, status, src, descrip, "")
return d.Client.UpdateStatus(repo, pull, status, src, descripWords, "")
}

func (d *DefaultCommitStatusUpdater) UpdateCombinedCount(repo models.Repo, pull models.PullRequest, status models.CommitStatus, cmdName command.Name, numSuccess int, numTotal int) error {
Expand Down Expand Up @@ -86,13 +85,19 @@ func (d *DefaultCommitStatusUpdater) UpdateProject(ctx command.ProjectContext, c
var descripWords string
switch status {
case models.PendingCommitStatus:
descripWords = "in progress..."
descripWords = genProjectStatusDescription(cmdName.String(), "in progress...")
case models.FailedCommitStatus:
descripWords = "failed."
descripWords = genProjectStatusDescription(cmdName.String(), "failed.")
case models.SuccessCommitStatus:
descripWords = "succeeded."
if ctx.CommandResult.PlanSuccess != nil {
descripWords = ctx.CommandResult.PlanSuccess.Summary()
} else {
descripWords = genProjectStatusDescription(cmdName.String(), "succeeded.")
}
}
return d.Client.UpdateStatus(ctx.BaseRepo, ctx.Pull, status, src, descripWords, url)
}

descrip := fmt.Sprintf("%s %s", cases.Title(language.English).String(cmdName.String()), descripWords)
return d.Client.UpdateStatus(ctx.BaseRepo, ctx.Pull, status, src, descrip, url)
func genProjectStatusDescription(cmdName, description string) string {
return fmt.Sprintf("%s %s", cases.Title(language.English).String(cmdName), description)
}
1 change: 1 addition & 0 deletions server/events/project_command_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ func (p *ProjectOutputWrapper) updateProjectPRStatus(commandName command.Name, c

// ensures we are differentiating between project level command and overall command
result := execute(ctx)
ctx.CommandResult = result
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

+	ctx.CommandResult = result

This change is making the TestProjectOutputWrapper to fail

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@albertorm95 is this change needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@albertorm95 it's odd that this line is causing the failure because it should not affect the result if statement on line 172. I imagine the code isn't falling into that if block because that is where SetJobURLWithStatus is called.

Perhaps it's a test issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it looks like a test issue

atlantis/server/events/project_command_runner_test.go

Line 230 in 9ca57d6

mockJobURLSetter.VerifyWasCalled(Once()).SetJobURLWithStatus(ctx, c.CommandName, models.PendingCommitStatus)

atlantis/server/events/models/commit_status.go

Lines 22 to 38 in 9ca57d6

const (
PendingCommitStatus CommitStatus = iota
SuccessCommitStatus
FailedCommitStatus
)
func (s CommitStatus) String() string {
switch s {
case PendingCommitStatus:
return "pending"
case SuccessCommitStatus:
return "success"
case FailedCommitStatus:
return "failed"
}
return "failed"
}

Basically, since you changed the status of the pr check, the test still thinks the status should be pending/success/failed.

Please update the test and it should work as expected.

cc: @albertorm95


if result.Error != nil || result.Failure != "" {
if err := p.JobURLSetter.SetJobURLWithStatus(ctx, commandName, models.FailedCommitStatus); err != nil {
Expand Down