[DISCUSS] Commit-Then-Review or Review-Then-Commit

[Owen O'Malley <om...@apache.org>]

Welcome everyone to the Metron project.

One of the first points that we need to decide on is how you'd like to run
your development. In particular, most Apache projects use either
Commit-Then-Review or Review-Then-Commit. Either one works and they each
have pluses and minuses and unfortunately it can turn in to a religious war
of the vi vs emacs level. *grin*

Commit-Then-Review lets committers make changes to the git repository and
everyone watches the commits@metron list review the changes.

Review-Then-Commit requires any changes to be uploaded to github or Jira
and +1'ed by a different committer before it gets pushed to Metron's master
branch.

The advantages are:

* CTR
 * Patches by committers don't get blocked waiting for review.
 * It enables the project to move faster.

* RTC
  * All changes are looked over before they are pushed, so some problems
caught beforehand.
  * Less churn on master branch as review comments force changes.

Thoughts?

   Owen

Re: [DISCUSS] Commit-Then-Review or Review-Then-Commit

[larry mccay <lm...@apache.org>]

Just to clarify...

The CTR/RTC policy for the project is for committers on the project.
All committers have permission to check in code.

All members of the incoming podling are generally both committer and PPMC
member.

Difference in permissions only exist between committers and non-committers
(contributors).

The hybrid approach is very much like the honor snack box - we already
trust our committers.
Any significant work should have a corresponding JIRA assigned to it with
some level of design description.

Changes that require architectural changes should also be communicated with
a [DISCUSS] thread. This is
to elicit feedback on the approach and to communicate it to the community.

Sometimes those guidelines will not be followed and it is our job as team
members to review commits and JIRAs as the emails go by and flag something
as having needed review or discussion.
We can even ask for a revert if necessary.

We can also require that everything have a review before it be committed.
This can be accomplished through JIRA, review board and other integrations
as well.
It would require at least one committer to +1 and then you can commit your
work - maybe give it 24 hrs before.


On Tue, Dec 8, 2015 at 10:26 AM, Charles Porter <ch...@gmail.com>
wrote:

> I like Larry's idea.
> I have used a similar approach in small commercial projects with good
> results
>
> However, it creates some procedural challenges:
>
> The advantage of the simple CTR/RTC dichotomy is that it is easily enforced
> by the mechanics of permissions.
> Under CTR, everyone has permission to commit.
> Under RTC very few have commit permission, so it is structurally enforced
> that someone at least claims to review the pull request.
>
> If we implement the hybrid approach we could need to structurally implement
> RTC, then the reviewer, as a matter of policy, simply has to merge the pull
> request -- effectively rubber-stamping the review.
> This creates a burden on the person who has to make the merges. This burden
> could be quite large if there is a lot of activity.
>
> If we take the opposite approach and trust people to make direct commits
> for bug fixes but always  create pull requests for anything bigger than a
> bug fix, it is a pretty sure thing that the rule will not be followed. It
> goes wrong for a variety of reasons ranging from simply forgetting to
> malice. Policing the behavior becomes a significant task in itself.
>
> Does anybody have other ideas about how we would implement Larry's
> strategy?
>
> -Charles
>
>
>
> On Tue, Dec 8, 2015 at 4:25 AM, larry mccay <lm...@apache.org> wrote:
>
> > My preference would actually be a hybrid with some simple guidelines:
> >
> > * Overall CTR - at least while we are developing critical mass and
> > functionality - however...
> > * Architectural, security related and largish patches should have a
> review
> > - if for nothing else but to communicate the intents clearly
> > * Bug fixes and minor changes should not require review but we should
> feel
> > free to request one at any time
> >
> > thoughts?
> >
> > On Tue, Dec 8, 2015 at 1:14 AM, Owen O'Malley <om...@apache.org>
> wrote:
> >
> > > Welcome everyone to the Metron project.
> > >
> > > One of the first points that we need to decide on is how you'd like to
> > run
> > > your development. In particular, most Apache projects use either
> > > Commit-Then-Review or Review-Then-Commit. Either one works and they
> each
> > > have pluses and minuses and unfortunately it can turn in to a religious
> > war
> > > of the vi vs emacs level. *grin*
> > >
> > > Commit-Then-Review lets committers make changes to the git repository
> and
> > > everyone watches the commits@metron list review the changes.
> > >
> > > Review-Then-Commit requires any changes to be uploaded to github or
> Jira
> > > and +1'ed by a different committer before it gets pushed to Metron's
> > master
> > > branch.
> > >
> > > The advantages are:
> > >
> > > * CTR
> > >  * Patches by committers don't get blocked waiting for review.
> > >  * It enables the project to move faster.
> > >
> > > * RTC
> > >   * All changes are looked over before they are pushed, so some
> problems
> > > caught beforehand.
> > >   * Less churn on master branch as review comments force changes.
> > >
> > > Thoughts?
> > >
> > >    Owen
> > >
> >
>

Re: [DISCUSS] Commit-Then-Review or Review-Then-Commit

[Charles Porter <ch...@gmail.com>]

I like Larry's idea.
I have used a similar approach in small commercial projects with good
results

However, it creates some procedural challenges:

The advantage of the simple CTR/RTC dichotomy is that it is easily enforced
by the mechanics of permissions.
Under CTR, everyone has permission to commit.
Under RTC very few have commit permission, so it is structurally enforced
that someone at least claims to review the pull request.

If we implement the hybrid approach we could need to structurally implement
RTC, then the reviewer, as a matter of policy, simply has to merge the pull
request -- effectively rubber-stamping the review.
This creates a burden on the person who has to make the merges. This burden
could be quite large if there is a lot of activity.

If we take the opposite approach and trust people to make direct commits
for bug fixes but always  create pull requests for anything bigger than a
bug fix, it is a pretty sure thing that the rule will not be followed. It
goes wrong for a variety of reasons ranging from simply forgetting to
malice. Policing the behavior becomes a significant task in itself.

Does anybody have other ideas about how we would implement Larry's strategy?

-Charles



On Tue, Dec 8, 2015 at 4:25 AM, larry mccay <lm...@apache.org> wrote:

> My preference would actually be a hybrid with some simple guidelines:
>
> * Overall CTR - at least while we are developing critical mass and
> functionality - however...
> * Architectural, security related and largish patches should have a review
> - if for nothing else but to communicate the intents clearly
> * Bug fixes and minor changes should not require review but we should feel
> free to request one at any time
>
> thoughts?
>
> On Tue, Dec 8, 2015 at 1:14 AM, Owen O'Malley <om...@apache.org> wrote:
>
> > Welcome everyone to the Metron project.
> >
> > One of the first points that we need to decide on is how you'd like to
> run
> > your development. In particular, most Apache projects use either
> > Commit-Then-Review or Review-Then-Commit. Either one works and they each
> > have pluses and minuses and unfortunately it can turn in to a religious
> war
> > of the vi vs emacs level. *grin*
> >
> > Commit-Then-Review lets committers make changes to the git repository and
> > everyone watches the commits@metron list review the changes.
> >
> > Review-Then-Commit requires any changes to be uploaded to github or Jira
> > and +1'ed by a different committer before it gets pushed to Metron's
> master
> > branch.
> >
> > The advantages are:
> >
> > * CTR
> >  * Patches by committers don't get blocked waiting for review.
> >  * It enables the project to move faster.
> >
> > * RTC
> >   * All changes are looked over before they are pushed, so some problems
> > caught beforehand.
> >   * Less churn on master branch as review comments force changes.
> >
> > Thoughts?
> >
> >    Owen
> >
>

Re: [DISCUSS] Commit-Then-Review or Review-Then-Commit

[larry mccay <lm...@apache.org>]

My preference would actually be a hybrid with some simple guidelines:

* Overall CTR - at least while we are developing critical mass and
functionality - however...
* Architectural, security related and largish patches should have a review
- if for nothing else but to communicate the intents clearly
* Bug fixes and minor changes should not require review but we should feel
free to request one at any time

thoughts?

On Tue, Dec 8, 2015 at 1:14 AM, Owen O'Malley <om...@apache.org> wrote:

> Welcome everyone to the Metron project.
>
> One of the first points that we need to decide on is how you'd like to run
> your development. In particular, most Apache projects use either
> Commit-Then-Review or Review-Then-Commit. Either one works and they each
> have pluses and minuses and unfortunately it can turn in to a religious war
> of the vi vs emacs level. *grin*
>
> Commit-Then-Review lets committers make changes to the git repository and
> everyone watches the commits@metron list review the changes.
>
> Review-Then-Commit requires any changes to be uploaded to github or Jira
> and +1'ed by a different committer before it gets pushed to Metron's master
> branch.
>
> The advantages are:
>
> * CTR
>  * Patches by committers don't get blocked waiting for review.
>  * It enables the project to move faster.
>
> * RTC
>   * All changes are looked over before they are pushed, so some problems
> caught beforehand.
>   * Less churn on master branch as review comments force changes.
>
> Thoughts?
>
>    Owen
>