+1 The template is just a reminder list. It won't apply to "all" PR's, for example most type of bugs probably won't have documentation, but we need to improve the PR's quality so that they are easier to merge.
-Dew On Tue, Aug 14, 2018 at 8:56 AM Dan Kirkwood <[email protected]> wrote: > +1 > > As a new contributor to other open source projects, I welcomed having > something in front of me to explicitly show what the committers expected to > see. > > As a committer, it helps me to take a more targeted approach at examining > a PR, which should help to streamline the process of getting things > merged. > > -dan > > On Tue, Aug 14, 2018 at 8:38 AM Dave Neuman <[email protected]> wrote: > > > Just because it can be used as a hammer doesn't mean it will be used as a > > hammer. We need to do a better job of providing good tests and > > documentation with our code, and I think this list is a reminder of that. > > There are times where a PR is justified in not having tests (like > > documentation) or docs (like a bug fix) and, in those cases, you can > simply > > not check the check box. Just because we don't like to do something > > doesn't mean it isn't the right thing to do. > > > > >> "It's already too difficult to get anything merged" > > I agree, and this is the exact problem this PR template is attempting to > > address. Often times we get PRs with hundreds of lines of code that > have a > > poor description, no tests, and no documentation. It is hard to expect > the > > community to jump right in, "trust the developer", and merge something > > without at least understanding what the PR does and what problem it is > > trying to solve. This template is meant to help us merge PRs more > > efficiently. > > > > >> "We need to be less hostile to new people" > > That is a very serious claim, do you have a specific example? We should > be > > welcoming to all new contributors and, in my experience, we have. If you > > have a specific example I would love to see it so that I can work with > the > > rest of the PMC to ensure it doesn't happen again. > > > > In my opinion, this template is the balance between getting things done > and > > doing things the right way. PR templates are pretty standard in the open > > source community -- as are requiring tests and documentation for that > > matter -- and I would love to see this adopted sooner rather than later. > > In my opinion this makes our project better. > > > > Thanks, > > Dave > > > > > > > > > > On Tue, Aug 14, 2018 at 8:18 AM Robert Butts <[email protected]> wrote: > > > > > > #### What is the best way to verify this PR? > > > > > > +1 > > > > > > > #### Check any that apply > > > > > > > > - [ ] This PR does *NOT* fix a serious security flaw. Read > > more: [ > > > www.apache.org/security](http://www.apache.org/security/) > <http://www.apache.org/security%5D(http://www.apache.org/security/)> > > <http://www.apache.org/security%5D(http://www.apache.org/security/)> > > > <http://www.apache.org/security%5D(http://www.apache.org/security/)> > > > <http://www.apache.org/security%5D(http://www.apache.org/security/)> > > > > - [ ] This PR includes tests > > > > - [ ] This PR includes documentation updates > > > > - [ ] This PR includes an update to CHANGELOG.md > > > > > > -1 > > > > > > Yes , those are all good things. But in practice, that list will be > used > > as > > > a hammer to beat people over the head, and refuse to merge PRs without > > > complete tests and docs. > > > > > > It's already too difficult to get anything merged, and our contributing > > > pages are already too hostile. We need to be less hostile to new > people, > > > and make PRs less difficult to merge, not more. > > > > > > Yes, all those things are great, complete tests and docs would be > great, > > > but that has to be balanced with getting things done, and being > welcoming > > > to new people. > > > > > > > > > On Tue, Aug 14, 2018 at 8:06 AM, Jeremy Mitchell < > [email protected]> > > > wrote: > > > > > > > thanks dave. i'll add that to the PR. > > > > > > > > > > > > > > > > On Tue, Aug 14, 2018 at 7:44 AM, David Neuman < > > [email protected]> > > > > wrote: > > > > > > > > > Looks good Jeremy. Only nit I would pick would be to add > > documentation > > > > to > > > > > your list of components. > > > > > > > > > > Thanks, > > > > > Dave > > > > > > > > > > On Mon, Aug 13, 2018 at 8:13 PM Jeremy Mitchell < > > [email protected] > > > > > > > > > wrote: > > > > > > > > > > > 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 < > [email protected] > > > > > > > > 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 < > [email protected] > > > > > > > > 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 < > > > [email protected] > > > > > > > > > > > > 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 < > > > [email protected] > > > > > > > > > > > > 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 < > > > > [email protected] > > > > > > > > > > > > > >> wrote: > > > > > > > >> > > > > > > > > >> >> +1 on keeping it short and to the point... > > > > > > > >> >> > > > > > > > >> >> 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 > > > > > > > >> >>> > >>> > > > > > > > > >> >>> > >>> > > > > > > > >> >>> > >> > > > > > > > >> >>> > >> > > > > > > > >> >>> > > > > > > > > >> >>> > > > > > > > >> >> > > > > > > > >> >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Aug 14, 2018 at 8:06 AM, Jeremy Mitchell < > [email protected]> > > > wrote: > > > > > > > thanks dave. i'll add that to the PR. > > > > > > > > > > > > > > > > On Tue, Aug 14, 2018 at 7:44 AM, David Neuman < > > [email protected]> > > > > wrote: > > > > > > > > > Looks good Jeremy. Only nit I would pick would be to add > > documentation > > > > to > > > > > your list of components. > > > > > > > > > > Thanks, > > > > > Dave > > > > > > > > > > On Mon, Aug 13, 2018 at 8:13 PM Jeremy Mitchell < > > [email protected] > > > > > > > > > wrote: > > > > > > > > > > > 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 < > [email protected] > > > > > > > > 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 < > [email protected] > > > > > > > > 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 < > > > [email protected] > > > > > > > > > > > > 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 < > > > [email protected] > > > > > > > > > > > > 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 < > > > > [email protected] > > > > > > > > > > > > > >> wrote: > > > > > > > >> > > > > > > > > >> >> +1 on keeping it short and to the point... > > > > > > > >> >> > > > > > > > >> >> 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 > > > > > > > >> >>> > >>> > > > > > > > > >> >>> > >>> > > > > > > > >> >>> > >> > > > > > > > >> >>> > >> > > > > > > > >> >>> > > > > > > > > >> >>> > > > > > > > >> >> > > > > > > > >> >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > >
