Suggest reviewers action #110

Nyholm · 2020-11-08T12:26:18Z

This PR brings two features:

Someone can make a comment in a PR to say @carsonbot find me a reviewer please and a minute later it will reply with a ping to someone.
If a PR is opened an had no comments/reviews in 24(?) hours, then we write a comment with a ping to someone.

This PR works with a Github action that clones the git-reviewer script, checks out the source (symfony/symfony or symfony/symfony-docs) and then runs the git-reviewer. That will produce a group of users that also have modified the changed files in the PR.

I ignore FrameworkBundle, all tests and the Changelogs because they are always changed. I also only looking for contributors for the past 2 years.

When the git-reviewer is done, we run the bin/console app:review:suggest command. It looks at the contributor lists, removes people (core team) from a block list and removes all people that already have interacted with the PR.

Then it just writes a comment.

.github/workflows/find-reviewer.yml

composer.json

config/services.yaml

Nyholm · 2020-11-08T12:28:57Z

src/Api/Issue/StdErrIssueApi.php

+ *
+ * @author Tobias Nyholm <[email protected]>
+ */
+class StdErrIssueApi extends GithubIssueApi


This class is never used. But it was really useful when testing/debugging, so I kept it in the PR.

Nyholm · 2020-11-08T18:27:41Z

This PR is ready for a review

src/Service/ReviewerFilter.php

javiereguiluz · 2020-11-09T18:14:34Z

Tobias, thanks for proposing this feature. I know that some communities have something similar. MY only fear it's whether this will be truly useful to add reviewers ... or it will be just annoying and won't improve the current situation. But, the only way to know it is to try it for real, so I'm 👍 to try this.

About the proposed command -> @carsonbot find me a reviewer please These "textual interfaces" are always complex to use ... so maybe we could design it with very simple commands/orders instead of textually correct phrases. E.g. @carsonbot help, @carsonbot find reviewer

Nyholm · 2020-11-09T18:19:46Z

MY only fear it's whether this will be truly useful to add reviewers

Yeah.. I am not sure.. We will see =)

About the proposed command

I agree. The only hard requirements at the moment is to write @carsonbot and review on the same line. So most phrases will be okey.

@carsonbot review
@carsonbot find reviewer please
@carsonbot get a review
Please @carsonbot fetch a reviewer for me
etc

src/Subscriber/FindReviewerSubscriber.php

.github/workflows/find-reviewer.yml

src/Command/SuggestReviewerCommand.php

Nyholm · 2020-11-10T10:14:46Z

Thank you @stof for the review. I've updated the PR accordingly.

weaverryan · 2020-11-12T13:49:04Z

.github/workflows/find-reviewer.yml

+
+on:
+  repository_dispatch:
+    types: [find-reviewer]


Does this mean this workflow is called each time a find-reviewer webhooks is received? Can you help me understand how this works? thx :)

Exact. Here are some resources about it you could find useful:

https://docs.github.com/en/free-pro-team@latest/actions/reference/events-that-trigger-workflows#repository_dispatch

https://docs.github.com/en/free-pro-team@latest/rest/reference/repos#create-a-repository-dispatch-event

see https://github.com/carsonbot/carsonbot/pull/113/files#diff-761989cba708b0fb2f270ad4d799a7a955b1c611d9cefa6fde84611e03e516c0R36

The corresponding code was added in a separate PR, which is indeed weird.

So I've defined a special event id called find-reviewer. In the GithubPullRequestApi::findReviewer() that @stof linked, I am dispatching this special event.

It is basically me saying: "Hey start the specific CI job called find-reviewer".

weaverryan · 2020-11-12T13:50:53Z

src/Subscriber/FindReviewerSubscriber.php

+        $repository = $event->getRepository();
+
+        // set scheduled task to run in 20 hours
+        $this->scheduler->runLater($repository, $data['number'], Task::ACTION_SUGGEST_REVIEWER, new \DateTimeImmutable('+20hours'));


Ok, so when a new PR is opened, or "ready_for_review", then, 20 hours later, we will run the "suggest reviewer" functionality. 20 hours seems fairly fast to me maybe ?

I also wonder what to do with PRs opened with the "Status: Needs Work" label for whatever reasons (or changed with Carson bot). But that's probably the same as opening a Draft PR; we don't want it?

the status is set to needs work when a reviewer asks for changes. Saying we don't want such PRs would be an issue. It means we only accept PRs when they are perfect at first review or when the contributor can change the PR and re-request a review in less than 20 hours after receiving a review (which is putting a lot of burden on them when they contribute in their free time)

@weaverryan Maybe it is too fast, maybe not. Whenever I make a PR I am secretly and eagerly waiting for some kind of feedback. So 20 hours is suuuper long.

@ogizanagi, I've not been thinking of that.. I should make sure the PR has the correct status.

A user may open a PR, then change the status them selves. If so, we should not call for a review.

I think everyone likes to get a review/comment within a couple hours after opening. However, there is a difference between what we think is reasonable and what we like.

If I think "when does someone start to add a ping or asks on Slack if someone can please take a look at their PR", I think this margin is closer to 5 days - 1 week than to 20 hours.

Should we try with 20 hours for the first 10 pings? Just to see what happens.

Also note that none of us never will be pinged. We will mostly ping symfony contributors that nobody ever pinged directly on github or slack.

