-
Notifications
You must be signed in to change notification settings - Fork 169
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.
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
clarify checklist in PR template #8466
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8466 +/- ##
==========================================
+ Coverage 56.38% 56.54% +0.16%
==========================================
Files 387 387
Lines 38716 38785 +69
==========================================
+ Hits 21830 21932 +102
+ Misses 16886 16853 -33 ☔ View full report in Codecov by Sentry. |
.github/pull_request_template.md
Outdated
@@ -8,12 +8,15 @@ Closes # | |||
<!-- describe the changes comprising this PR here --> | |||
This PR addresses ... | |||
|
|||
**Checklist for maintainers** | |||
**Checklist for PR authors** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We sometimes get PRs from developers external to the organization who don't have permission to set labels or milestones. But if maintainers
was confusing I agree it should be changed. May be qualify it with
Checklist for PR authors (skip items if you don't have permissions):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we split this into 2 checklists? 1 for items that require permissions which I think are:
- milestones
- regression tests
- labels (although these should get set automatically)
and one for the PR author (the remaining items).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I modified the wording based on your suggestion to hopefully resolve this comment (see db6f096):
**Checklist for PR authors (skip items if you don't have permissions or they are not applicable)**
@nden hopefully this doesn't change your approval. If all looks good to you and @hbushouse feel free to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me.
This PR changes
maintainers
toPR authors
in the PR template to clarify who should fill out the checklist. I never thought it was me :)This PR also adds a new item to the checklist:
Checklist for maintainers
[ ] added entry inCHANGES.rst
within the relevant release section[ ] updated or added relevant tests[ ] updated relevant documentation[ ] ran regression tests, post a link to the Jenkins job below.How to run regression tests on a PR
[ ] Make sure the JIRA ticket is resolved properly