[DISCUSS] Commit guidelines

[Ajay Yadava <aj...@apache.org>]

Hello everyone,

Over time, I have observed a few patterns and I feel that there are certain
areas where we can collectively improve on. Following is the list of
guidelines that I have put together.

   - Don't commit patches without docs.
   - Don't commit patches without test-cases
   - Ensure release notes are updated in JIRA wherever applicable. *[new]*

Please don't use / indulge separate JIRAs for docs / tests. All code
changes should go in along with tests and docs.

   - Don't commit unless all review comments are addressed / answered.

Please ensure all review comments, including ones from other contributors
(not just committers') are addressed. As much as possible, avoid creating
separate JIRAs for review comments.

   - Encourage reuse of pull requests

Sometimes contributors unintentionally create a new pull request instead of
updating the old one where the review comments were provided. Contributors
should strive to reuse the old prs in order to preserve the history of
reviews. Committers should try to check that prs are reused and old
feedback is addressed.

   - Avoid putting meaningless squashed commit messages in the detailed
   commit message.

The pr-merge tool has an option to list the squashed commit messages as
part of the detailed commit message. Most pull requests have meaningless
intermediate commit messages, please don't include them in the detailed
commit message. However, if commit messages add valuable context, please
include them. May be we should change the default option to no in the tool.

   - Give others a chance to review.

IMHO, it is polite to wait at least 24 Hrs after a +1 before committing a
change. If yours is first +1 and you intend to commit it, please consider
putting a comment expressing the same. This gives others who may have some
opinions on the pull request, but couldn't get to it before you, a chance
to review.

   - Update fixVersions and update thoughtfully.

Again using pr-merge tool here helps ensuring that fixVersion is updated,
though figuring out correct fixVersion requires some explanation. Simple
rule of thumb is - change belongs to the earliest release in which it got
committed e.g. if a release branch has been cut and a commit is applied to
both master and release branch, fixVersion should be release version (and
not trunk).

   - Commit changes to all applicable branches

A common mistake is to commit the patch only to the branch and not to
master. Although, this has been reduced a lot by the pr tool, but it is
still happening and something to keep in mind when you are not using the
pr-merge tool.

Just to clarify, this list is not meant to serve as list of rules but as a
set of guidelines. Like all good things, these are not expected to be
followed blindly. Part of being a committer is to develop the art of
knowing when to make an exception. A simple comment explaining your
rationale goes a long way ;)

As usual, suggestions / modifications / addenda are welcome.

Regards
Ajay Yadava

RE: [DISCUSS] Commit guidelines

[Srikanth Sundarrajan <sr...@hotmail.com>]

There are generally good set of guidelines and should make its way to https://cwiki.apache.org/confluence/display/FALCON under the Committers guide I feel.
RegardsSrikanth Sundarrajan

> From: ajayyadava@apache.org
> Date: Sat, 3 Sep 2016 18:37:34 +0000
> Subject: [DISCUSS] Commit guidelines
> To: dev@falcon.apache.org
> 
> Hello everyone,
> 
> Over time, I have observed a few patterns and I feel that there are certain
> areas where we can collectively improve on. Following is the list of
> guidelines that I have put together.
> 
>    - Don't commit patches without docs.
>    - Don't commit patches without test-cases
>    - Ensure release notes are updated in JIRA wherever applicable. *[new]*
> 
> Please don't use / indulge separate JIRAs for docs / tests. All code
> changes should go in along with tests and docs.
> 
>    - Don't commit unless all review comments are addressed / answered.
> 
> Please ensure all review comments, including ones from other contributors
> (not just committers') are addressed. As much as possible, avoid creating
> separate JIRAs for review comments.
> 
>    - Encourage reuse of pull requests
> 
> Sometimes contributors unintentionally create a new pull request instead of
> updating the old one where the review comments were provided. Contributors
> should strive to reuse the old prs in order to preserve the history of
> reviews. Committers should try to check that prs are reused and old
> feedback is addressed.
> 
>    - Avoid putting meaningless squashed commit messages in the detailed
>    commit message.
> 
> The pr-merge tool has an option to list the squashed commit messages as
> part of the detailed commit message. Most pull requests have meaningless
> intermediate commit messages, please don't include them in the detailed
> commit message. However, if commit messages add valuable context, please
> include them. May be we should change the default option to no in the tool.
> 
>    - Give others a chance to review.
> 
> IMHO, it is polite to wait at least 24 Hrs after a +1 before committing a
> change. If yours is first +1 and you intend to commit it, please consider
> putting a comment expressing the same. This gives others who may have some
> opinions on the pull request, but couldn't get to it before you, a chance
> to review.
> 
>    - Update fixVersions and update thoughtfully.
> 
> Again using pr-merge tool here helps ensuring that fixVersion is updated,
> though figuring out correct fixVersion requires some explanation. Simple
> rule of thumb is - change belongs to the earliest release in which it got
> committed e.g. if a release branch has been cut and a commit is applied to
> both master and release branch, fixVersion should be release version (and
> not trunk).
> 
>    - Commit changes to all applicable branches
> 
> A common mistake is to commit the patch only to the branch and not to
> master. Although, this has been reduced a lot by the pr tool, but it is
> still happening and something to keep in mind when you are not using the
> pr-merge tool.
> 
> Just to clarify, this list is not meant to serve as list of rules but as a
> set of guidelines. Like all good things, these are not expected to be
> followed blindly. Part of being a committer is to develop the art of
> knowing when to make an exception. A simple comment explaining your
> rationale goes a long way ;)
> 
> As usual, suggestions / modifications / addenda are welcome.
> 
> Regards
> Ajay Yadava