In the past week, I’ve finally stopped to fix something that I’ve been wishing for years: inline code reviews in Launchpad. Well, I haven’t exactly managed fix it in Launchpad, but the integration with Rietveld feels nice enough to be relatively painless.
The integration is done using the lbox tool, that was developed in Go using the lpad package for the communication with Launchpad, and a newly written rietveld package for communication with Rietveld.
If you want to join me in my happines, here are the few steps to get that working for you as well.
First, install lbox from the Launchpad PPA. Since it’s written in Go, it has no dependencies.
$ sudo add-apt-repository ppa:gophers/go $ sudo apt-get update $ sudo apt-get install lbox
Now, as an example of using it, let’s suppose we want to perform a change in the lbox code itself. First, we take the branch out of Launchpad.
$ mkdir hacking $ cd hacking $ bzr branch lp:lbox Branched 9 revision(s).
Then, let’s create a feature branch based on the original trunk, and perform a change.
$ bzr branch lbox my-nice-feature Branched 9 revision(s). $ cd my-nice-feature $ echo # Yo >> Makefile $ bzr commit -m "Yo-ified makefile" Committing to: /home/user/hacking/my-nice-feature/ modified Makefile Committed revision 10.
Ok, we’re ready for the magic step, which is actually pushing that branch and proposing the merge on the original branch on both Launchpad and Rietveld. It’s harder to explain than to do it:
$ lbox propose -cr 2011/11/17 23:29:49 Looking up branch information for "."... 2011/11/17 23:29:49 Looking up branch information for "hacking/lbox"... 2011/11/17 23:29:49 Found landing target: bzr+ssh://bazaar.../lbox/ (...)
This command will ask you for a few details interactively, like your authentication details in Launchpad and in Rietveld (your Google Account, details sent over SSL to Google itself; you may have to visit Rietveld first for that to work), and also the change description.
In case something fails, feel free to simply execute the command again, as many times as you want. The command is smart enough to figure that an existing merge proposal and change in Rietveld exist and will update the existing ones with the new details you provide, rather than duplicating work.
Once the command finishes, you can visit the URL for the merge proposal in Launchpad that was printed, and you should see something like this:
Note that the change description already includes a link onto the Rietveld issue at codereview.appspot.com. The issue on Rietveld will look something like this:
Observe how the issue has the same description as the merge proposal, but it links back onto the merge proposal. At the left-hand side, there’s also an interesting detail: the original merge proposal email has been added as the reviewer of this change. This means that any changes performed in Rietveld will be mailed back onto the merge proposal for its record.
In the center you can find the meat of the whole work: the actual change set that is being reviewed. Rietveld works with patch sets, so that you can not only see a given change, but you can also review the history of proposals that the proponent has made, and any inline comments performed in them.
Click on the side-by-side link next to Makefile to get an overview of the actual change, and to make comments on it just click on the desired line:
Your comments won’t be sent immediately. Once you’re done making comments and want to deliver the review, click on the “Publish+Mail Comments” link at the top-right, which will take you onto a page that enables complementing with any heading details if desired.
Since the merge proposal is registered as the reviewer of the issue in Rietveld, publishing the review will deliver a message back onto the merge proposal itself, including context links that enable anyone to be taken to the precise review point back in Rietveld:
Then, once you do make the suggested changes and want to publish a new version of the branch, simply repeat the original command: “lbox propose -cr”. This will push the new diff onto Rietveld and create a new patch set. You’ll also be given the chance to edit the previous description, and any changes there will take place both in the merge proposal and in the Rietveld issue.
lbox also has other useful command line options, such as -bug, -new-bug, to associate Launchpad bugs with the merge proposal and put them in progress, or -bp to associate a blueprint with the branch and bug (if provided) being handled.
This should turn your code reviews in Launchpad into significantly more pleasant tasks, and maybe even save some of your precious life time for more interesting activities.
Happy reviewing!
The “rietveld package” link (https://goneat.org/lp/goetveld/rietveld) is dead.
My bad, the links on goneat are http not https. That’s fixed now.
This blog post currently serves as the only real documentation of lbox outside of the src. One trick/feature that i wanted to point out is the check facility that will verify that a branch is in a good state before merging or proposing it.
A branch with an executable .lbox.check file at its root will have that file executed and its status examined for a zero exit code. This file can be used to run unit tests, perform lint checks, etc.