Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?
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 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. 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.
What do you think?
Thanks, Chris Dumez.
On Nov 28, 2023, at 1:09 PM, Jonathan Bedard via webkit-dev <webkit-dev@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.
On Tue, Nov 28 2023 at 01:23:12 PM -0800, Chris Dumez via webkit-dev <webkit-dev@lists.webkit.org> wrote:
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.
Sure. It doesn't really matter whether rejections are canceled or not, because the important part of the rejection is the comments that were added, not the rejection status itself. A rejection from a non-reviewer is not effective anyway, so it's fine to have a bot clarify that. Michael
FYI, our official documentation on WebKit.org <http://webkit.org/> says: ``` Making unofficial reviews before you become a reviewer is encouraged. This is an excellent way to show your skills. Note that you should not put r+ nor r- on patches in such unofficial reviews. ``` I guess this wan’t updated after the move to GitHub. For me, no r+ or r- on bugzilla translates to no approve / deny PRs on GitHub. So I simply wish we’d start enforcing this policy again. Having the tools help us would be great but I don’t think it stops us from enforcing our own policies like we used to.
On Nov 28, 2023, at 1:58 PM, Michael Catanzaro <mcatanzaro@redhat.com> wrote:
On Tue, Nov 28 2023 at 01:23:12 PM -0800, Chris Dumez via webkit-dev <webkit-dev@lists.webkit.org> wrote:
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.
Sure. It doesn't really matter whether rejections are canceled or not, because the important part of the rejection is the comments that were added, not the rejection status itself. A rejection from a non-reviewer is not effective anyway, so it's fine to have a bot clarify that.
Michael
Hi
On 29 Nov 2023, at 9:44 am, Chris Dumez via webkit-dev <webkit-dev@lists.webkit.org> wrote:
FYI, our official documentation on WebKit.org <http://webkit.org/> says: ``` Making unofficial reviews before you become a reviewer is encouraged. This is an excellent way to show your skills. Note that you should not put r+ nor r- on patches in such unofficial reviews. ``` I guess this wan’t updated after the move to GitHub. For me, no r+ or r- on bugzilla translates to no approve / deny PRs on GitHub. So I simply wish we’d start enforcing this policy again.
Having the tools help us would be great but I don’t think it stops us from enforcing our own policies like we used to.
Personally, I’ve been requesting non-official reviewers to review my patches because I know that their skill set is perfectly matched (and it will help make them official reviewer) Having them giving r+ explicitly is, I find, easier to spot than looking through the often busy GitHub page to find the comments. Could we relax the ability to give informal r+ review to people with commit rights? (And it’s also great to be able to provide stats later to say see, that person did XX informal reviews :) ) Jean-Yves
On Nov 28, 2023, at 4:06 PM, Jean-Yves Avenard <jean-yves.avenard@apple.com> wrote:
Hi
On 29 Nov 2023, at 9:44 am, Chris Dumez via webkit-dev <webkit-dev@lists.webkit.org> wrote:
FYI, our official documentation on WebKit.org <http://webkit.org/> says: ``` Making unofficial reviews before you become a reviewer is encouraged. This is an excellent way to show your skills. Note that you should not put r+ nor r- on patches in such unofficial reviews. ``` I guess this wan’t updated after the move to GitHub. For me, no r+ or r- on bugzilla translates to no approve / deny PRs on GitHub. So I simply wish we’d start enforcing this policy again.
Having the tools help us would be great but I don’t think it stops us from enforcing our own policies like we used to.
Personally, I’ve been requesting non-official reviewers to review my patches because I know that their skill set is perfectly matched (and it will help make them official reviewer)
Having them giving r+ explicitly is, I find, easier to spot than looking through the often busy GitHub page to find the comments.
Even if someone approves, you’d still need to find the comments. r+ with comments is super common.
Could we relax the ability to give informal r+ review to people with commit rights?
They can just leave a comment saying the patch looks good. They don’t have to approve the PR. Approving the PR means nothing since it won’t let you merge your PR. People are expected to wait for an official review to trigger the merge queue, we’re currently making it harder than it needs to be if we have an official review or not.
I agree with Chris that we don’t need to invent something new. Instead we should restore the very good system we already had. Where non-reviewers could comment but that’s not a review, and so should not do review+ or review-. Technical enforcement was not required before and is not required now either. But a tool that helps explain the policy would be better. — Darin
participants (6)
-
Chris Dumez
-
Darin Adler
-
Jean-Yves Avenard
-
Jonathan Bedard
-
Michael Catanzaro
-
Ryosuke Niwa