Use GitHub review API to populate the reviewer(s)

[tgross35]

Just an idea, what if @bors r+ and @bors r=somebody were removed in favor of @bors merge? Then:

• Bors would ensure there is at least one approving review in the GitHub review UI and no outstanding changes requested, consistent with the regular GH requirements (change requests can be dismissed if somebody else reapproves)
• Bors would use all approving reviews to populate the list of reviewers for the commit message (multiple reviewers are somewhat often done now with e.g. @bors r=my-username,somebody-else)
• Whoever commented @bors merge gets listed as the git commit committer

The advantages I see here are:

• The GitHub UI is more familiar to most people outside of rust-lang
• First class support for multiple reviewers, no need for whoever does r+ to find everyone who left a green check or said r=me
• Using the @bors merger as the committer means history will show "authored by Bob, committed by Alice" for some review context, rather than Bors always being the committer. This is some better use of Git flow and shows up nicely in GH's UI

[tgross35]

Something else that could be pretty cool - Bors could add Reviewed-by trailers to the bottom of each commit for everyone who left a check, which are pretty tooling friendly.

Reviewed-by: Reviewer Number1 <[email protected]>
Reviewed-by: Reviewer Number2 <[email protected]>

Git has a built in way to add and (I think) dedupe these, git interpret-trailers --trailer "Reviewed-by: Reviewer Number1 <[email protected]>" --trailer "Reviewed-by: Reviewer Number2 <[email protected]>" commit-message.txt

[Kobzol]

It would be cool if we could make the review workflow more GitHub-native, I agree. One issue with GH approvals is that in rust-lang/rust, everyone with write permissions can approve PRs (and there are a lot of people with write permissions, to enable issue management). But I suppose that the bot could still filter people who have approved the PR, but are also in the r+ list.

[tgross35]

It looks like the "triage" role from GH might allow issue management without being able to approve reviews, would that work? https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/managing-repository-roles/repository-roles-for-an-organization#permissions-for-each-role. I guess filtering would be pretty easy if something more fine grained is needed.

[Kobzol]

Triage is not enough, as you cannot manage labels with it and edit comments/issues. But as I said, this specific thing is not an issue, we can just check through the team permissions if the approvers have the correct rights :)

[TroyKomodo]

One thing that is different with the github review list is the stale reviews. I know github has a nudge for stale reviews however, right now with bors if there is a change in the contents and someone else (not the main reviewer) approves the changes after the main reviewer has approved the PR they typically add the main reviewer as part of the review chain in r+

in this way, they wouldn't be able to add them back because the main reviewer hasn't "approved it".

Just something to be aware of.

Edit: after writing this i thought about rollup prs, typically those dont get reviews and just get merged rust-lang/rust#134414 i suppose a variant of @bors merge self-approve but im not sure.

[tgross35]

One thing that is different with the github review list is the stale reviews. I know github has a nudge for stale reviews however, right now with bors if there is a change in the contents and someone else (not the main reviewer) approves the changes after the main reviewer has approved the PR they typically add the main reviewer as part of the review chain in r+

in this way, they wouldn't be able to add them back because the main reviewer hasn't "approved it".

I think this is configurable right? Looking at one of my repos

It seems like if "dismiss stale pull request approvals" is unchecked then the old reviews should stay around.