At the request of the contribex sig meeting today, I'm sending a copy of the text in
this issue out for broader awareness and feedback.
Copy of issue description:Today there is a general challenge of having far more PR
contributions than there are people to review them. Time is wasted for
reviewers to determine which PRs are ready for review. The process today goes
as follows:
- PR
created and gets auto-assigned to reviewers list in OWNERS file.
- Reviewers
do initial review and typically either want review from another person
and/or have feedback for author to address
- Author
addresses feedback from reviewer
- Reviewer
approves
- Approvers
approve
- All
tests pass and merge happens
The wasted time occurs in this flow typically at:
- Between
2 and 3 - the reviewers keep checking the PRs to see if the author has
addressed the feedback
- After
3 - the author may have not caught that the tests are failing or believe
the tests which are failing, so reviewers have to check this out and ping
the author again. Also possible tests fail later after an automated rebase
and author/reviewers aren't aware until manually checking PR.
- After
2 - If anyone requests a second reviewer and reviewers keep coming back to
check the PR to see if 2nd reviewer reviewed the PR.
- After
4 - the reviewer may have approved the PR, but doesn't have approval
permission so author is waiting for an approver to approve/merge.
All of these things result in not only wasted time for
reviewers/approvers, but also delayed time for authors in getting their PRs
merged.
What's the proposed solution to help?
Add 5 additional labels which
are automatically added based on criteria to make it more quickly visible what
state the PRs are in for reviewers/approvers and authors.
- Needs Review - No
review has occurred on the PR yet by someone in the reviewer or approver
list.
Automation criteria:
- Add:
PR creation along with assigning people from reviewers list
- Remove:
Once the first review happens.
- Add:
If reviewer then assigns someone, label gets added again (signaling they
want another reviewer).
- Add:
Assuming the review was not approved, then after the author adds another
commit after this, the label gets automatically added again.
- Remove:
Review with approval
- Needs Approval - There
has been one reviewer who has approved, but the PR has not be approved by
approver OWNER.
Automation criteria:
- Add:
One review with approval exists, but no approval status is present.
- Add:
Approval status was present, but removed (i.e. rebased)
- Remove:
Approval status is present.
- Wait on Author -
Reviewer has provided feedback author needs to address
Automation criteria:
- Added:
First review has happened with a non-approved
- Removed:
Author has made a single commit after the non-approved review
- Abandoned - The PR
has been in "Wait on Author" state for an extended period of
time. Next step here after this is implemented would be to automatically
close PRs after abandoned state label has been present for another
pre-determined time period (i.e. 30 days to label, another 30 days in
abandoned), but this should be in a separate issue.
Automation criteria:
- Add:
PR has been in state "Wait on Author" for 30 days.
Automatically tag the author to prompt them for a status update or close
out the PR if they no longer plan to contribute.
- Remove:
Author adds new commit to PR
- Tests Fail - One or
more tests are failing
Automation criteria:
- Add:
One or more of the tests have failure
- Remove:
Label exists and then all tests pass