Pushing revisions to a Gerrit code review

Martin Stoyanov recently asked an excellent question about the proper way to push revisions to a changeset in Gerrit. This is a very common question that folks new to Gerrit or Git have, and I think it deserves its own post.

When you do the first push of a local working branch to Gerrit, the act of pushing your code creates a Gerrit changeset. The changeset can be reviewed, and in the process of doing that review, it’s common for reviewers to request that the submitter make some changes to the code. Sometimes these changes are stylistic or cosmetic. Other times, the requested modifications can be extensive.

How you handle making the requested modifications and submitting those changes back to Gerrit depends on a few things:

  1. Are the changes requested mostly stylistic or cosmetic?
  2. Are the changes requested going to provide additional functionality that is dependent on the existing changeset?
  3. Are the changes requested going to provide additional functionality that is independent of the existing changeset?

Depending on the answers to the above questions, you should either amend the existing changeset commit, push a new commit to Gerrit from the same local branch, or push a new commit from a new local branch. Here’s some quick guidelines to help you decide:

The Changes Requested Are Cosmetic or Style-related

When a reviewer is providing some stylistic advice or offering suggestions for cosmetic changes or cleanups, you should amend the original commit. Do so like this:

# After making the requested changes...
git commit -a --amend
# Modify the commit message, if necessary, and save your editor
git review

While this looks fairly simple (and it is…), many folks make a fatal mistake when they modify the commit message and add sections that describe the “sausage-making” involved in the cleanups. DO NOT do this. It’s not necessary. Avoid adding any lines to the commit message that look like this:

  • “Cleaned up whitespace”
  • “DRY’d up some stuff based on review comments”
  • “Fixed typos found during reviews”

If all you did was correct typos and whitespace, simply leave the commit message as it was originally. After the call to git review, you will see a new patchset appear in the original code review. This is expected. The changeset is still viewed by Gerrit and reviewers as a single changeset, and reviewers may even select “Patchset 1” from the “Old Version History” dropdown instead of “Base” in order to see only the changes made in this last amended commit.

The Changes Requested Are Extensive and Depend on Original Commit

If a reviewer has asked for modifications to your original code, and the requested modifications are fairly extensive and depend on the code in your original commit, you have four choices:

  • Amend the original commit to include all new changes
  • Amend the original commit for some things, push those changes to the original commit, make additional changes in the same local branch, git commit those additional changes and git review
  • Make additional changes, git commit those additional changes and git review
  • Lobby for your original commit to be accepted as-is, then when your change is accepted and merged into master, then create a new branch from master and push the additional changes in a new changeset

Whenever you do not amend a commit and issue a call to git review, you will be created a dependent changeset. Gerrit will assign a new Change-Id to the patchset, but understands that the commit logically follows your original changeset’s code. If you go to the code review screen of your newly-created changeset, you will see your original changeset referenced in the “Dependencies” section. Below, you can see a screenshot of a changeset that is part of a “dependency chain”. Another patchset is dependent on this patchset and this patchset is dependent on another patchset.

Changeset showing a dependent changeset and a changeset dependent on this one (chained dependent patchsets)

Changeset showing a dependent changeset and a changeset dependent on this one (chained dependent patchsets)

It’s best to avoid long chains of dependent patchsets. The reason is because if a reviewer requests changes for one of the changesets at the “bottom” of the dependency chain, the entire chain of changesets (even changesets that are approved like the one shown above) are going to be held up from going through the gate tests.

The Changes Requested Are Extensive but are Independent of the Original Commit

If a reviewer has requested extensive changes, but points out that the changes they want made are actually independent of the changes in your original commit, the reviewers will generally ask the original committer to wait until this changeset is merged and create a new branch for the additional work. Normally, depending on the extent of the requested changes, reviewers will insist that the submitter create a new bug or blueprint on Launchpad to keep track of the additional work they feel is needed.


It’s up to the discretion of core reviewers and the original submitter to work out which of the above solutions works best for the particular changeset. Each changeset introduces code and functionality that must be treated differently, and changesets from one submitter may be dealt with differently than others. However, to keep things simple for yourself and upstream reviewers, it’s best to follow this simple advice:

  1. Prefer to amend the original commit. In most cases, this is the appropriate solution to push revisions to Gerrit.
  2. Don’t include sausage-making comments in the commit message.
  3. Prefer free-standing changesets to long chains of dependent patches.
  4. Ask reviewers what their preferences are.

