Sorry I got lost this thread. I think we agree abort guiding PR title. I have some more suggestions:
- target branch: it should be 'master' unless the patch is only affect on that branch version line - link to DEVELOPER.md: to remind how to contribute - tests (build passed on local? tested manually?): I saw some pull requests even fail to compile. Reviewers shouldn't spend their time regarding this - guide about issue description: I guess JIRA doesn't provide issue template for free, so there is only place to guide or even enforce description of the issue or pull request What do you think? Do you have more ideas to add? - Jungtaek Lim (HeartSaVioR) 2017년 1월 21일 (토) 오전 1:00, Hugo Da Cruz Louro <[email protected]>님이 작성: > Agree! My suggestions are not all or nothing. They are a set of ideas of > which we can pick some. In the cases of multiple commits, I would suggest > that we try to make it that all of them start with the same title > STORM-XXX, such that it’s easily eye catching. That’s what lead to my > suggestion of having a default token like STORM-MINOR when there is no JIRA > associated with the patch, such that we can “group” changes and easily > track them down. However, if this is too much of a requirement, I am OK > with skipping it. > > Hugo > > > On Jan 19, 2017, at 9:00 PM, Harsha Chintalapani <[email protected]> wrote: > > > > I am ok with adopting templates but lets keep this process simpler. > > We've new contributors coming in and they probably didn't have chance to > go > > through process. > > We can guide them through the process or have strict template that > everyone > > needs to adopt to. > > I don't think having guidelines such as having exactly 1 space > after > > the description is helpful to anyone. Its just a PR as long as the title > > reflects the JIRA title and > > JIRA is descriptive enough we all understand whats the patch intends do. > > We shouldn't be going through loops for opening a PR with > > restrictions such as above. > > Hugo, > > "STORM-XXX: Title matching exactly the JIRA Summary (title)" this > > looks good to me. But having description details is hard to follow and > > unnecessary. > > regarding Squashing guidelines, we've gone through this discussion and I > > think this should be left to author discretion as they would understand > > what commits are more important. > > Also as reviewers one can suggest to squash the commits if additional > > commits doesn't make sense. > > > > Thanks, > > Harsha > > > > On Thu, Jan 19, 2017 at 10:29 AM Hugo Da Cruz Louro < > [email protected]> > > wrote: > > > >> I am strongly +1. In addition to the templates suggested by Jungtaek, in > >> the past I suggested the template bellow to a different project. It is a > >> bit simpler than the ones listed, as I think it may be too demanding to > ask > >> people to categorize in the git message the type of contribution the > patch > >> is addressing (BUG, Feature, …), if it has docs, if it has tests, etc. I > >> also think that it may make the git message too verbose. That info is > >> contained in the JIRA, and if there is a 1 to 1 mapping between Git > commit > >> and JIRA, it’s fairly easy to track down. > >> > >> The PR template is important, but more important, in my opinion, is to > >> have strong guidelines requiring that people squash commits within > >> functional units/features the code is addressing. Furthermore, if the > >> commits are for one JIRA/change (when no JIRA), and there are more than > one > >> commit, all should have the same title (STORM-XXX) in it. We should at > all > >> costs avoid merging into master extreme cases like 10 commits related to > >> one patch, where 5 or 6 of them are commits along the lines, addressed > code > >> review, added tests, cleaned up tests, addressed more code reviews, > etc... > >> without even having the STORM-XXX prefix in them. > >> > >> Squashing commits not only will make cherry-pick much easier, but more > >> importantly, if there is a regression, it is very easy to track down > which > >> change is causing the regression. Not doing this makes it very hard to > >> track down which change is causing problems. > >> > >> If we agree on some guidelines, I volunteer to update the documentation. > >> > >> === GIT TEMPLATE GUIDELINES === > >> > >> STORM-XXX: Title matching exactly the JIRA Summary (title) > >> > >> - An optional, bulleted (+, -, ., *), description of the contents of > >> - the patch. The goal is not to describe the contents of every file, > >> - but rather give a quick overview of the main functional areas > >> - addressed by the patch. > >> > >> The text immediately following the JIRA number (STORM-XXX: ) must be an > >> exact transcription of the JIRA summary (title), not the a summary of > the > >> contents of the patch. If the JIRA summary does not accurately describe > >> what the patch is addressing, the JIRA summary must be fixed, and then > >> copied to the Git commit message. > >> > >> A description with the contents of the patch is optional but strongly > >> encouraged if the patch is large and/or the JIRA title is not expressive > >> enough to highlight what the patch is doing. This text must be added > >> bellow, with one empty line of space in between, and be bulleted using > one > >> of the following bullet points (+, -, ., *). There must be at last a 1 > >> space indent before the bullet char, and exactly one space between > bullet > >> char and the first letter of the text. Bullets are not optional but > >> required. > >> > >> === GIT TEMPLATE GUIDELINES FOR COMMITS WITHOUT JIRA === > >> > >> For the sake of having a pattern one could grep, or exclude from grep, I > >> suggest that we prefix every commit that does not have a JIRA associated > >> with it with something along the lines: > >> > >> STORM-MINOR: Title summarizing what the patch is addressing. > >> > >> If a description of the change is necessary, it should follow the > >> guidelines above. The squashing guidelines also apply. > >> > >> === SQUASHING GUIDELINES === > >> > >> - Each commit message should have the first line matching the associated > >> JIRA (STORM-XXX, or STORM-MINOR) as described > >> - If possible, one JIRA/change should be one commit. > >> - If the patch is large, and it makes sense to break it into multiple > >> commits to make it easier to understand and cherry-pick (not easy to > >> merge), each commit should represent the functional units/features the > code > >> is addressing > >> - During code review it is OK to have multiple commits addressing the > >> changes proposed by the reviewers (also because it makes it easy to > track > >> those changes), but once the patch is ready to merge (has enough +1’s), > the > >> commits should be squashed according to the guidelines. > >> > >> Thanks, > >> Hugo > >> > >> On Jan 18, 2017, at 6:15 PM, P. Taylor Goetz <[email protected]<mailto: > >> [email protected]>> wrote: > >> > >> +1 for adopting a template. Not sure if it is possible, but I wouldn't > >> mind eyeing the same for JIRA. > >> > >> -Taylor > >> > >> On Jan 18, 2017, at 9:10 PM, Jungtaek Lim <[email protected]<mailto: > >> [email protected]>> wrote: > >> > >> Hi devs, > >> > >> I have seen some pull requests which are not having JIRA issue, or bad > >> title, or no description for rationale on pull request and issue. > >> > >> We already have DEVELOPER.md but this document covers more than > >> contribution and also not shown while opening pull request. > >> > >> I think we should add pull request template guiding how to create pull > >> request properly. Ideally it can be detailed form but at least it should > >> guide about title, and target branch. > >> > >> Some examples are here: > >> > >> > https://github.com/apache/spark/blob/master/.github/PULL_REQUEST_TEMPLATE > >> > >> > https://github.com/apache/flink/blob/master/.github/PULL_REQUEST_TEMPLATE.md > >> > >> > https://github.com/apache/zeppelin/blob/master/.github/PULL_REQUEST_TEMPLATE > >> > >> > https://github.com/apache/incubator-gearpump/blob/master/.github/PULL_REQUEST_TEMPLATE.md > >> > >> What's your opinion? > >> > >> Thanks, > >> Jungtaek Lim (HeartSaVioR) > >> > >> > >> > >
