[DISCUSS] Code Review with Gerrit

[Sean Schulte <Se...@rackspace.com>]

We currently have a process where code review happens on Github, and merges manually happen back to the official ASF git repository; then, the ASF repository is synced over to Github, and the Github pull request is automatically closed.

The merging process is like so:

  1.  Add the official Apache repo as a remote in your local clone (one time step): git remote add apache https://git-wip-us.apache.org/repos/asf/incubator-metron.git
  2.  Checkout whatever branch you want to merge it into: git checkout master
  3.  Merge the pull request: git pull git@github.com:charlesporter/incubator-metron.git master
  4.  Push the results to the Apache repo: git push apache master

However, this does not verify that a code review has taken place or that the code has been approved. It does not verify that the code builds or that the tests pass. It does not rebase or squash the commits into a single commit. These are all separate steps that aren't controlled as part of this workflow, leaving us all open to making mistakes.

It would be better for us to run Gerrit, which would be directly linked to the ASF git repository. We would create pull requests against this Gerrit instance, and code review it there. The interface would prevent a mistaken merge without a passing code review, and (once we set it up) a passing CI build.

This simplifies the merge process, gates it on code review and CI, and provides access control. (We would want to make it so only Gerrit has write access to the actual repository master branch, and committers have access to Gerrit.)

We would need to host the Gerrit instance somewhere. Rackspace can host it, or if Apache has another place to host it that would be great too.

What are everyone's thoughts on replacing our current ASF/Github/manual process with Gerrit, and setting up a workflow that requires a code review before a merge is possible?

Thanks, Sean

Re: [DISCUSS] Code Review with Gerrit

[Elana Hashman <el...@rackspace.com>]

On Wed, 6 Jan 2016, P. Taylor Goetz wrote:

> Gerrit has been a very sticky subject in the past, see this thread for more background:[1].
> (snip)
> [1] http://mail-archives.apache.org/mod_mbox/incubator-general/201507.mbox/%3cCAF8HOZ+8=B4joiOLek4g+o-96aX+C41Ujm243jUUh9j1VPo=Tw@mail.gmail.com%3e

Thanks for sharing this thread, interesting read.

> You can ensure that all the tests pass by setting up a CI build that runs on every pull request. A lot of projects use Travis CI for this, and ASF INFRA has support for it.

So Paul is looking to enable Travis CI for our pull requests in #9 (https://github.com/apache/incubator-metron/pull/9).

Rather than using a gating Gerrit, what if we were to e.g. add a git pre-receive hook on the master ASF repo that verified the CI status for a given commit/branch, and rejected the push if, using the Travis API, we verify that either the build has not yet completed or failed the CI? I'm curious how other Apache projects integrate gating CI into their merge workflows, if not through Gerrit.

- e

Re: [DISCUSS] Code Review with Gerrit

["P. Taylor Goetz" <pt...@gmail.com>]

Gerrit has been a very sticky subject in the past, see this thread for more background:[1].


> On Jan 6, 2016, at 11:33 AM, Sean Schulte <Se...@rackspace.com> wrote:
> 
> We currently have a process where code review happens on Github, and merges manually happen back to the official ASF git repository; then, the ASF repository is synced over to Github, and the Github pull request is automatically closed.
> 
> The merging process is like so:
> 
>  1.  Add the official Apache repo as a remote in your local clone (one time step): git remote add apache https://git-wip-us.apache.org/repos/asf/incubator-metron.git
>  2.  Checkout whatever branch you want to merge it into: git checkout master
>  3.  Merge the pull request: git pull git@github.com:charlesporter/incubator-metron.git master
>  4.  Push the results to the Apache repo: git push apache master

I probably should have added a step:

2a. Check that the pull request has been reviewed and received the requisite number of +1’s from committers, and that no committers have vetoed (-1) the change.

> 
> However, this does not verify that a code review has taken place or that the code has been approved. It does not verify that the code builds or that the tests pass. It does not rebase or squash the commits into a single commit. These are all separate steps that aren't controlled as part of this workflow, leaving us all open to making mistakes.

The way a lot of projects typically ensure that a review has taken place is to check the JIRA/Pull Request to ensure that it has the required number of +1’s from committers. Mistakes can happen, but they can be easily reverted.

You can ensure that all the tests pass by setting up a CI build that runs on every pull request. A lot of projects use Travis CI for this, and ASF INFRA has support for it.

You should also be able to trust other committers on the project to do the right thing, and trust that others will catch and correct any mistakes that are made. Trust is a key aspect of healthy ASF communities, IMO.

> 
> It would be better for us to run Gerrit, which would be directly linked to the ASF git repository. We would create pull requests against this Gerrit instance, and code review it there. The interface would prevent a mistaken merge without a passing code review, and (once we set it up) a passing CI build.
> 
> This simplifies the merge process, gates it on code review and CI, and provides access control. (We would want to make it so only Gerrit has write access to the actual repository master branch, and committers have access to Gerrit.)

This (a non-human as the only one with write access to the official ASF repo) likely won’t happen.

> 
> We would need to host the Gerrit instance somewhere. Rackspace can host it, or if Apache has another place to host it that would be great too.
> 
> What are everyone's thoughts on replacing our current ASF/Github/manual process with Gerrit, and setting up a workflow that requires a code review before a merge is possible?

I would encourage the project community to discuss guidelines for code review based on the currently available ASF infrastructure and mutual trust.

If you still want to pursue using gerrit, I would ping INFRA to see if it’s supported/allowed.

> 
> Thanks, Sean

-Taylor

[1] http://mail-archives.apache.org/mod_mbox/incubator-general/201507.mbox/%3cCAF8HOZ+8=B4joiOLek4g+o-96aX+C41Ujm243jUUh9j1VPo=Tw@mail.gmail.com%3e