[webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Chris Dumez]

> On Nov 28, 2023, at 1:09 PM, Jonathan Bedard via webkit-dev <webkit-dev at lists.webkit.org> wrote:
> 
> 
>> Hi,
>> 
>> Back in the Bugzilla days, only reviewers were allowed to r+ or r- patches. Non-reviewers were - of course - encouraged to do informal reviews but they would do so by leaving comments. They would never r+ / r-.
>> 
>> Since we?ve moved to Github, it seems we have become a lot more lax about this and I have seen non-reviewers approve and reject PRs, not just leaving comments.
>> My understanding is that there is no way to prevent this with Github but could we at least make it a policy that non-reviewers should not approve / reject PRs and only leave comments instead?
> 
> To provide some context, one of the reasons I didn’t attempt to exactly replicate Bugzilla’s behavior when porting our review mechanisms to GitHub was that Bugzilla only allows a single reviewer, which meant that an r+ would naturally clobber an r-, regardless of who gave the r- initially. Even in Bugzilla, non-reviewers could give r+es or r-es, it’s just that Commit-Queue wouldn’t respect them.
> 
>> The reason I would like us to make enforce this rule is that I find it confusing. We have a lot of new comers in the project and I do not always know if a person is a reviewer or not yet. I imagine it may be even more confusing for non-Apple people.
> 
> I agree with this, especially since folks GitHub usernames are not always obvious.
> 
>> I have in some cases not reviewed patches because I had seen the "green check? and thought the PR already had been approved.
>> I have also seen cases of PRs rejected, asking the author to do more work, that I didn?t feel was necessary.
>> There is no easy way from the GitHub UI to tell if the person who approved/rejected your PR is actually a reviewer, as far as I know.
> 
> I think it’s fair for a non-reviewer to “Request Changes” on a PR, it’s often the case that a non-reviewer has comments that should block a change from landing. If a reviewer (or even the PR author) thinks that the blocking comment has been addressed, they can “re-request review” which will get-rid of symbol until the account whose review is requested re-reviews the PR.

I think non-reviewers can suggest changes without rejecting the PR (which I think is what you mean by “Request Changes”). I know from experience that some of those "requested changes” from non-reviewers sometimes go against our usual practices. Again, they are not reviewers yet and this is part of learning.

> 
> I think this also points to the right way to address the confusion for r+es (which I think are the bigger issue): if we formally make our policy that non-reviewers should not leave a persistent “Approved” checkmark on a PR, we could have EWS listen for PR approvals and simply request a re-review any time a non-reviewer approves a PR. In practice, this will mean that a non-reviewer which approves a PR will have their approval nearly instantly removed by a bot. We could even have the bot comment something like “Unofficial r+ from non-reviewer <github user>” to make exactly what’s going on clear.

I’m on board if it also cancels PR rejections from non-reviewers, not just approvals. I don’t see how approvals differ from rejections.

> 
> The issue with a policy change like this one is that it’s WebKit reviewers who deeply understand the policies of the WebKit project, but it’s non-reviewers who need to change their behavior. If we don’t back this policy clarification with a tools change, I think non-reviewers are unlikely to be aware project policy has changed.

I think it is great if we can enforce it with tools, better in fact. However, I’ll say that things were going fine on bugzilla with a simple clear policy (no r+ / r- from non-reviewers), even though it wasn’t enforced by the tools.
This only became an issue after the change to GitHub (maybe because the UI encourages it). We also had a lot of newcomers to the project around the same time, who were not necessarily familiar with how we were doing things on Bugzilla since they’ve mostly experienced Github.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20231128/29074d65/attachment.htm>