Difference between revisions of "Gerrit"

From Dogtag
Jump to: navigation, search
(Setting up gerrit in your git repos)
(Gerrit UI)
Line 13: Line 13:
 
## As long as the change-id is not changed, it will update the existing review.
 
## As long as the change-id is not changed, it will update the existing review.
  
== Gerrit UI ==
+
== Reviewing a patch ==
 
* Go to https://review.gerrithub.io/  and click github sign-in.
 
* Go to https://review.gerrithub.io/  and click github sign-in.
 +
* Click the patch you wish to review.
 
* You can make comments on specific files, or on the patch as a whole.
 
* You can make comments on specific files, or on the patch as a whole.
 
* To make comments on a specific file, click on the file name.  You will see a side by side diff of the file.  I usually end up putting in comments on the right hand side (the changed file).  Click on the line number at/near the place you want to comment.  A box should open up.  You can type stuff in and click "Save".
 
* To make comments on a specific file, click on the file name.  You will see a side by side diff of the file.  I usually end up putting in comments on the right hand side (the changed file).  Click on the line number at/near the place you want to comment.  A box should open up.  You can type stuff in and click "Save".
 
* Once you have finished making comments in all the files you want, go to the top level page.  You will see draft:X in the files you have commented on.  Click "Reply" -- You may add a additional comment on the whole update - or just click Post.
 
* Once you have finished making comments in all the files you want, go to the top level page.  You will see draft:X in the files you have commented on.  Click "Reply" -- You may add a additional comment on the whole update - or just click Post.
* You have a choice of things -2/-1/0/+1/+2 for code-review and -1/0/+1 for verified.  These will post too - and handle the flow.  I will explain what each of these mean now.
+
* When posting a comment, you can specify values for three flags: code-review (-2/-1/0/+1/+2), Verified (-1,+1) and Workflow (-1,0,+1).
  
 
=== code-review ===
 
=== code-review ===
Line 28: Line 29:
 
to the core reviewer group, so you should all see +2/-2.
 
to the core reviewer group, so you should all see +2/-2.
  
Example, edewata submits a change.  msauton tries out a change and determines
+
A change cannot be submitted to merge unless it receives at least one +2.
it works for him.  He will signal this by providing a +1.  cfu reviews the
 
change and makes suggestions of things to fix.  She would provide a -1.
 
 
 
Endi would make changes, and submit a new review.  cfu looks at the changes
 
and approves them by giving them a +2.
 
 
 
We have to decide what our policy is going to be.  The default policy is that
 
a change cannot be submitted to merge unless it receives at least one +2.
 
 
This is independent of however many -1s or +1s a patch has.
 
This is independent of however many -1s or +1s a patch has.
  
Line 43: Line 36:
 
submitter must work with the core reviewer to have them remove their -2.
 
submitter must work with the core reviewer to have them remove their -2.
  
In summary:
+
The possible values are:
   -2 Do not merge this.  You're on the wrong path.  If you want to merge this, you need to convince me first.  Even if you submit another patchset. the -2 will remain until I'm satisfied.
+
   -2 Do not merge this.  You're on the wrong path.  If you want to merge this, you need to convince me first.  Even if you submit another patch for this review. the -2 will remain until I'm satisfied.
   -1 This patch did not work for me, or this patch needs some work.
+
   -1 This patch did not work for me, or this patch needs some additional work.
 
   0  Neutral.  I have some comments but have not really had a chance to give it a thorough review.  Or maybe I'm commenting on my own patch.
 
   0  Neutral.  I have some comments but have not really had a chance to give it a thorough review.  Or maybe I'm commenting on my own patch.
 
   +1 Patch looks good to me but I'm not a core reviewer, or  - I'm a core reviewer and the patch looks good to me but its not really my area.  I want someone else to look at this.
 
   +1 Patch looks good to me but I'm not a core reviewer, or  - I'm a core reviewer and the patch looks good to me but its not really my area.  I want someone else to look at this.
 
   +2 Looks good to me (core reviewer).  Lets merge this puppy.
 
   +2 Looks good to me (core reviewer).  Lets merge this puppy.
 +
 +
Some example flows:
 +
 +
edewata submits a change.  msauton tries out a change and determines
 +
