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

albertorm95 · 2022-11-20T22:07:46Z

what

Plan contains no changes.

Plan: 1 to add, 0 to change, 0 to destroy

why

references

Closes Instead of PR check status of Plan Succeeded show Plan: 1 to add, 0 to change, 1 to destroy #2625
Closes Summarize changes #1267
Partially implements Feat: Make "No changes" stand out more + add summary of number of (un)changed projects #2069

nitrocode · 2022-11-20T22:17:27Z

server/events/commit_status_updater.go

@@ -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...")


Nice work @albertorm95 !

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

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

--- FAIL: TestProjectOutputWrapper

jamengual · 2022-11-20T23:15:37Z

is this using the checks api?

…

On Sun, Nov 20, 2022, 3:13 p.m. Alberto Rojas ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In server/events/commit_status_updater.go <#2699 (comment)> : > @@ -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...") Sure, I can create one, just that I don't understand my current test fail :( --- FAIL: TestProjectOutputWrapper — Reply to this email directly, view it on GitHub <#2699 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ3ERFF6VUT7NLEP4OLI23WJKWBFANCNFSM6AAAAAASGBUB44> . You are receiving this because your review was requested.Message ID: ***@***.***>

albertorm95 · 2022-11-20T23:17:59Z

is this using the checks api?

sorry @jamengual, what do you mean with that?

jamengual · 2022-11-20T23:27:14Z

I might be confused, @nitrocode was this the same. issue you opened, or there was another one to implement the checks api (like spacelift checks)

albertorm95 · 2022-11-20T23:33:01Z

server/events/project_command_runner.go

@@ -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


+ ctx.CommandResult = result

This change is making the TestProjectOutputWrapper to fail

@albertorm95 is this change needed?

@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?

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

nitrocode · 2022-11-21T00:28:49Z

@jamengual yes that was a separate issue. The ChecksAPI is specific for the github app and would move the PR comments to the Github native ChecksAPI page.

This change will affect both Github App and non Github App since it will impact the status checks. Hopefully the future ChecksAPI implementation in the other issue will take advantage of the updated status messages from this PR. :)

albertorm95 · 2022-11-21T00:38:53Z

I am not sure how to comply with the TestProjectOutputWrapper test-case 😞

nitrocode · 2022-11-21T03:35:13Z

I'm unsure on how to fix that failure. The error is saying that SetJobURLWithStatus should have been called at least once but wasn't called at all.

After that one is fixed, you will have to update TestUpdateCombined in server/events/commit_status_updater_test.go because the expected status messages will be different with this change.

krrrr38 · 2022-12-17T04:56:32Z

server/events/command/project_context.go

@@ -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


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

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

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

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

inkel · 2023-01-03T16:57:09Z

👋🏽 this looks like a really nice to have feature, what's the status of this? I'm asking because after the work done (pending merge) in #2907 I've been exploring some options to have better information passed on to the templates, in order to be able to override them with more useful stats, and I've a local branch that adds a stats structure to models.PlanSuccess, and I'd like to know if it's ok to continue exploring that path or wait for this to be done and piggyback on these changes. Thanks!

nitrocode · 2023-01-03T20:39:04Z

Unfortunately, i believe it's stalled. We're always looking for contributors. Please feel free if you can help guide the pr author or please feel free to take over the PR or create a new one to get this over the line

inkel · 2023-01-03T20:55:34Z

@nitrocode thanks for the quick response. I'll see what I can do, as this one seem to be focused on using the checks APIs, whereas my local branch is mostly focused on the rendered templates for commenting.

…-status feat: PR check status to show summary e.g. `Plan: 1 to add, 0 to change, 1 to destroy`

albertorm95 requested a review from a team as a code owner November 20, 2022 22:07

albertorm95 force-pushed the feat-change-plan-status branch from 741a94e to ea7be17 Compare November 20, 2022 22:14

nitrocode reviewed Nov 20, 2022

View reviewed changes

