User:MHolloway (WMF)/Draft/Differential code review workflow

Sending changes to Phabricator's code review tool, Differential, is accomplished via the Arcanist command line utility. Setting this up is easy, and Differential provides a pleasant, user-friendly interface. However, some CI integration and merge pipeline work remains to be done before Differential can take Gerrit's place in our workflow.

(Note: Beyond the Arcanist install step, these instructions presume that the project includes an .arcconfig file; this will be added to the Android repo when https://gerrit.wikimedia.org/r/#/c/305002/ is merged.)

Install Arcanist[edit]

Simply git clone the following repos:

https://secure.phabricator.com/diffusion/PHU/libphutil.git

https://secure.phabricator.com/diffusion/ARC/arcanist.git

Then add /path/to/arcanist/bin to your $PATH.

Alternatively, for OS X users, an arcanist homebrew formula (homebrew/php/arcanist) appears to be available, but I have not tested it.

Install credentials[edit]

Run arc install-certificate from the project folder and follow the instructions. You will be asked to retrieve a key from a Phabricator URL (after logging in, if necessary) and paste it at the prompt.

(Note: If you run into a segmentation fault error at this step, you likely need to upgrade your PHP version to 5.6 or higher. See https://stackoverflow.com/questions/32777671/arcanist-gives-segmentation-fault-11.)

Submit changes for review[edit]

Changes are submitted with arc diff [<ref commit>]. You will receive a URL to view the change in Differential, as well as an email notification.

Arcanist provides greater flexibiilty than git-review with respect to what range of commits is pushed for review as part of a given change set. A reference commit can be specified as an argument, and a default can be set. (You will be prompted to set a default, origin/master suggested, the first time you issue an arc diff command without specifying a reference commit.)

Arcanist/Differential do not provide an out-of-the-box way to auto-populate a certain set of reviewers for all changes to a given project. We can work around this by using personal Herald rules to be automatically added as reviewers to change sets ("Revisions," in Differential lingo) to all projects we maintain. (For example: https://phabricator.wikimedia.org/H183.) Otherwise, reviewers can be added with the --reviewers flag when issuing the arc diff command, or by manually editing the revision from the Differential interface.

The review interface is pretty straightforward. It provides a reviewer the option to approve a change, or to downvote and request improvements. It also offers the option to "commandeer" a revision (intended for a change we'd like to merge but that the author appears to have abandoned, for example).

Differential doesn't appear to offer a +1/+2 distinction, so we may have to create a new norm of having someone accept a relatively complex change only after another engineer has verbally signed off. However, at this point, nothing further happens when a change is "accepted," so the point is moot for now.

Next steps[edit]

CI integration[edit]

I believe all of the groundwork is in place for existing jenkins jobs to be included in a build plan in Harbormaster, Phabricator's build and CI support application. (See this existing build plan, for example.) I don't have permission to create a build plan, though; we'll probably have to ask RelEng for this. Once our build plan is created, I believe we can have the CI tests automatically run for all proposed changes to the project by setting up a Herald rule to apply a tag corresponding to the build plan when a change is pushed for review.

Merge pipeline[edit]

Changes aren't automatically merged after being accepted in code review. Publishing a change after acceptance in code review appears to be accomplished by issuing the arc land command, but I haven't tested this. I don't know whether there are any plans to automate this step similar to the current Gerrit workflow.