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)
>
>
>
