And to fill out with your example:
*Describe your PR:* _(copy/paste from changeset comments is encouraged!)_ Delete all DS regexes when a DS is deleted. If the DS regexes are not deleted when a DS is deleted, they wind up as orphans, forever to live in the database, unreferenced. This changeset performs a cascade delete on all DS regexes as part of the DS delete process when DSs are deleted from the api endpoint: DELETE /api/1.3/deliveryservices/:id . Previously, this was done in the TO UI, so this brings the API in line with the old, correct, behaviour. [Where that is copy/pasted straight from the checkin comment.] *If there's no update to CHANGELOG.md, why not?* *Does this break backward compatibility?* No. *Quick Checklist:* - [X] Each commit message tells you everything you need to know about the change, including an issue number if appropriate. (Squashing can help with this.) - [X] This PR does *not* fix a serious security flaw. (Read more: www.apache.org/security ) - [X] Tests are complete. - [X] Docs are updated. (Docs are unchanged, but still correct. This PR brings actual behaviour in line with the docs.) On Wed, Feb 28, 2018 at 10:30 AM, Chris Lemmons <[email protected]> wrote: > What about: > > *Describe your PR:* _(copy/paste from changeset comments is encouraged!)_ > > > > *If there's no update to CHANGELOG.md, why not?* > > *Does this break backward compatibility?* > > *Quick Checklist:* > > - [ ] Each commit message tells you everything you need to know about > the change, including an issue number if appropriate. (Squashing can > help with this.) > - [ ] This PR does *not* fix a serious security flaw. (Read more: > www.apache.org/security ) > - [ ] Tests are complete. > - [ ] Docs are updated. > > ... > > The idea is that the form only asks for information that the reviewer > can't trivially get elsewhere, or is of particular importance. I think > you're right, though: short is more important than detailed. Is there > anything else that could be taken out? > > I do think that "How do I verify this?" isn't a great question for the > PR template. If the answer isn't immediately obvious, then the commit > messages haven't described the feature in enough detail. The PR will > disappear, but the commit messages stick around. A discussion of what > the feature does in the comments of a PR won't be useful to someone > trying to determine what the commit was trying to accomplish a year > down the line when it's unclear whether the behaviour was intended or > accidental. > > On Wed, Feb 28, 2018 at 10:14 AM, Jeremy Mitchell <[email protected]> > wrote: >> Chris, I really wanted the PR template to be less daunting and super short >> and to the point. It's intention is to give a super quick summary of what's >> included in this PR to help the merger... >> >> Example: >> >> #### What does this PR do? Is there a related Github issue? >> >> "See Issue #1245" or "This PR cascade deletes all delivery service regexes >> when a delivery service is deleted" >> >> #### What is the best way to verify this PR? <-- IMO this is really >> important for the merger so I know how to "test" or "verify" the >> functionality. >> >> Hit the DELETE /api/1.3/deliveryservices/:id endpoint and ensure all >> entries in the deliveryservice_regex table are deleted for that delivery >> service. >> >> #### Does your PR include any of the following? >> >> - [ ] Tests >> - [ ] Documentation >> - [X] CHANGELOG.md entry >> >> ^^ I wasn't trying to imply that those last things were required. I just >> wanted to provide a checklist that might be helpful for the contributer and >> the merger. For example, I always for get to look for a CHANGELOG.md >> entry... >> >> Jeremy >> >> >> On Tue, Feb 27, 2018 at 9:47 PM, Chris Lemmons <[email protected]> wrote: >> >>> If there's a relevant GitHub issue, that should be noted in the >>> check-in comment, I think. Same for "what does it do?" I don't usually >>> want to spell out steps for someone to verify my stuff because those >>> are the steps that I took to verify it. The PR is so you can see the >>> things I didn't see. And the commit list will make the presence of >>> tests, documentation, and a changelog entry really obvious. >>> >>> Taking yours and reformatting a bit, what if we did something like this? >>> >>> ... >>> >>> *Describe your PR:* _(copy/paste from changeset comments is encouraged!)_ >>> >>> >>> >>> *Quick Checklist:* >>> >>> - [ ] Each commit message tells you everything you need to know about >>> the change. (Squashing can help with this.) >>> - [ ] If applicable, the commit message mentions the appropriate issue >>> number. >>> - [ ] This PR does *not* fix a serious security flaw. (Read more: >>> www.apache.org/security ) >>> - [ ] Automatic code formatters (like gofmt) have been run. >>> >>> *Tests:* >>> >>> - [ ] Are not necessary. >>> - [ ] Would be helpful, but aren't in this PR. >>> - [ ] Are present, but incomplete. >>> - [ ] Are included. >>> >>> *Doc updates:* >>> >>> - [ ] Are not necessary. >>> - [ ] Would be helpful, but aren't in this PR. >>> - [ ] Are present, but incomplete. >>> - [ ] Are included. >>> >>> *If there's no update to CHANGELOG.md, why not?* >>> >>> *Does this break backward compatibility?* >>> >>> *Is there anyone specific that ought to take a look at this?* >>> >>> ... >>> >>> We want to be friendly to committers, while still getting good >>> information for checking PRs. I could be easily convinced that the >>> "Tests" or "Doc updates" sections in there are too long, but I think >>> it should be clear that a potential committer can offer up some code >>> without hitting 100% on tests, docs, and such. >>> >>> On Tue, Feb 27, 2018 at 1:24 PM, Jeremy Mitchell <[email protected]> >>> wrote: >>> > How about something like this for a PR template? >>> > >>> > #### What does this PR do? Is there a relevant Github issue? >>> > >>> > >>> > #### What is the best way to verify this PR? >>> > >>> > >>> > #### Does your PR include any of the following? >>> > >>> > - [ ] Tests >>> > - [ ] Documentation >>> > - [ ] CHANGELOG.md entry >>> > >>> > On Tue, Feb 27, 2018 at 10:46 AM, Jeremy Mitchell <[email protected] >>> > >>> > wrote: >>> > >>> >> With an issue and/or pr template, we will have 6/6 items checked: >>> >> >>> >> https://github.com/apache/incubator-trafficcontrol/community >>> >> >>> >> I actually think PR templates would be quite helpful. As a >>> >> committer/merger, it would be nice to know what problem the PR is >>> solving >>> >> and how to verify the functionality. A PR template could also help >>> >> contributors ensure that their PRs are complete. I.e. does this PR >>> includes >>> >> tests, documentation, etc. >>> >> >>> >> I'll take a stab at a couple of templates and run them by the group. >>> >> >>> >> Jeremy >>> >> >>> >> On Wed, Jan 31, 2018 at 1:10 PM, Chris Lemmons <[email protected]> >>> wrote: >>> >> >>> >>> I'm +1 on Issue Templates, for sure. I don't know that PR templates >>> >>> are quite as critical, but it might be nice to have a reminder in >>> >>> there about verifying that the changelog is updated if necessary and >>> >>> documentation for new features is present. If the PR Template >>> >>> overwrites the default comment that you get from the commit body, it >>> >>> might be more annoying than valuable, though. >>> >>> >>> >>> I'm also +1 on hiding these particular files in a .github directory. >>> >>> Unlike CONTRIBUTING and README, they don't provide all that much >>> >>> benefit for a new person looking for stuff to read. >>> >>> >>> >>> On Wed, Jan 31, 2018 at 12:17 PM, Durfey, Ryan < >>> [email protected]> >>> >>> wrote: >>> >>> > Always +1 on standardization and consistency >>> >>> > >>> >>> > I still want to circle back and setup project/kanbans for organizing >>> >>> tickets in Github. >>> >>> > >>> >>> > Ryan Durfey M | 303-524-5099 >>> >>> > CDN Support (24x7): 866-405-2993 or [email protected]<mailto: >>> >>> [email protected]> >>> >>> > >>> >>> > From: Dewayne Richardson <[email protected]> >>> >>> > Reply-To: "[email protected]" < >>> >>> [email protected]> >>> >>> > Date: Wednesday, January 31, 2018 at 11:15 AM >>> >>> > To: "[email protected]" < >>> >>> [email protected]> >>> >>> > Subject: Github PR/Issues Format Templates >>> >>> > >>> >>> > I was working through the go-swagger repo and found a bug. I >>> submitted >>> >>> a >>> >>> > new issue and found this interesting approach I think the TC github >>> >>> should >>> >>> > adopt, "Issue and PR Templates". I think the main value here is >>> >>> > consistency in our PRs/Issues and user friendly prompts to say "these >>> >>> are >>> >>> > the data points we need to help you solve your issue". >>> >>> > >>> >>> > Working example: >>> >>> > https://github.com/go-swagger/go-swagger/issues/new >>> >>> > >>> >>> > Github Doc on how to implement templates: >>> >>> > https://github.com/blog/2111-issue-and-pull-request-templates >>> >>> > >>> >>> > If we think it's a good idea, then I'll respond with some examples >>> for >>> >>> > Issues and PR's that we can discuss. >>> >>> > >>> >>> > -Dew >>> >>> > >>> >>> >>> >> >>> >> >>>
