I've created a very simple PR template based on some input from this email thread. The PR is found here: https://github.com/apache/trafficcontrol/pull/2673
My goal was 3-fold: 1. keep it simple 2. capture enough info to help the PR merger and increase the odds of a PR getting merged in a timely fashion 3. provide a checklist or a reminder for the submitter Jeremy On Wed, Feb 28, 2018 at 6:38 PM, Chris Lemmons <alfic...@gmail.com> wrote: > > Unfortunately, some PRs contain many commits and even with good commit > messages, it's may be hard to figure out what exactly all the commits are > trying to accomplish. > > This is a solid point and it's part of the point I'm making. If you've > got a bunch of commits that aren't explained well, then squash them > and explain them well. > > > I can't tell you how many times I've had to read through commit messages > and/or code to try to figure out what a PR is trying to accomplish and how > to verify it. > > I totally agree. But putting the exposition exclusively in the PR > means that the reviewer can understand it (a good thing!), but it > leaves that information behind. It's quite difficult to find the PR in > which a particular changeset was merged (especially given our > cherry-picking strategy), so it's much better if we encourage people > to put that information in the commit itself. > > > "Perfect is the enemy of good". > > Sure, though I'm not sure we're quite there. It seems we both agree on > basically which information should be included, I just want some of it > in the commit message instead. > > > I'm just trying to save the reviewer/merger some time. > > Agreed. The purpose of the template is to make sure the PR opener > communicates information that will be most useful and time-saving to > the reviewer. Likewise, it should save the PR-opener time by a) saving > a few keystrokes and b) keeping them from having to come back and > answer questions about stuff they forgot to mention. > > What if it looked like this? > > *Describe your PR:* _(copy/paste from changeset comments is encouraged!)_ > > *What's a good way to verify this PR?* > > *Check any that apply:* > > - [ ] This PR does *not* fix a serious security flaw. (Read more: > www.apache.org/security ) > - [ ] CHANGELOG.md is updated. > - [ ] Tests are complete. > - [ ] Docs are updated. > > ... > > I think we might be asking for information that would be better gotten > elsewhere, but I might have a minority opinion on that point. > > On Wed, Feb 28, 2018 at 6:00 PM, Dan Kirkwood <dang...@gmail.com> wrote: > > Chris, > > > > "Perfect is the enemy of good". The point here is to make the PR > > description good enough to work with, not perfect. I appreciate what > > you're trying to get to, but from experience, it's not realistic. > > > > -dan > > > > On Wed, Feb 28, 2018 at 1:31 PM, Chris Lemmons <alfic...@gmail.com> > wrote: > > > >> `What effect should I expect to see from this change?` > >> > >> This is a perfect question, and one that absolutely needs to be > >> answered. But either > >> > >> a) it is answered already in the commit message; or > >> b) the commit message is insufficient and needs to be `git -amend`ed. > >> > >> I definitely wouldn't want a PR that contained that information only > >> in the PR body, > >> and there's not a whole lot of value in asking them to re-type it. The > >> "copy/paste" thing > >> at the top is already a bit of duplication anyway. > >> > >> On Wed, Feb 28, 2018 at 11:03 AM, Dan Kirkwood <dang...@gmail.com> > wrote: > >> > followup: rather than > >> > `What is the best way to verify this PR? ` > >> > > >> > what about > >> > > >> > `What effect should I expect to see from this change?` > >> > > >> > On Wed, Feb 28, 2018 at 10:25 AM, Dan Kirkwood <dang...@gmail.com> > >> wrote: > >> > > >> >> +1 on keeping it short and to the point... > >> >> > >> >> On Wed, Feb 28, 2018 at 10:14 AM, Jeremy Mitchell < > >> mitchell...@gmail.com> > >> >> 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 <alfic...@gmail.com> > >> >>> 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 < > >> mitchell...@gmail.com > >> >>> > > >> >>> > 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 < > >> >>> mitchell...@gmail.com > >> >>> > > > >> >>> > > 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 < > >> alfic...@gmail.com> > >> >>> > 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 < > >> >>> > ryan_dur...@comcast.com> > >> >>> > >>> 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 cdn_supp...@comcast.com > >> >>> <mailto: > >> >>> > >>> cdn_supp...@comcast.com> > >> >>> > >>> > > >> >>> > >>> > From: Dewayne Richardson <dewr...@gmail.com> > >> >>> > >>> > Reply-To: "d...@trafficcontrol.incubator.apache.org" < > >> >>> > >>> d...@trafficcontrol.incubator.apache.org> > >> >>> > >>> > Date: Wednesday, January 31, 2018 at 11:15 AM > >> >>> > >>> > To: "d...@trafficcontrol.incubator.apache.org" < > >> >>> > >>> d...@trafficcontrol.incubator.apache.org> > >> >>> > >>> > 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 > >> >>> > >>> > > >> >>> > >>> > >> >>> > >> > >> >>> > >> > >> >>> > > >> >>> > >> >> > >> >> > >> >