Follow those guidelines and you’ll keep yourself out of the weeds. For more detailed information, including strategies for handling updates to your code that is dependent on another branch of code that gets updated, see the excellent OpenStack GerritWorkflow documentation.

Working with the OpenStack Code Review and CI system – Chef Edition

For too long, the state of the OpenStack Chef world had been one of duplicative effort, endless forks of Chef cookbooks, and little integration with how many of the OpenStack projects choose to control source code and integration testing. Recently, however, the Chef + OpenStack community has been getting its proverbial act together. Folks from lots of companies have come together and pushed to align efforts to produce a set of well-documented, flexible, but focused Chef cookbooks that install and configure OpenStack services.

My sincere thanks go out to the individuals who have helped to make progress in the last couple weeks, the individuals on the upstream openstack.org continuous integration team, and of course, the many authors of cookbooks whose code and work is being merged together.

OK, so what’s happened?

StackForge now hosting a set of Chef cookbooks for OpenStack

Individual cookbooks for each integrated OpenStack project have been created in the StackForge GitHub organization. Each cookbook name is prefixed with cookbook-openstack- followed by the OpenStack service name (not the project code name):

Note that we have not yet created the cookbook for Heat, but that will be coming in the Havana timeframe, for sure. Also note that the Ceilometer (metering) cookbook is empty right now. We’re in the process of pulling the ceilometer recipes out of the compute cookbook into a separate cookbook.

UPDATE: The Heat cookbook repository is now up on Stackforge.

In addition to the OpenStack project cookbooks listed above, there are three other related cookbooks:

Finally, there will be another repository called openstack-chef-repo that will contain example Chef roles, databags and documentation showing how all the OpenStack and supporting cookbooks are tied together to create an OpenStack deployment.

UPDATE: The OpenStack Chef Repository is now up on Stackforge.

Code in cookbooks gated by Gerrit like any other OpenStack project

The biggest advantage of hosting all these Chef cookbooks on the StackForge GitHub repository is the easy integration with the upstream continuous integration system. The upstream CI team has built a metric crap-ton (technical term) of automation code that enabled us to quickly have Gerrit managing the patch queues and code reviews for all these cookbook repositories as well as have each repository guarded by a set of gate jobs that run linter and unit tests against the cookbooks.

The rest of this blog post explains how to use the development and continuous integration systems when working on the OpenStack Chef cookbooks housed in Stackforge.

Prepare to develop on a cookbook

OK, so you want to start working on one of the OpenStack Chef cookbooks? Great! You will need to install the git-review plugin. The easiest way to do so is to use pip:

sudo pip install git-review

The first thing you need to do is clone the appropriate Git repository containing the cookbook code and set up your Gerrit credentials. Here is the code to do that:

git clone git@github.com:stackforge/cookbook-openstack-$SERVICE
cd cookbook-openstack-$SERVICE
git review -s

Of course, replace $SERVICE above with one of common, compute, identity, image, block-storage, object-storage, network, metering, or dashboard. What that will do is clone the upstream Stackforge repository for the corresponding cookbook to your local machine, change directory into that clone’d repository, and set up a git remote called “gerrit” pointing to the review.openstack.org Gerrit system.

If everything was successful, you should see something like this:

jpipes@uberbox:~/gerrit-tut$ git clone git@github.com:stackforge/cookbook-openstack-common
Cloning into 'cookbook-openstack-common'...
remote: Counting objects: 506, done.
remote: Compressing objects: 100% (168/168), done.
remote: Total 506 (delta 246), reused 503 (delta 243)
Receiving objects: 100% (506/506), 81.97 KiB, done.
Resolving deltas: 100% (246/246), done.
jpipes@uberbox:~/gerrit-tut$ cd cookbook-openstack-common/
jpipes@uberbox:~/gerrit-tut/cookbook-openstack-common$ git review -s
Creating a git remote called "gerrit" that maps to:

Repeat the above for each cookbook you wish to clone and work on locally, or simply execute this to clone them all:

for i in common compute identity image block-storage object-storage network metering dashboard;\
do git clone git@github.com:stackforge/cookbook-openstack-$i; cd cookbook-openstack-$i; git review -s; cd ../;\

Start to develop on a cookbook

Now that you have git clone’d the upstream cookbook repository and set up your Gerrit remote properly, you can begin coding on the cookbook. Remember, however, that you should never make changes in your local “master” branch. Always work in a local topic branch. This allows you to work on a branch of code separately from the local master branch you will use to bring in changes from other developers.

Create a new topic branch like so:

git checkout -b <TOPIC_NAME>

Here is an example of what you can expect to see:

jpipes@uberbox:~/gerrit-tut/cookbook-openstack-common$ git checkout -b tut-example
Switched to a new branch 'tut-example'

Once you are checked out into your topic branch, you can now add, edit, delete, and move files around as you wish. When you have made the changes you want to make, you then need to commit your changes to the working tree in source control.

IMPORTANT NOTE:: If you created any new files while working in your branch, you will need to tell Git about those new files before you commit. An easy way to check if you’ve added any new files that should be added to Git source control is to always call git status before doing your commit. git status will tell you if there are any untracked files in your working tree that you may need to add to Git:

jpipes@uberbox:~/gerrit-tut/cookbook-openstack-common$ touch something_new.txt
jpipes@uberbox:~/gerrit-tut/cookbook-openstack-common$ git status
# On branch tut-example
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#	something_new.txt
nothing added to commit but untracked files present (use "git add" to track)

As the note shows, you use the git add command to add the untracked file to source control:

jpipes@uberbox:~/gerrit-tut/cookbook-openstack-common$ git status
# On branch tut-example
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#	new file:   something_new.txt

If you make changes to files, they will show up in git status as changed files, as shown here:

jpipes@uberbox:~/gerrit-tut/cookbook-openstack-common$ vi README.md 
jpipes@uberbox:~/gerrit-tut/cookbook-openstack-common$ git status
# On branch tut-example
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#	new file:   something_new.txt
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#	modified:   README.md

As you can see, I edited the README.md file, and the call to git status shows that file as modified. If you want to review the changes you made, use the git diff command:

jpipes@uberbox:~/gerrit-tut/cookbook-openstack-common$ git diff
diff --git a/README.md b/README.md
index 4fbbd57..e197d3b 100644
--- a/README.md
+++ b/README.md
@@ -24,6 +24,8 @@ of all the settable attributes for this cookbook.
 Note that all attributes are in the `default["openstack"]` "namespace"
+TODO(jaypipes): Should we list all the attributes in the README?

If you are happy with the changes, you’re now ready to commit those changes to source control. Call git commit, like so:

git commit -a

This will open up your text editor and present you with an area to write your commit message describing the contents of your patch. Commit messages should be properly formatted and abide by the upstream conventions. Feel free to read that link, but here is a brief rundown of stuff to keep in mind:

  • Make the first line of the commit message 50 chars or less
  • Separate the first line from the rest of the commit message with a blank newline
  • Make the commit message descriptive of what the patch is and what the motivation for the patch was
  • Do NOT make the commit message into a list of the things in the patch you changed in each revision — we can already see what is contained in the patch

Save and close your editor to finalize the commit. Once successfully committed, you now need to push your changes to the Gerrit code review and patch management system on review.openstack.org. You do this using a call to git review.

When you issue a call to git review for any of the cookbooks, a patch review is created in Gerrit. Behind the scenes, the git review plugin is simply doing the call for you to git push to the Gerrit remote. You can always see what git review is doing by passing the -v flag, like so:

jpipes@uberbox:~/gerrit-tut/cookbook-openstack-common$ git commit -a
[tut-example 66f844a] Simple patch for tutorial -- please IGNORE
 1 file changed, 2 insertions(+)
 create mode 100644 something_new.txt