FYI: The Rust community has a bot that pings potential reviewers as soon as the PR is open.

https://github.com/rust-lang/rust/pull/78714

Nyholm · 2020-11-12T14:47:45Z

src/Service/TaskHandler/SuggestReviewerHandler.php

+        $status = $this->statusApi->getIssueStatus($task->getNumber(), $repository);
+        if (Status::NEEDS_REVIEW === $status || null === $status) {
+            $this->pullRequestApi->findReviewer($repository, $task->getNumber(), SuggestReviewerCommand::TYPE_SUGGEST);
+        } else {
+            // Try again later
+            $this->scheduler->runLater($repository, $task->getNumber(), Task::ACTION_SUGGEST_REVIEWER, new \DateTimeImmutable('+20hours'));
+        }


@ogizanagi I've added this logic.

If the PR does not have status "needs review" we will try again 20 hours later. Just until somebody else has made a comment.

nicolas-grekas · 2020-12-09T14:08:22Z

src/Service/ReviewerFilter.php

+    private $blocked = [
+            'fabpot', 'tobion', 'nicolas-grekas', 'stof', 'dunglas', 'jakzal',
+            'xabbuh', 'javiereguiluz', 'lyrixx', 'weaverryan', 'chalasr', 'ogizanagi',
+            'sroze', 'yceruto', 'nyholm', 'wouterj', 'derrabus', 'jderusse',


does the bot have access to the mergers team?

Im not sure.

But that would be a great future update to implement.

nicolas-grekas · 2020-12-09T14:12:10Z

Does any project use this logic successfully already?
I'm curious about how relevant this will be.
Also: which problem does it solve? Will this solve something that is actually slowing us down right now? That's a honest question :) I feel like things are pretty smooth, but I'm curious to see how this proposal can improve things.
What's the way to verify this provides value? Enable, then wait and see?

Nyholm · 2020-12-09T14:30:00Z

Does any project use this logic successfully already?

Yes, @javiereguiluz showed some examples from the Ruby community. They have bot that suggest a reviewer when the PR is opened.

which problem does it solve?

Just two very tiny problems.

If a PR dont get any attention, it will try to ping someone
It will ping a person that probably never have been pinged before

What's the way to verify this provides value? Enable, then wait and see?

Yes, Wait and see. Maybe this is a good idea, maybe it is something we disable before Christmas. Let's try something new.

nicolas-grekas

Let's give it a try :)

weaverryan · 2021-01-06T14:18:24Z

Let's see how it works! Thanks very much Tobias!

Nyholm commented Nov 8, 2020

View reviewed changes

Nyholm force-pushed the suggest-reviewer branch 2 times, most recently from f392c4b to 3375cc0 Compare November 8, 2020 13:25

Nyholm mentioned this pull request Nov 8, 2020

Adding PullRequestApi #113

Merged

Nyholm force-pushed the suggest-reviewer branch 2 times, most recently from 9725a9d to c39047f Compare November 8, 2020 18:27

Nyholm commented Nov 9, 2020

View reviewed changes

src/Service/ReviewerFilter.php Show resolved Hide resolved

Nyholm commented Nov 9, 2020

View reviewed changes

src/Subscriber/FindReviewerSubscriber.php Outdated Show resolved Hide resolved

stof reviewed Nov 10, 2020

View reviewed changes

.github/workflows/find-reviewer.yml Show resolved Hide resolved

src/Command/SuggestReviewerCommand.php Outdated Show resolved Hide resolved

src/Command/SuggestReviewerCommand.php Outdated Show resolved Hide resolved

weaverryan reviewed Nov 12, 2020

View reviewed changes

Nyholm commented Nov 12, 2020

View reviewed changes

Nyholm force-pushed the suggest-reviewer branch 3 times, most recently from 4d1db63 to 5ddc389 Compare November 18, 2020 08:00

nicolas-grekas reviewed Dec 9, 2020

View reviewed changes

nicolas-grekas approved these changes Dec 14, 2020

View reviewed changes

Nyholm force-pushed the suggest-reviewer branch from 5ddc389 to 9a09adf Compare December 21, 2020 17:57

Nyholm force-pushed the suggest-reviewer branch 2 times, most recently from 493c6fa to 5daa4d3 Compare January 4, 2021 20:46

Suggest reviewers action

4ba7344

weaverryan force-pushed the suggest-reviewer branch from 5daa4d3 to 4ba7344 Compare January 6, 2021 14:18

weaverryan merged commit e58dc43 into symfony-tools:master Jan 6, 2021

Suggest reviewers action #110

Suggest reviewers action #110

Uh oh!

Conversation

Nyholm commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Nov 8, 2020

Uh oh!

Uh oh!

javiereguiluz commented Nov 9, 2020

Uh oh!

Nyholm commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nyholm commented Nov 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nyholm commented Dec 9, 2020

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Jan 6, 2021

Uh oh!

Uh oh!

Nyholm commented Nov 8, 2020 •

edited

Loading

Nyholm commented Nov 9, 2020 •

edited

Loading

stof Nov 12, 2020 •

edited

Loading

ogizanagi Nov 12, 2020 •

edited

Loading

Nyholm Nov 12, 2020 •

edited

Loading

nicolas-grekas commented Dec 9, 2020 •

edited

Loading