ci: add require multiple reviewers action #204
Conversation
|
|
zlayaAvocado
force-pushed
the
require-two-reviewers
branch
from
736b46f to
9db5eb9
Compare
zlayaAvocado
changed the title
| conclusion = approvals.length < numberOfReviewers ? 'failure' : 'success' | ||
| } | ||
|
|
||
| await octokit.request('POST /repos/{owner}/{repo}/check-runs', { |
There was a problem hiding this comment.
Did you test that this does the right thing in the event that we already left a check for the same commit? eg, if it's triggered from a pull_request and then later a pull_request_review trigger, did you verify that the second one looks like it's updating the original check status (as opposed to leaving a new second status)? What about the annotations - do they get cleared out when posting the updated check, or do they need to be removed explicitly?
(I'd recommend trying this out in a separate test repo and linking the results of your testing in the PR description - I started this in https://github.com/dequelabs/dbjorge-test-repo/pull/25, feel free to use that. You can go to hit "approve" to see what happens in this scenario)
There was a problem hiding this comment.
I'ts going to fail and re-run the Tests / require-multiple-reviewers action
Annotations are warnings and don't go anywhere, they are for the information purpose so a PR author knows what important files are, and I don't think they should be removed.
There was a problem hiding this comment.
I'm fine with deferring this and leaving it as-is for the purposes of this PR. I suspect this is going to feel noisy, but we can adjust that once we see it in action if we need to.
|
Does |
I think with codeowners you need to request specific people if you want a PR to be reviewed and approved more than once, something like That is to my understanding of codeowners though, let me know if I am wrong 😄 |
I agree with your interpretation. GitHub's implementation of CODEOWNERs unfortunately doesn't support requiring multiple reviewers; you can specify a file as being owned by multiple entities, but that results in GitHub requiring a review from any one of those entities (not each of those entities). From GitHub's docs:
CODEOWNERS isn't a particularly well-defined format, and there exist other managed Git platforms that support more advanced CODEOWNERS features (including this). For example, GitLab supports this as part of their CODEOWNERS section syntax. Unfortunately, support for this sort of thing doesn't seem to have gained any traction at GitHub. I think using special comments in CODEOWNERS or something instead of a new file would be a fine interface for this to use, but I didn't suggest it originally because I worried I'd be getting into bikeshedding territory. |
| expect(result).to.equal(2) | ||
| }) | ||
|
|
||
| it('should handle multiple reviews from the same user and count only the latest approval', () => { |
Requires a specified number of reviewers for certain important files, the files are to be provided as an
important-files-pathvar.See
README.mdfor more details https://github.com/dequelabs/axe-api-team-public/pull/204/files#diff-db32ad4c1ae8b5f09def402ee7d7876046a269ae8c7b5bfb42567d703807902bNOTE: this version doesn't account for edge cases like accounting for approvals from people who requested changes, e.g. if a PR requires 2 reviewers, 1 reviewer requested changes but other 2 reviewers approved the PR, the action will pass.
Tests / require-multiple-reviewerscheck that would fail until a required number of reviewers approves a PR that uses the action.Changes requested:
Changes approved:
Annotations:
Ref: https://github.com/dequelabs/axe-linter/issues/1632