jpipes@uberbox:~/gerrit-tut/cookbook-openstack-common$ git review -v
2013-05-20 12:48:54.333823 Running: git log --color=never --oneline HEAD^1..HEAD
2013-05-20 12:48:54.337044 Running: git remote
2013-05-20 12:48:54.339673 Running: git branch -a --color=never
2013-05-20 12:48:54.342580 Running: git rev-parse --show-toplevel --git-dir
2013-05-20 12:48:54.345139 Running: git remote update gerrit
Fetching gerrit
2013-05-20 12:48:55.475616 Running: git rebase -i remotes/gerrit/master
2013-05-20 12:48:55.616412 Running: git reset --hard ORIG_HEAD
2013-05-20 12:48:55.620552 Running: git config --get color.ui
2013-05-20 12:48:55.623098 Running: git log --color=always --decorate --oneline HEAD --not remotes/gerrit/master --
2013-05-20 12:48:55.626745 Running: git branch --color=never
2013-05-20 12:48:55.629703 Running: git log HEAD^1..HEAD
Using local branch name "tut-example" for the topic of the change submitted
2013-05-20 12:48:55.634665 Running: git push gerrit HEAD:refs/publish/master/tut-example
remote: Resolving deltas: 100% (2/2)
remote: Processing changes: new: 1, done    
remote: New Changes:
remote:   https://review.openstack.org/29797
To ssh://jaypipes@review.openstack.org:29418/stackforge/cookbook-openstack-common.git
 * [new branch]      HEAD -> refs/publish/master/tut-example
2013-05-20 12:48:56.775939 Running: git rev-parse --show-toplevel --git-dir

As you can see in the above output, a new patch review was created at the location https://review.openstack.org/29797. You can go to that URL and see the patch review, shown here before any comments or reviews have been made on the patch.


Reviewing patches with Gerrit

Gerrit has three separate levels of reviews: Verify, Code-Review, and Approve.

The Verify (V) review level

The Verify level is limited to the Jenkins Gerrit user, which runs the automated tests that protect each cookbook repository’s master branch. These automated tests are known as gate tests.

When you push code to Gerrit, there are a set of automatic tests that are run against your code by Jenkins. Jenkins is a continuous integration job system that the upstream OpenStack CI team has integrated into Gerrit so that projects managed by Gerrit may have a series of automated check jobs run against proposed patches to the project. Below, you can see that the Jenkins user in Gerrit has already executed two jobs — gate-cookbook-openstack-common-chef-lint and gate-cookbook-openstack-common-chef-unit — against the proposed code changes. The jobs (expectedly) both pass, as I haven’t actually changed anything in the code, only added a blank file and added a line to the README file.


Curious about how those gate jobs are set up? Check out the github.com openstack-infra/config project. Hint: look at these two files.

If the Jenkins jobs fail, you will see Jenkins issue a -1 in the V column in the patch review. Any -1 from Jenkins as a result of a failed gate test will prevent the patch from being merged into the target branch, regardless of the reviews of any human.

The Code-Review (R) review level

Anyone who is logged in to Gerrit can review any proposed patch in Gerrit. To log in to Gerrit, click the “Sign In” link in the top right corner and log in using the Launchpad Single-Signon service. Note: This requires you to have an account on Launchpad.

Once logged in, you will see a “Review” button on each patch in the patchset. You can see this Review button in the images above. If you were the one that pushed the commit, you will also see buttons for “Abandon”, “Work in Progress”, and “Rebase Change”. The “Abandon” button simply lets you mark the patchset as abandoned and gets the patch off of the review radar. “Work in Progress” lets you mark the patchset as not ready for reviews, and “Rebase Change” is generally best left alone unless you know what you’re doing. 😉

Each file (including the commit message itself) has a corresponding link that you can view the diff of that file and add inline comments similar to how GitHub pull requests allow inline commenting. Simply double click directly below the line you wish to comment on, and a box for your comments will appear, as shown below:


IMPORTANT NOTE: Unlike GitHub inline commenting on pull requests, your inline comments on Gerrit reviews are NOT viewable by others until you finalize your review by clicking the “Review” button. Your comments will appear in red as “Draft” comments on the main page of the patch review, as shown below:


To put in a review for the patch, click the “Review” button. You will see options for:

  • +1 Looks good to me, but someone else must approve
  • +0 No score
  • -1 I would prefer you didn’t merge this

