Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mention 2 cpp-reviewer requirement in pull request template #10768

Merged
merged 3 commits into from
May 10, 2022
+4 −0
Merged

Mention 2 cpp-reviewer requirement in pull request template #10768

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93b9642
Mention 2 cpp-reviewer requirement in pull request template
davidwendt May 2, 2022
f6793a0
Merge branch 'branch-22.06' into cpp-2-reviewer-pull-req
davidwendt May 3, 2022
c9f5493
fix misspelling
davidwendt May 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 assigned two cudf-cpp-codeowners reviewers.
Ensure at least two approvals from cudf-cpp-codeowners before merging.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something that the submitter needs to worry about, or is it rather the responsibility of the eventual merger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

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."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
image

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, makes sense given the current merger restrictions.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Many thanks in advance for your cooperation!
-->