[DISCUSS] Review sign-off procedures

[Kevin Sweeney <ke...@apache.org>]

Hi all,

>From a discussion in IRC I realized we should probably formalize the
convention we've been following for accepting commits into aurora.

Here's my take at formalizing the informal convention, please comment
inline:

In order to facilitate high-quality code contributions, all changes should
be reviewed and signed off by at least one committer (if a committer
submits a review it should be reviewed by at least one *other* committer).
Review requests should be addressed to *specific* committers who are
familiar with the codebase being changed.

It is each committer's responsibility to promptly address all review
requests addressed to them, either by signing off, providing feedback, or
explicitly declining to review the change. Changes should not be committed
until all reviewers sign off or explicitly decline to review a change.

Reviews for code changes should generally have associated test coverage and
attached test output (at least a short description of testing done).

Small patches are preferred over large ones when possible as they are
easier to review, but patches should be atomic such that tests always pass
on master.

Nontrivial changes should generally have an associated entry in the issue
tracker.

In order to minimize process overhead and encourage community contributions
trivial changes may be accepted by a single committer. Trivial changes
include small documentation improvements (including in code comments),
typos in code and scripts, and anything that in the discretion of the
committer doesn't warrant additional review (code changes must still pass
relevant tests).

The above are guidelines - committers still have discretion to unilaterally
sign-off on and accept any patch on a case-by-case basis, but they are
expected to follow the above guidelines.

Re: [DISCUSS] Review sign-off procedures

[Jake Farrell <jf...@apache.org>]

Kevin
Thanks for summarizing this, I think that this is a great start and we
should put this into a new doc, committers.md. I would like to also suggest
that we reference the committers responsibilities (link below) in this
document to ensure that a new committer is aware of these.

There is a lot of overlap between what committers need to know as
formalized process and informative for a new contributor who has a patch.
ideally keeping contributing to as concise and outlined of a process as
possibly will help all involved. I think that contributing.md should become
more of that outline for the entire process for someone new to the project
and detail the step for bug/feature -> ticket -> patch -> review there.

-Jake

http://www.apache.org/dev/committers.html#committer-responsibilities





On Fri, Jan 17, 2014 at 5:42 PM, Kevin Sweeney <ke...@apache.org> wrote:

> Hi all,
>
> From a discussion in IRC I realized we should probably formalize the
> convention we've been following for accepting commits into aurora.
>
> Here's my take at formalizing the informal convention, please comment
> inline:
>
> In order to facilitate high-quality code contributions, all changes should
> be reviewed and signed off by at least one committer (if a committer
> submits a review it should be reviewed by at least one *other* committer).
> Review requests should be addressed to *specific* committers who are
> familiar with the codebase being changed.
>
> It is each committer's responsibility to promptly address all review
> requests addressed to them, either by signing off, providing feedback, or
> explicitly declining to review the change. Changes should not be committed
> until all reviewers sign off or explicitly decline to review a change.
>
> Reviews for code changes should generally have associated test coverage and
> attached test output (at least a short description of testing done).
>
> Small patches are preferred over large ones when possible as they are
> easier to review, but patches should be atomic such that tests always pass
> on master.
>
> Nontrivial changes should generally have an associated entry in the issue
> tracker.
>
> In order to minimize process overhead and encourage community contributions
> trivial changes may be accepted by a single committer. Trivial changes
> include small documentation improvements (including in code comments),
> typos in code and scripts, and anything that in the discretion of the
> committer doesn't warrant additional review (code changes must still pass
> relevant tests).
>
> The above are guidelines - committers still have discretion to unilaterally
> sign-off on and accept any patch on a case-by-case basis, but they are
> expected to follow the above guidelines.
>