Skip to content
This repository has been archived by the owner on Jul 21, 2024. It is now read-only.

feat: support multiple PR reviews #223

Merged
merged 6 commits into from
Mar 11, 2020

Conversation

allout58
Copy link
Contributor

@allout58 allout58 commented Feb 20, 2020

Description

Provide support for requiring multiple reviews. Add a "partially-approved" state, which occurs when
the number of approvals is less than the required number (from branch protections) or the number of
reviewers who have been requested but have not submitted an approval

(also includes the fix included in my PR #222, but actually from the CLI so it updates the required tests as well)

Fixes

fixes #194

New Permissions

Requires an update to the requested permissions:

  • read-only access to Administration for branch permissions

Provide support for requiring multiple reviews. Add a "partially-approved" state, which occurs when
the number of approvals is less than the required number (from branch protections) or the number of
reviewers who have been requested but have not submitted an approval

fixes pr-triage#194
Resolve incorrectly using the requested number of reviews. The value
is not the total number of requested reviews, but the number of reviews
remaining. Also, fix a commit issue where the `payload.json` fixture was
not committed along with the tests
@sotayamashita
Copy link
Member

@allout58 Sorry not to replay quickly. I am gonna review your awesome work. Thanks a lot.

Copy link
Member

@sotayamashita sotayamashita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm late and I appreciate you work but I left some comments. Thanks in advance.

lib/defaults.js Show resolved Hide resolved
lib/pr-triage.js Outdated Show resolved Hide resolved
const requiredReviews =
branchProtections.data.required_pull_request_reviews;
return (
(requiredReviews && requiredReviews.required_approving_review_count) || 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it return 1 even if there is no required number of reviews? I would appreciate it if you could tell me the reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a result of this comment on the original issue. Should be easy enough to change to zero, or in the future have some way of configuring the default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allout58 I understand!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multiple PR reviews
2 participants