it works for him.  msauton is not a core-reviewer, so he can only provide a +1.
 +
This is insufficient to merge the change. cfu reviews the change and makes suggestions
 +
of things to fix.  She would provide a -1.
 +
 +
edewata make changes as requested by cfu, and submits a new review.
 +
Because the new submission uses the same change ID, the new patch will show up in the
 +
same review. cfu looks at the changes and approves them by giving them a +2.
 +
 +
In another example, alee submits a review.  jmagne looks at it and decides alee has clearly
 +
lost his marbles, and rejects the patch with a -2.  The patch cannot merge until alee
 +
convinces jmagne of the rightness of his approach.  alee fails to do so, and abandons the
 +
patch. (by clicking Abandon).
 +
 +
In another example, edewata submits a simple one-liner change.  This change meets the criteria of not
 +
requiring a review.  He gives himself and +2.
 +
 +
Note that the policy around +2's is collaborative.  We could write submit rules that enforce,
 +
for instance, that reviewers not be the same as the authors, but I think this is unnecessary.
 +
There will be some patches that you can give yourself a +2 and others that you will ask two or
 +
more core reviewers to review.  As usual, we'll just trust each other and use best judgment.
  
 
=== Workflow ===
 
=== Workflow ===
Line 54: Line 71:
 
This is a custom field that I added to follow what Openstack does.  Basically,
 
This is a custom field that I added to follow what Openstack does.  Basically,
 
this field is an indication that the change is ready to be merged.
 
this field is an indication that the change is ready to be merged.
 +
For some changes, for instance, we might want more than one reviewer
 
In Openstack, we often require more than one core reviewer to approve a change.
 
In Openstack, we often require more than one core reviewer to approve a change.
 
The second reviewer specifies approval with +2 and +1 Workflow.
 
The second reviewer specifies approval with +2 and +1 Workflow.
Line 75: Line 93:
 
also set Verified: +1.
 
also set Verified: +1.
  
=== Submit Button ===
+
== Merging a patch ==
 +
 
 +
If you are the reviewer for a patch and you think the patch is ready to be merged,
 +
you would set code-review +2, workflow +1 and Verified +1.  At that point, the
 +
patch will automatically be merged.
 +
 
 +
Assuming that all the submit conditions are satisfied, a submit button will appear.
 +
* at least one +2 and no -2 for code review,
 +
* +1 for Verified
 +
* +1 for Workflow,
 +
 
 +
Once clicked, the code is merged.
  
Once the conditions are satisfied: at least one +2 and no -2 for code review,
+
In future, when we have set up the jenkins jobs, this will be simpler.  The jenkins
+1 for Verified and +1 for Workflow, then a submit button pops up. Once clicked,
+
job kick off once the Workflow +1 flag is set, and will set the value for Verified.
the code is merged.  In Openstack, the submission is automatically done by
+
Assuming that passes, the jenkins just will automatically submit the patch and merge the
some job once the conditions are metI have not figured out how to do that
+
codeFor now, we'll just do it manually.
yet, but it should not be too hard.
 

Revision as of 16:51, 17 January 2017

Submitting a patch for review

  1. Clone the git repo and set up gerrit as a remote repo. This only needs to be done once.
    1. yum install git-review
    2. git clone git@github.com:test-pki/dogtag-pki.git
    3. git remote add gerrit ssh://<git_user_id>@review.gerrithub.io:29418/test-pki/dogtag-pki.git
    4. git review -s
  2. Make a change in the repo and commit the change. So, add some files or add a comment to an existing file or whatever. You will notice that a Change-Id: is added to the commit. DO NOT CHANGE this change-Id. It is used to track the commit in gerrit.
  3. git review -- This will generate a review in gerrit, which you can visit by going to the link provided. The commit is tracked by the change ID. If you have multiple commits, multiple reviews will be created - and gerrit will keep track of the dependencies.
  4. If you want to update a review:
    1. Make a change in your repo.
    2. git commit -a --amend
    3. git review
    4. As long as the change-id is not changed, it will update the existing review.

