I retract my -1. On Tue, Aug 14, 2018 at 9:52 AM, David Neuman <[email protected]> wrote:
> Thanks Rob, > Those are all your translations made to fit your narrative. I have read > that text and I hardly find it "hostile". I don't think we are asking for > anything unreasonable in our contributing.md file but, if you feel that > strongly about it, you are a committer on this project and can help change > the wording of the file. The truth is that we have always been, and will > continue to be, very helpful to new contributors and I can't think of one > situation where we refused to merge the PR of a new contributor unless > they've become completely unresponsive after submitting it. > > >>I say this as the person who has probably has the most PRs recently, and > easily has the most difficulty getting things merged. > > Seems like there is some frustration here. As the person with "the most PRs > recently", it seems like you should be the first one in line to do anything > to help us merge your PRs faster. We aren't suggesting creating this > template because it is cool or fun, we are suggesting adding this template > because we have real issues that we are trying to address. There are too > many times where PRs are submitted with very little or no information that > can be used to figure out how to test it before merging it. This template > at least gives us a starting point and some information to use when trying > to merge PRs. Additionally, this template helps us set expectations with > contributors of what we expect of the code that is getting merged into our > codebase. I don't believe it is unreasonable to ask for tests or > documentation for new code, especially new features. I would hope that > anyone that wants to contribute to our codebase would also want to help > make our project, and code, better by trying to meet some simple standards. > > I don't like the stance of not doing something because of fear, > uncertainty, and doubt. We are trying to make the process better, we are > not trying to sabotage people so that they can never get their PRs merged. > We have already said that we know that all of this template won't > necessarily apply to all changes. > > At the very least we should try this for a few months, see if it helps or > not, and then move on from there. > > -Dave > > > On Tue, Aug 14, 2018 at 9:17 AM Robert Butts <[email protected]> wrote: > > > >> "We need to be less hostile to new people" > > > That is a very serious claim, do you have a specific example? > > > > https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md > > > > > Once your issue has been approved and you're ready to start slinging > code > > Translation: you're not allowed to even write code, until we approve it. > > > > > Please verify any document changes by installing Sphinx > > <http://www.sphinx-doc.org/en/stable/> > > Translation: you're not allowed to make PRs unless you get our docs tool > > set up. Trouble setting it up? New to programming and having > difficulties? > > Sorry, you're not allowed to make a PR. > > > > > Make sure all existing tests pass. > > To say setting up the Traffic Ops "API test" framework is nontrival is an > > understatement. How about the Perl tests? I don't know that I've ever got > > them to run completely correctly. You're not allowed to make a PR until > you > > get those working, or whatever other frameworks apply to your project; > I'm > > not sure we have any test frameworks that aren't incredibly difficult to > > set up, at least for me. > > > > Contributing.md reads like it has multiple-personality disorder, with > lines > > like "We love pull requests!" and then saying in as many words "you're > not > > allowed to even write code, much less submit it, until you do all these > > things, and we authorize it." > > > > > > >> "It's already too difficult to get anything merged" > > > I agree, and this is the exact problem this PR template is attempting > to > > address. > > > > Yes, and I realize it has the best intentions. But I really, really think > > it's going to have the opposite effect: it's going to be used against > > people, and it's going to make things harder to merge. I say this as the > > person who has probably has the most PRs recently, and easily has the > most > > difficulty getting things merged. > > > > > > 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/)> > > > > > - [ ] 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]. > > > apache.org" > > > > < > > > > > > > > >> >>> > >>> [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]. > > > apache.org" > > > > < > > > > > > > > >> >>> > >>> [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 > > > > > > > > >> >>> > >>> > > > > > > > > > >> >>> > >>> > > > > > > > > >> >>> > >> > > > > > > > > >> >>> > >> > > > > > > > > >> >>> > > > > > > > > > >> >>> > > > > > > > > >> >> > > > > > > > > >> >> > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
