Improve PR reviewer efficiency through improved label automation

5 views
Skip to first unread message

David Tesar

unread,
Dec 7, 2022, 7:29:39 PM12/7/22
to kubernetes-sig-contribex
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:

  1. PR created and gets auto-assigned to reviewers list in OWNERS file.
  2. Reviewers do initial review and typically either want review from another person and/or have feedback for author to address
  3. Author addresses feedback from reviewer
  4. Reviewer approves
  5. Approvers approve
  6. All tests pass and merge happens

The wasted time occurs in this flow typically at:

  1. Between 2 and 3 - the reviewers keep checking the PRs to see if the author has addressed the feedback
  2. 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.
  3. 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.
  4. 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.

  1. Needs Review - No review has occurred on the PR yet by someone in the reviewer or approver list.
    Automation criteria:
    1. Add: PR creation along with assigning people from reviewers list
    2. Remove: Once the first review happens.
    3. Add: If reviewer then assigns someone, label gets added again (signaling they want another reviewer).
    4. Add: Assuming the review was not approved, then after the author adds another commit after this, the label gets automatically added again.
    5. Remove: Review with approval
  2. Needs Approval - There has been one reviewer who has approved, but the PR has not be approved by approver OWNER.
    Automation criteria:
    1. Add: One review with approval exists, but no approval status is present.
    2. Add: Approval status was present, but removed (i.e. rebased)
    3. Remove: Approval status is present.
  3. Wait on Author - Reviewer has provided feedback author needs to address
    Automation criteria:
    1. Added: First review has happened with a non-approved
    2. Removed: Author has made a single commit after the non-approved review
  4. 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:
    1. 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.
    2. Remove: Author adds new commit to PR
  5. Tests Fail - One or more tests are failing
    Automation criteria:
    1. Add: One or more of the tests have failure
    2. Remove: Label exists and then all tests pass
Reply all
Reply to author
Forward
0 new messages