nitrocode added the waiting-on-response Waiting for a response from the user label Nov 20, 2022

albertorm95 commented Nov 20, 2022

View reviewed changes

nitrocode added needs tests Change requires tests help wanted Good feature for contributors labels Nov 25, 2022

nitrocode mentioned this pull request Dec 12, 2022

feat: Add pre and post workflow hook status #2441

Merged

krrrr38 reviewed Dec 17, 2022

View reviewed changes

nitrocode added the unmaintained Looking for community to build upon label Dec 19, 2022

albertorm95 added 3 commits December 19, 2022 08:52

feat: Update plan GitHub Status summary

b2b2a4b

feat: revert result changes for Plan and Apply functions

dc4b540

docs: Add CommandResult description

d6d8977

albertorm95 force-pushed the feat-change-plan-status branch from a0f29b2 to d6d8977 Compare December 19, 2022 15:52

nitrocode marked this pull request as draft December 19, 2022 15:55

nitrocode added work-in-progress and removed unmaintained Looking for community to build upon help wanted Good feature for contributors labels Dec 19, 2022

nitrocode changed the title ~~Instead of PR check status of Plan Succeeded show Plan: 1 to add, 0 to change, 1 to destroy~~ feat: PR check status to show summary e.g. Plan: 1 to add, 0 to change, 1 to destroy Dec 23, 2022

nitrocode added this to the 0.22.1 milestone Dec 23, 2022

nitrocode removed this from the 0.22.2 milestone Jan 6, 2023

nitrocode modified the milestones: 0.22.3, 0.23.0 Jan 6, 2023

krrrr38 added a commit to krrrr38/atlantis that referenced this pull request Jan 14, 2023

Merge pull request runatlantis#2699 from albertorm95/feat-change-plan…

e1103b6

…-status feat: PR check status to show summary e.g. `Plan: 1 to add, 0 to change, 1 to destroy`

krrrr38 mentioned this pull request Jan 14, 2023

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

Merged

1 task

nitrocode closed this in #2983 Jan 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

albertorm95 commented Nov 20, 2022 •

edited by nitrocode

Loading

nitrocode Nov 20, 2022

albertorm95 Nov 20, 2022

jamengual commented Nov 20, 2022 via email

albertorm95 commented Nov 20, 2022

jamengual commented Nov 20, 2022

albertorm95 Nov 20, 2022

nitrocode Nov 21, 2022

nitrocode Dec 11, 2022

nitrocode Dec 11, 2022

nitrocode commented Nov 21, 2022

albertorm95 commented Nov 21, 2022

nitrocode commented Nov 21, 2022 •

edited

Loading

krrrr38 Dec 17, 2022 •

edited

Loading

krrrr38 Dec 17, 2022 •

edited

Loading

nitrocode Dec 19, 2022

albertorm95 Dec 19, 2022

inkel commented Jan 3, 2023

nitrocode commented Jan 3, 2023

inkel commented Jan 3, 2023 •

edited

Loading

	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"
	}

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

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

Conversation

albertorm95 commented Nov 20, 2022 • edited by nitrocode Loading

what

why

references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamengual commented Nov 20, 2022 via email

albertorm95 commented Nov 20, 2022

jamengual commented Nov 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitrocode commented Nov 21, 2022

albertorm95 commented Nov 21, 2022

nitrocode commented Nov 21, 2022 • edited Loading

krrrr38 Dec 17, 2022 • edited Loading

Choose a reason for hiding this comment

krrrr38 Dec 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inkel commented Jan 3, 2023

nitrocode commented Jan 3, 2023

inkel commented Jan 3, 2023 • edited Loading

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

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

albertorm95 commented Nov 20, 2022 •

edited by nitrocode

Loading

nitrocode commented Nov 21, 2022 •

edited

Loading

krrrr38 Dec 17, 2022 •

edited

Loading

krrrr38 Dec 17, 2022 •

edited

Loading

inkel commented Jan 3, 2023 •

edited

Loading