Mention 2 cpp-reviewer requirement in pull request template #10768
Conversation
davidwendt
added
3 - Ready for Review
| @@ -47,6 +47,10 @@ Here are some guidelines to help the review process go smoothly. | |||
| If conflicts occur against the target branch they should be resolved by | |||
| merging the target branch into the branch used for making the pull request. | |||
|
|
|||
| 8. Pull requests that modify cpp source that are marked ready for review | |||
| will automatically be assign two cudf-cpp-codeowners reviewers. | |||
| Ensure at least two approvals from cudf-cpp-codeowners before merging. | |||
There was a problem hiding this comment.
Is this something that the submitter needs to worry about, or is it rather the responsibility of the eventual merger?
There was a problem hiding this comment.
I guess it's part of the PR workflow, however, it's easy to forget the instructions in the template (since I guess one deletes it and replaces it with the PR summary). At least for me, in the github web UI it's rather hard to see which approving reviews would match this criterion. e.g. looking at #10761 one has to mouse over the individual usernames to see which groups they belong to (and then figure out if the criteria are met). WDYT?
I wonder if the automerging functionality of @rapids-ops-bot can be augmented to apply the criteria, and then this instruction would say something like: "Pull requests that modify cpp source require at least two approvals from cudf-cpp-codeowners before merge."
There was a problem hiding this comment.
The assignments are automatic although somewhat random. So you will have 2 cpp reviewers assigned if you modify cpp files. You are on your honor to not ignore these and to merge with 2 approvals. This is not something that can be enforced through the merger at this time so I felt it should be at least documented in this template.
Along with the mouse hover, you can see was assigned in the PR stream. For example here is where the assignments occurred in #10761
There was a problem hiding this comment.
Because this rule is specific to C++ developers and not all of the repository, I would recommend putting this in the C++ Developer Guide. I share the concern that the PR template is likely to be missed / deleted.
Additionally, the requirement of two C++ reviews does not affect the actions of an external contributor who is submitting a PR since a RAPIDS developer will be the one responsible for review and merge. I view external contributors as the primary readers of the PR template, since experienced developers on the RAPIDS team probably pay it no attention.
Eventually I would like to minimize the long comments in this PR template and replace it with a checklist style of PR template that gives all contributors actionable items. Examples:
There was a problem hiding this comment.
Thanks, makes sense given the current merger restrictions.
There was a problem hiding this comment.
I opened a draft #10774 for revising the PR template content and would be happy to hear your thoughts. I recognize it conflicts with this PR's changes to the template, so I apologize in advance!
davidwendt
added
doc
Codecov Report
@@ Coverage Diff @@
## branch-22.06 #10768 +/- ##
================================================
+ Coverage 86.40% 86.43% +0.03%
================================================
Files 143 143
Lines 22448 22448
================================================
+ Hits 19396 19403 +7
+ Misses 3052 3045 -7
Continue to review full report at Codecov.
|
|
@gpucibot merge |
Although 2 cpp reviewers are automatically assigned to a PR, there is no documentation that indicates 2 are required. This PR adds to the list of guidelines in the Pull Request Template that appears when a PR is created through the github web page.