twisted workflow addition: "I accept the review"

chevah / github-hooks-server

Handling of GitHub hooks for Chevah project

BSD 3-Clause "New" or "Revised" License

1 stars 1 forks source link

twisted workflow addition: "I accept the review" #55

Open glyph opened 1 year ago

glyph commented 1 year ago

Before we fully migrated to Github in the Tracocalypse, we had an additional process rule: any contributor with commit access could accept a passing review from someone outside the project, if they deemed that the reviewer in question was operating in good faith and following the process sufficiently that it should count.

The main thing we need members of the project to do is code review, so getting this practice and earning this trust gradually from project members seems like an important step to me and it's weird that Github doesn't have something built-in. Anyway.

Right now, a few admins (mostly just me, probably) still follow this part of the process by using the admin-only red merge button, but it would be nice if the robot could understand the magic phrase "I accept the review" to get an official passing checkmark from the robot iff there is a grey checkmark review (passing from non-member).

adiroiban commented 1 year ago

So the requirement is something like this.

The PR is created by a member of Twisted team
The review is done by a non-member of Twisted team.
The non-member accepts the PR
GitHub will not allow the non-member to do a review
chevah-robot will act as a "surrogate" and will perform the "Approve" action using GitHub API (on behalf of the non-member)
The review approve comment can contain a text informing that the review was approved in the name of user X

If the PR is not created by a member of Twisted team, and a non-member tries to approve it, nothing will happen (an info comment saying that "We don't trust yet your review" can be nice)

adiroiban commented 1 year ago

Ok. So it looks like non-team member can do a review ... but maybe that review from a non-team member is not accepted by the branch protection... which I hope it's the case

danuker commented 6 months ago

Branches should be protected - not a lot of people can commit to a repository; unless maybe auto-merge is enabled. chevah-robot will not merge anything by itself.

Whoever has merge access could check, and "I accept the review" could mean the same as the act of merging. Is that adequate?

(an info comment saying that "We don't trust yet your review" can be nice)

If the above solution is good enough, then I think this is what's left of this ticket. It would be nice to get a message that the approval was from someone outside the contributor team.

glyph commented 6 months ago

Whoever has merge access could check, and "I accept the review" could mean the same as the act of merging. Is that adequate?

I guess if we move into the "needs-merge" state, reviewers should be checking that queue as well