feat: merge and fail should update check run status #876

zolamk · 2023-05-14T11:19:06Z

Description

In order to support merge policies the $merge and $fail built-ins need to be able to set the status of the check run, this pull request updates reviewpad so that it receives a check run id that the built-ins can use to control the status of the check.

`🤖 Generated by Copilot at b5f2400`

This pull request adds support for GitHub check runs for reviewpad policies. It modifies the lang/aladino package and its plugins to handle check run IDs, conclusions, and outputs in the interpreter and the built-in functions. It also updates the runner.go file and the engine package to pass and return the check run information. It adjusts the test files and the cli package to reflect the changes in the function signatures.

Closes #874

Code review and merge strategy

Ask: this pull request requires a code review before merge

How

`🤖 Generated by Copilot at b5f2400`

Add support for GitHub check runs for reviewpad policies (link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link)
Add a new parameter checkRunId to the Run function signature in the runner.go file and pass it to the runReviewpadFile and runReviewpadCommand functions (link, link)
Add a new parameter checkRunId to the runReviewpadFile function signature in the runner.go file and pass it to the NewInterpreter function in the lang/aladino package (link)
Add a new parameter checkRunId to the runReviewpadCommand function signature in the runner.go file and pass it to the runReviewpadCommandDryRun function (link)
Add a new parameter checkRunId to the runReviewpadCommandDryRun function signature in the runner.go file and pass it to the reviewpad.Run function in the cli package (link)
Add a new parameter checkRunId to the reviewpad.Run function signature in the cli package and pass it to the NewInterpreter function in the engine package (link)
Add a new parameter checkRunId to the NewInterpreter function signature in the engine package and pass it to the NewEvalEnv function in the lang/aladino package (link)
Add a new parameter checkRunId to the NewEvalEnv function signature in the lang/aladino package and assign it to the CheckRunID field of the BaseEnv struct (link, link)
Add two new fields CheckRunID and CheckRunConclusion to the BaseEnv struct in the lang/aladino package to store the check run ID and conclusion that correspond to the aladino interpreter's execution (link)
Add three new methods GetCheckRunID, SetCheckRunConclusion, and GetCheckRunConclusion to the Env interface in the lang/aladino package to access and modify the check run ID and conclusion (link)
Implement the GetCheckRunID, SetCheckRunConclusion, and GetCheckRunConclusion methods for the BaseEnv struct in the lang/aladino package (link, link)
Implement the GetCheckRunConclusion method for the Interpreter struct in the lang/aladino package to return the check run conclusion from the aladino environment (link)
Add a new method GetCheckRunConclusion to the Interpreter interface in the engine package to return the conclusion of the check run that corresponds to the interpreter's execution (link)
Modify the runReviewpadFile function in the runner.go file to return the check run conclusion from the aladino interpreter as the third value (link, link, link, link, link, link, link)
Modify the runReviewpadCommandDryRun function in the runner.go file to return an empty string as the third value, which corresponds to the check run conclusion (link, link, link)
Modify the runReviewpadCommand function in the runner.go file to return an empty string as the third value, which corresponds to the check run conclusion (link, link, link)
Remove the code that checks for fatal errors in the aladino environment and stops the execution in the Run method of the Interpreter struct in the lang/aladino package, as the fatal errors are handled by the fail built-in function (link)
Modify the failCode function in the plugins/aladino/actions/fail.go file to set the check run conclusion to "failure" and update the check run status and output with the fatal messages from the aladino environment, if the check run ID is not nil (link)
Modify the mergeCode function in the plugins/aladino/actions/merge.go file to set the check run conclusion to "success" and update the check run status and output with a success message, if the check run ID is not nil and there are no fatal messages from the aladino environment (link)
Add new imports to the plugins/aladino/actions/fail.go and plugins/aladino/actions/merge.go files for the new logic that updates the check run status and output (link, link)
Add a new variable targetEntity to the mergeCode function in the plugins/aladino/actions/merge.go file to hold the target entity information for the check run output (link)
Add a new argument nil to the NewInterpreter function calls in the test functions in the lang/aladino package, which corresponds to the checkRunID parameter that was added to the NewInterpreter function signature (link, link, link)
Add a new argument nil to the NewEvalEnv function calls in the test functions in the lang/aladino package, which corresponds to the checkRunID parameter that was added to the NewEvalEnv function signature (link, link, link, link)
Add a new argument nil to the NewInterpreter function call in the mockEnvWith function in the lang/aladino package, which corresponds to the checkRunID parameter that was added to the NewInterpreter function signature (link)
Add a new argument nil to the mockAladinoInterpreter function call in the engine package, which corresponds to the checkRunId parameter that was added to the NewInterpreter function signature (link)