If you are a core reviewer, in addition to the above three options, you will also see:

  • +2 Looks good to me (core reviewer)
  • -2 Do Not Merge

There is also a comment are for you to put your review comments, which is directly above an area that shows all the inline comments you have made:


After selecting the review +/- that matches your overall thoughts on the patch, and entering any comment, click the “Publish Comments” button, and your review will show in the comments of the patch, as shown below:


The Approve (A) review level

Members of the core review team also see a separate area in the review screen for the Approve (A) review level. This level tells Gerrit to either proceed with the merge of the patch into the target branch (+1 Approve) or to wait (0 No Score).

The general rule of thumb is that core reviewers should not hit +1 Approve until 2 or more core reviewers (can include the individual doing the +1 Approve) have added a +2 (R) to the patch in reviews. This rule is subject to the discretion of the core reviewer for trivial changes like typo fixes, etc.


I hope this tutorial has been a help for those new to the Gerrit and Jenkins integration used by the OpenStack upstream projects. Contributing to the Chef OpenStack cookbooks should be no different than contributing to the upstream OpenStack projects now, and additional gate tests — including full integration test runs using Vagrant or even a multi-node deployment — are on our TODO radar. Please sign up on the OpenStack Chef mailing list if you haven’t already. We look forward to your contributions!

The Science (or Art?) of Commit Messages

There are some things in the world of development that you appreciate much more when you do a lot of code reviews. One of those things is commit messages.

At first glance, commit messages seem to be a small, relatively innocuous thing for a developer. When you commit code, you type in some description about your code changes and then, typically, push your code somewhere for review by someone.

Regardless of whether the code you pushed is going to an open source project, an internal proprietary code repository, or just some code exchanged between friends working on a joint project, that simple little commit message tells the person reading your code a whole lot about you. It speaks volumes about the way you feel about the code you submit and the quality of the review you expect for your code.

As an example, suppose I was working on some code that fixed a bug. I got my code ready for initial review and I did the following:

$> git commit -a -m "Fixes some stuff"

And then I push my commit somewhere using git push

Inevitably, what happens is that another developer will get some email or notification that I have pushed code up to some repository. It is likely that this notification will look something like this:

Change subject: Fixes some stuff

Fixes some stuff

Change-Id: I79bbac32b5c99742b5cb283c6e55e6204bf92adc
M path/to/some/changed/file
1 file changed, 1 insertion(+), 1 deletion(-)

And in the notification will be some link to a place to go do a code review.

Now, what do you think is the first thought that goes through the reviewer’s mind? My guess would be: Really? Fixes what stuff? By not including any context about what the patch is attempting to solve, you leave the reviewer with a bad taste in their mouth. And a bad taste in the reviewer’s mouth generally means one thing: a reluctance to review your patch.

OK, so what could we do to make the commit message better, to provide the reviewer with more initial context about your patch? Well, the first thing that comes to mind is to reference a specific bug that you are fixing with this patch.

Alright, so we amend our commit message to include a bug identifier:

$> git commit --amend -m "Fixes Bug 123456"

And subsequently push our amended commit message. The reviewer now gets a new notification that you’ve amended a previous patch. Now the notification includes the bug identifier. What do you think the next thought a typical reviewer might have? My guess is this: What, does this developer think that I’ve memorized all the bug IDs for all open bugs? How should I know what Bug 123456 is about? And here comes that bad taste in the mouth again.

OK, so this time, we will forgo the use of the time-saving -m switch to git commit and actually type a proper, multi-line commit message in our editor of choice that describes the bug that our patch fixes, including a brief description of how we fixed the bug:

git commit --amend  # This will open up your editor...

Now we’d enter a good commit message … something like this would work:

Fixes Bug 123456 - ImportError raised improperly in Storage Driver

Due to a circular dependency, an ImportError was improperly
being thrown when the storage driver was set to XYZ. Rearranged
code to remove circular dependency.

The commit message now will give the reviewer everything they need in the notification to understand what the patch is for and how you solved a bug, without needing to go to their bug tracker to figure out what the bug was about.

A detailed commit message shows you care about the time that reviewers spend on your patch and that you value the code you are submitting.