Juju @ github – The Life of a Bugfix

 How to Deal with an Issue in Juju-core

UPDATE: The workflow described below was recently simplified. I’m working on updating this to reflect those changes.

One day after I have returned from the great and productive Juju Sprint in the beautiful Brussels, Belgium, I revisited the awesome Juju Academy website. My skills to write juju charms needed a refresher course. For an upcoming blog post I’m drafting, I need to write a couple of example charms. They will demonstrate how to take advantage of the significant improvements to juju related to networking and TCP/UDP port management — more on that soon.

While using Juju Academy’s first lesson, I found a small typo in the `juju help commands` output. Decided to write this quick blog post to explain the process of proposing a patch to Juju, getting it reviewed, and landing a bugfix. It could serve as an example of how you can easily contribute to Juju as well!

Discovering the Issue

What was the issue? Take a look at this paste: http://paste.ubuntu.com/8551037/.

As you can see the `api-endpoints` command has a “purpose” documentation string starting with a uppercase letter:

`api-endpoints    Print the API server addresses`

All the other commands’ “purpose” string begins with a lowercase letter, like:

`…
bootstrap        start up an environment from scratch
debug-hooks      launch a tmux session to debug a hook
debug-log        display the consolidated log file
…`

The typo becomes more apparent when running `juju help api-endpoints` (not implemented in the Juju Academy commands, but can be run locally, if you have juju installed): http://paste.ubuntu.com/8551081/ (if you’re wondering what’s that “[maas-trusty]” prefix in my bash prompt, take a look at Current Juju Environment Name in Bash Prompt).

At first I decided just to propose a patch with a single changed character in the diff (replacing “Print” with “print”). That would have been possibly the smallest patch ever 🙂

A better fix would be to write a unit test to verify all juju commands have consistent documentation strings.

Filling a Bug Against Juju-Core

We encourage all users and contributors to file bugs against juju-core in Launchpad: https://bugs.launchpad.net/juju-core/+filebug

That way we can keep track of outstanding issues and prioritize what gets fixed for this or upcoming releases. Before filing a bug, please do a quick search in the existing bugs to see if the issue is already known. You can also send a mail (and subscribe) to the juju@lists.ubuntu.com (users mailing list), juju-dev@lists.ubuntu.com (developers and contributors mailing list), or ask a question in #juju or #juju-dev IRC channels on FreeNode.

Having done the check if the issue is already known, I filled bug #1380521. Usually, if the issue is critical or affects a lot of users, it gets triaged soon and is scheduled for a fix as soon as possible. Using the channels described above, you can get in touch with the juju-core team and explain the issue is blocking you. A workaround can be available while the fix is being done.

Since I’m also a juju-core developer and can triage bugs myself, I assigned the bug to me and set its priority as Low and status In Progress, as I’m already working on a fix. If you’re a user or external contributor, the bug you’ve just filed will appears as New and the team will be notified to investigate the report and triage the bug.

Creating and Proposing a Bugfix Branch

Juju-core source code is hosted on Github, while bugs are still hosted on Launchpad (where the source used to be before the migration). Assuming you have a Github account already, you’ll need to fork “juju/juju” main repository into your own forked repo on Github, and finally clone it locally to work on the bugfix branch. Take a look at README.md file (displayed on https://github.com/juju/juju repo page) and CONTRIBUTING.md for more information on how to build juju from source and contribute. Since migrating to Github we encountered some issues with just using Github pull requests (PR) and decided to adopt ReviewBoard (RB; we use a self-hosted version at http://reviews.vapour.ws/) as an extra step during code review. There are some plans for automating the process (e.g. automatically create a RB diff whenever a PR against juju:master is created, automatically close the RB diff when merged or discared, etc.), but for now it’s needed to manually create a RB diff after the PR, using the RB’s CLI tool `rbt`.

As any juju-core team member, I have my local fork of juju: https://github.com/dimitern/juju and use a couple of scripts and git plugins locally to make my workflow easier: git-sync (pulls upstream/master, updates my fork’s origin/master, interactively rebases my current changes on top of master) and git-propose (pushing a local feature branch into my fork, creating a Github pull request), as well as a few useful git aliases (displayed by running `git config -l`). Both git plugin scripts need to be in `$PATH` somewhere, so git can find them and you can run them as `git sync` or `git propose`.

First, I `cd` into my juju-core working dir, then run `git sync` while I’m on the “master” branch. Then, I can create the bugfix branch off master, `git new api-endpoints-fix-help`, and start working on a fix.

As mentioned earlier, I don’t just want to fix the typo, but also add a unit tests to verify all commands. The best place for it is in `cmd/juju/main_test.go` along with other similar tests. I’ll call it `TestAllCommandsPurposeDocCapitalization` (naming is hard I know), and using the pre-existing `TestEnvironCommands` I’ll use a similar approach to loop over all registered commands.

I’ll show you the source of the patch a bit later, but the process essentially is:

• write the unit test;
• run `go test -gocheck.v -gocheck.f TestAllCommandsPurposeDocCapitalization` to make sure it fails for `api-endpoints`;
• fix the command’s Purpose string in `cmd/juju/endpoints.go`;
• re-run the test to make sure it passes;
• finally propose the patch as a PR (`git-propose`) and RB diff (`rbt post -o`).

Having done all this, I add a comment on the created PR (https://github.com/juju/juju/pull/904) with the RB diff link (http://reviews.vapour.ws/r/168/).

Code Review

juju-core team has adopted a policy each PR has to be reviewed and approved by at least one senior team member, or approved both by a junior team member and their “review mentor”.

You can ask for a code review either in #juju-dev IRC channel on FreeNode or on the juju-dev@lists.ubuntu.com developer mailing list, providing a link to the RB diff.

The code review process usually goes like this:

• The reviewer sends suggestions for improvements, if they see a need for it, either approving the patch (using “Ship It!” on RB), blocking it until fixed (adding a comment like “NOT LGTM until…”), or just sending comments without final approval or denial (like “I’d like fwereade to take a look as well”, or “Can we discuss this further”, etc.);
• The proposer replies to comments, fixes issues (or not, if not relevant, but explaining why), and making changes locally to the bugfix branch;
• What I usually do is commit my changes, run `git sync` to rebase against master, then `git repropose` to push changes to my fork (updating the PR diff), and running `rbt post -up` to update the RB diff.
• The original reviewer or other reviewer(s) take another look, makes more suggestions (i.e. the above steps are repeated, sometimes over a few days for complex patches) or approves the patch.
• Once the proposer have approval, they send the patch for merging (see next section).

NOTE: At the time of writing this the PR #904 have not been approved yet, but since it’s trivial I have the confidence it will be soon, and you can follow the comments on the PR and RB issue #168, if you’re interested.

Submitting an Approved Patch

When approved by code reviewer(s), the proposer of the patch needs to open the PR and add a `$$merge$$` comment, optionally (if it’s a bugfix like our case) also adding a `__fixes-##__` tag to link the PR with a bug number.

Then the landing bot we use, which is monitoring PRs for these special comments, starts a CI Jenkins job for the branch, and adds a comment to the PR like this: `Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju`. The bot then spins up a clean AWS instance using juju (of course! :), pulls the branch, merges it into juju:master, runs all unit tests, and if they pass, merges the branch into the upstream master (i.e. main juju-core repository), finally marking the PR as Merged (and comments on the PR with `Merged #123`). If the tests fail, the bot adds a comment about it on the PR (e.g. `Build failed: Tests failed — build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/894`) to inform the proposer of the failure.