ferreiratiago

Thanks for this @zolamk

Can you please share what was the rationale for providing Reviewpad with the check id instead of the check itself?

My understanding was to provide the check itself as an interface that could provide the required methods to update it. The proposed way binds the check to GitHub and disables the ability to easily work for different code hosts.

What is your understanding of this?

zolamk · 2023-05-16T06:08:52Z

Thanks for this @zolamk

Can you please share what was the rationale for providing Reviewpad with the check id instead of the check itself?

My understanding was to provide the check itself as an interface that could provide the required methods to update it. The proposed way binds the check to GitHub and disables the ability to easily work for different code hosts.

What is your understanding of this?

The check run id is an integer both in github and gitlabs case, that's why I am passing the id to reviewpad and then the host service could implement a set of calls to work with check runs

reviewpad

Some checks are failing. Please fix them.

shay2025 · 2023-05-16T15:19:33Z

plugins/aladino/actions/merge.go

+			Status:     github.String("completed"),
+			Conclusion: github.String("success"),
+			Output: &github.CheckRunOutput{
+				Title:   github.String("Reviewpad about to merge"),


What about Reviewpad Merge Notification?

@fmallet what do you think the title of the check run output should when the merge built-in is about to merge the pull request

plugins/aladino/actions/merge.go

reviewpad · 2023-05-16T17:10:32Z

Pull request is not up to date with the base branch. Reviewpad will rebase it. After the rebase, please trigger Reviewpad by running the command /reviewpad run in a comment.

zolamk · 2023-05-16T17:30:59Z

/reviewpad run

marcelosousa · 2023-05-17T12:57:53Z

/reviewpad summarize

reviewpad · 2023-05-17T12:59:00Z

/reviewpad summarize

@marcelosousa an error occurred running your command

Errors:

summarize failed - please contact us on Discord

reviewpad

The pull request has git conflicts. Please fix them.

reviewpad

Pull request is not rebaseable. Please rebase it manually.

reviewpad

Pull request is not rebaseable. Please rebase it manually.

reviewpad · 2023-05-18T08:35:51Z

Pull request is not up to date with the base branch. Reviewpad will rebase it. After the rebase, please trigger Reviewpad by running the command /reviewpad run in a comment.

reviewpad · 2023-05-18T08:38:20Z

Pull request is not up to date with the base branch. Reviewpad will rebase it. After the rebase, please trigger Reviewpad by running the command /reviewpad run in a comment.

…tatus

ferreiratiago · 2023-05-18T09:37:21Z

plugins/aladino/actions/failCheckStatus.go

+	if e.GetCheckRunID() != nil {
+		e.SetCheckRunConclusion("failure")
+
+		summary := strings.Builder{}
+
+		for _, msg := range e.GetBuiltInsReportedMessages()[aladino.SEVERITY_FAIL] {
+			summary.WriteString(fmt.Sprintf("- %s\n", msg))
+		}
+
+		_, _, err := e.GetGithubClient().GetClientREST().Checks.UpdateCheckRun(e.GetCtx(), targetEntity.Owner, targetEntity.Repo, *e.GetCheckRunID(), github.UpdateCheckRunOptions{
+			Name:       "reviewpad",
+			Status:     github.String("completed"),
+			Conclusion: github.String("failure"),
+			Output: &github.CheckRunOutput{
+				Title:   github.String("Reviewpad policy failed"),
+				Summary: github.String(summary.String()),
+			},
+		})
+		return err
+	}


/reviewpad explain

This code checks if the current environment has a non-nil CheckRunID. If it does, it sets the check run conclusion to "failure" and constructs a summary of all the reported messages with a severity of "fail." Then, using the available GithubClient, it updates the check run with the required info, such as name, status, conclusion, and output. This section is responsible for updating the check run on the Github platform to reflect the policy failure and its reason.

ferreiratiago · 2023-05-18T09:37:47Z

plugins/aladino/actions/merge.go

@@ -23,6 +24,7 @@ func Merge() *aladino.BuiltInAction {

 func mergeCode(e aladino.Env, args []aladino.Value) error {


/reviewpad explain

This line defines a function called mergeCode with two parameters, an aladino.Env object named e and a slice of aladino.Value objects named args. The function returns an error value.

The mergeCode function is responsible for performing a merge in the context of a built-in action handler in the Reviewpad application. It fetches the target entity and checks the status of the pull request. If it's not open or is a draft, the function logs a message and returns early with a nil error. The function then parses the merge method from the provided arguments and checks if there are any failures reported by built-in checks. If all conditions are met, the function updates the check-run with a success status and proceeds to perform the merge using the desired merge method, logging a warning message if the merge fails.

ferreiratiago · 2023-05-18T09:38:07Z

runner.go

 	}

 	defer config.CleanupPluginConfig()

-	aladinoInterpreter, err := aladino.NewInterpreter(ctx, log, dryRun, gitHubClient, codeHostClient, collector, targetEntity, eventDetails.Payload, plugins_aladino.PluginBuiltInsWithConfig(config))
+	aladinoInterpreter, err := aladino.NewInterpreter(ctx, log, dryRun, gitHubClient, codeHostClient, collector, targetEntity, eventDetails.Payload, plugins_aladino.PluginBuiltInsWithConfig(config), checkRunId)


/reviewpad explain

This line creates a new instance of the Aladino interpreter by calling the aladino.NewInterpreter function and passing in several arguments which include the context, logger, dry-run flag, GitHub client, code host client, collector, target entity, payload from the event details, built-ins from the plugin configuration, and the check run ID. The Aladino interpreter is responsible for executing a given program and reporting the results.

zolamk · 2023-05-18T10:23:57Z

/reviewpad run

ferreiratiago · 2023-05-18T16:27:21Z

/reviewpad run

reviewpad · 2023-05-18T16:28:49Z

The merge is blocked by the label do-not-merge.

reviewpad

Pull request is ready to be merged. Reviewpad will merge it for you.

reviewpad bot added the ask Pull request requires a code review before merge label May 14, 2023

zolamk marked this pull request as ready for review May 15, 2023 09:16

reviewpad bot added large large pull request critical Modifications to critical changes plugins Modifications to the plugins directory labels May 15, 2023

reviewpad bot assigned zolamk May 15, 2023

reviewpad bot requested a review from shay2025 May 15, 2023 09:17

ferreiratiago requested changes May 15, 2023

View reviewed changes

reviewpad bot added requires-author-attention Pull request requires the author to take action waiting-approval Pull request is waiting for approval labels May 15, 2023

zolamk added the do-not-merge Pull request cannot be merged label May 16, 2023

reviewpad bot requested changes May 16, 2023

View reviewed changes

zolamk removed the do-not-merge Pull request cannot be merged label May 16, 2023

shay2025 reviewed May 16, 2023

View reviewed changes

reviewpad bot force-pushed the feat/merge-fail-check branch from 7199b51 to 93e88e3 Compare May 16, 2023 17:10

zolamk added the do-not-merge Pull request cannot be merged label May 17, 2023

zolamk marked this pull request as draft May 17, 2023 08:44

zolamk force-pushed the feat/merge-fail-check branch from 93e88e3 to f74f055 Compare May 17, 2023 10:42

zolamk marked this pull request as ready for review May 17, 2023 14:10

reviewpad bot requested changes May 17, 2023

View reviewed changes

zolamk force-pushed the feat/merge-fail-check branch from 510a095 to 6035364 Compare May 17, 2023 17:52

zolamk added 14 commits May 18, 2023 12:35

fix: merge check

cdc18da

feat: log merge failures

955082e

feat: fail should stack messages in summary

1e9c50f

fix: lint error

c859f1c

test: fix intergration tests

e6fa311

test: fail no longer fails reviewpad

b82474c

revert: intergration test tag

43b1b14

fix: set title and summary on merge check run update

98a16c4

fix: merge issue

9df5353

feat: use a new built-in to fail status checks

1559dcd

test: fix integration test

3aec673

fix: create proper summary

e9bbdd5

fix: check if fail status check was called before setting check run s…

93eb6d4

…tatus

docs: add comment to fail check status built-in

a2ce912

zolamk force-pushed the feat/merge-fail-check branch from 7c8e15b to a2ce912 Compare May 18, 2023 09:35

ferreiratiago reviewed May 18, 2023

View reviewed changes

zolamk added 4 commits May 18, 2023 14:08

fix: don't report fail messages

0aaa3a7

feat: add header to fail status check summary

63022bb

refactor: reduce header size

94c0afe

refactor: remove fail messages completely before building report

b2fa02c

reviewpad deleted a comment from reviewpad bot May 18, 2023

reviewpad bot removed the waiting-approval Pull request is waiting for approval label May 18, 2023

zolamk removed the do-not-merge Pull request cannot be merged label May 18, 2023

reviewpad bot approved these changes May 18, 2023

View reviewed changes

reviewpad bot merged commit 77f1653 into main May 18, 2023

reviewpad bot deleted the feat/merge-fail-check branch May 18, 2023 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: merge and fail should update check run status #876

feat: merge and fail should update check run status #876

zolamk commented May 14, 2023 •

edited by ghost

Loading

ferreiratiago left a comment

zolamk commented May 16, 2023

reviewpad bot left a comment

shay2025 May 16, 2023

zolamk May 17, 2023

reviewpad bot commented May 16, 2023

zolamk commented May 16, 2023

marcelosousa commented May 17, 2023

reviewpad bot commented May 17, 2023

reviewpad bot left a comment

reviewpad bot left a comment

reviewpad bot left a comment

reviewpad bot commented May 18, 2023

reviewpad bot commented May 18, 2023

ferreiratiago May 18, 2023

reviewpad bot May 18, 2023

ferreiratiago May 18, 2023

reviewpad bot May 18, 2023

ferreiratiago May 18, 2023

reviewpad bot May 18, 2023

zolamk commented May 18, 2023

ferreiratiago commented May 18, 2023

reviewpad bot commented May 18, 2023

reviewpad bot left a comment

		@@ -23,6 +24,7 @@ func Merge() *aladino.BuiltInAction {

		func mergeCode(e aladino.Env, args []aladino.Value) error {

feat: merge and fail should update check run status #876

feat: merge and fail should update check run status #876

Conversation

zolamk commented May 14, 2023 • edited by ghost Loading

Description

🤖 Generated by Copilot at b5f2400

Code review and merge strategy

How

🤖 Generated by Copilot at b5f2400

ferreiratiago left a comment

Choose a reason for hiding this comment

zolamk commented May 16, 2023

reviewpad bot left a comment

Choose a reason for hiding this comment

shay2025 May 16, 2023

Choose a reason for hiding this comment

zolamk May 17, 2023

Choose a reason for hiding this comment

reviewpad bot commented May 16, 2023

zolamk commented May 16, 2023

marcelosousa commented May 17, 2023

reviewpad bot commented May 17, 2023

reviewpad bot left a comment

Choose a reason for hiding this comment

reviewpad bot left a comment

Choose a reason for hiding this comment

reviewpad bot left a comment

Choose a reason for hiding this comment

reviewpad bot commented May 18, 2023

reviewpad bot commented May 18, 2023

ferreiratiago May 18, 2023

Choose a reason for hiding this comment

reviewpad bot May 18, 2023

Choose a reason for hiding this comment

ferreiratiago May 18, 2023

Choose a reason for hiding this comment

reviewpad bot May 18, 2023

Choose a reason for hiding this comment

ferreiratiago May 18, 2023

Choose a reason for hiding this comment

reviewpad bot May 18, 2023

Choose a reason for hiding this comment

zolamk commented May 18, 2023

ferreiratiago commented May 18, 2023

reviewpad bot commented May 18, 2023

reviewpad bot left a comment

Choose a reason for hiding this comment

zolamk commented May 14, 2023 •

edited by ghost

Loading

`🤖 Generated by Copilot at b5f2400`

`🤖 Generated by Copilot at b5f2400`