Reviewing a patch

  • Go to https://review.gerrithub.io/ and click github sign-in.
  • Click the patch you wish to review.
  • You can make comments on specific files, or on the patch as a whole.
  • To make comments on a specific file, click on the file name. You will see a side by side diff of the file. I usually end up putting in comments on the right hand side (the changed file). Click on the line number at/near the place you want to comment. A box should open up. You can type stuff in and click "Save".
  • Once you have finished making comments in all the files you want, go to the top level page. You will see draft:X in the files you have commented on. Click "Reply" -- You may add a additional comment on the whole update - or just click Post.
  • When posting a comment, you can specify values for three flags: code-review (-2/-1/0/+1/+2), Verified (-1,+1) and Workflow (-1,0,+1).

code-review

In gerrit, there is the concept of a group of core-reviewers. These are the ones that determine what code ultimately gets merged into the repo. These reviewers have the power to +2/-2. Everyone else can review the code and signal their approval by providing +1/-1. All the CS developers have been added to the core reviewer group, so you should all see +2/-2.

A change cannot be submitted to merge unless it receives at least one +2. This is independent of however many -1s or +1s a patch has.

-2 is the core reviewer's indication of rejection with prejudice. No patch can be submitted to merge with a -2. If a review gets a -2, the patch submitter must work with the core reviewer to have them remove their -2.

The possible values are:

 -2 Do not merge this.  You're on the wrong path.  If you want to merge this, you need to convince me first.  Even if you submit another patch for this review. the -2 will remain until I'm satisfied.
 -1 This patch did not work for me, or this patch needs some additional work.
 0  Neutral.  I have some comments but have not really had a chance to give it a thorough review.  Or maybe I'm commenting on my own patch.
 +1 Patch looks good to me but I'm not a core reviewer, or  - I'm a core reviewer and the patch looks good to me but its not really my area.  I want someone else to look at this.
 +2 Looks good to me (core reviewer).  Lets merge this puppy.

Some example flows:

edewata submits a change. msauton tries out a change and determines it works for him. msauton is not a core-reviewer, so he can only provide a +1. This is insufficient to merge the change. cfu reviews the change and makes suggestions of things to fix. She would provide a -1.

edewata make changes as requested by cfu, and submits a new review. Because the new submission uses the same change ID, the new patch will show up in the same review. cfu looks at the changes and approves them by giving them a +2.

In another example, alee submits a review. jmagne looks at it and decides alee has clearly lost his marbles, and rejects the patch with a -2. The patch cannot merge until alee convinces jmagne of the rightness of his approach. alee fails to do so, and abandons the patch. (by clicking Abandon).

In another example, edewata submits a simple one-liner change. This change meets the criteria of not requiring a review. He gives himself and +2.

Note that the policy around +2's is collaborative. We could write submit rules that enforce, for instance, that reviewers not be the same as the authors, but I think this is unnecessary. There will be some patches that you can give yourself a +2 and others that you will ask two or more core reviewers to review. As usual, we'll just trust each other and use best judgment.

Workflow

This is a custom field that I added to follow what Openstack does. Basically, this field is an indication that the change is ready to be merged. For some changes, for instance, we might want more than one reviewer In Openstack, we often require more than one core reviewer to approve a change. The second reviewer specifies approval with +2 and +1 Workflow.

The rule is that a change needs a +1 to be merged. A -1 will prevent a merge. In general, the patch author will often set a -1 for workflow to indicate a patch that is still a work in progress. This is useful to share patches between team members.

Verified

When we get around to it, we'll have a jenkins gate job that will run and at the very least confirm that the build does not break with this change. This will run automatically when workflow is set to +1. Later, we can add CI tests, pylint etc.

The rule is that you need a +1 to merge. If the jenkins job fails, then it will set -1.

Until those jobs are put in place, the developer who sets Workflow: +1 should also set Verified: +1.

Merging a patch

If you are the reviewer for a patch and you think the patch is ready to be merged, you would set code-review +2, workflow +1 and Verified +1. At that point, the patch will automatically be merged.

Assuming that all the submit conditions are satisfied, a submit button will appear.

  • at least one +2 and no -2 for code review,
  • +1 for Verified
  • +1 for Workflow,

Once clicked, the code is merged.

In future, when we have set up the jenkins jobs, this will be simpler. The jenkins job kick off once the Workflow +1 flag is set, and will set the value for Verified. Assuming that passes, the jenkins just will automatically submit the patch and merge the code. For now, we'll just do it manually.