Fwd: Changes to code review tools and processes

[Andrew McCreight]

This was posted on mozilla.tools yesterday but doesn't seem to have made it
to dev.platform. It is likely to be of interest to many people on this
list. Note the comment "If you have questions or concerns beyond those
addressed below, please
use the mozilla.tools group and/or #phabricator on IRC and Slack to raise
them. " in the message

Andrew


---------- Forwarded message ----------
Subject: Fwd: Changes to code review tools and processes

On Thursday, May 11, 2017 at 1:46:49 PM UTC-7, mc...@mozilla.com wrote:
>
> tl;dr Splinter and MozReview will be fully replaced by Differential,
> Phabricator’s code-review tool, in Q3.
>
>
> Why the change?
>
> Code review is a critical part of the Firefox engineering process but
> hasn’t changed much since Mozilla’s early days. Our home-developed
> tools have worked well for us but are increasingly dependent upon
> scarce resources to maintain, build and customize. Using our own tool,
> something that isn’t used by other organizations, also makes
> onboarding new people, staff and volunteers much more time-consuming
> than it needs to be.
>
> As with our decision to move forward with Project Dawn, this change is
> an investment we need to make now (so we get the benefits as soon as
> possible) to ensure the long-term health of Firefox. We are confident
> the short-term/immediate costs incurred—standing up a new tool, having
> to shift expectations and refactor workflows—will be more than paid
> back with surplus time and energy available for process automation
> (and later, with faster onboarding).
>
> If you have questions or concerns beyond those addressed below, please
> use the mozilla.tools group and/or #phabricator on IRC and Slack to
> raise them.
>
>
> ~ Laura Thomson & Mark Côté
>
> ===
> FAQs
>
> * Why did we choose Differential?
>
> Differential, originally developed at Facebook and currently used by
> many organizations, both open- and closed-source, has been trialled by
> several teams at Mozilla over the last few months. Although no code
> review tool is perfect, we believe it addresses several of the
> deficiencies in Review Board and is better suited to the Firefox
> engineering process. It is also an opportunity to decouple our
> automation pieces in order to focus on them after Differential’s
> launch, where we believe we will have a greater impact to Firefox
> development.
>
> * What will go out in the initial launch?
>
> 1. Deployment of a central Differential (code review) installation,
> supported by CloudOps, along with supporting tools like Diffusion
> (repository browser) and Herald (event-driven notifications). We will
> continue to use our Bugzilla instance, bugzilla.mozilla.org (aka BMO),
> for issue tracking for the foreseeable future.
>
> 2. Lightweight integration of Differential with BMO. This will include:
> - Authentication/identity.
> - Links from Differential revisions to BMO bugs.
> - Mirroring approvals (only) of Differential revisions back to BMO
> bugs as r+s on stub attachments, for record-keeping purposes. Aside
> from this, the code-review process will occur only in Differential to
> avoid fragmenting the discussion.
> - Security-group mapping to support reviews of security and otherwise
> confidential patches as seamlessly as possible.
> - Integration with and improvements to the Autoland service, which is
> currently used for around 50% of commits to mozilla-central and an
> integral part of the Quantum Stylo project.
>
> * What will happen to Splinter and MozReview?
>
> After a short trial period, MozReview will be shut down, and Splinter
> will be turned off for the main Firefox products on BMO: Core,
> Firefox, and Toolkit. Contribution guides and documentation will be
> updated to refer to Phabricator for code-change submissions. The exact
> archival procedures are will be figured out soon.
>
> * Will I be able to request customizations in Differential?
>
> We plan to limit customizations, but not to zero. We’re making this
> change (to using a third-party tool) to reduce the support burden on
> internal teams. The fewer customizations we make the more that
> happens. Any customizations will generally use Phabricator’s Conduit
> API; extensions will be limited to changes that are not possible to
> implement via the API. We have no intention of forking any Phabricator
> tools.
>
> * How will I get patches/commits up for review?
>
> We intend to keep the general Phabricator workflow, including use of
> the Arcanist command-line tool, although we will likely provide easy
> installation and some abstraction via mach and MozillaBuild.
>
> * Where will the flags for feedback/review/ui-review be?
>
> We’re going with the review flow built into Differential, which does
> not support multiple types of flags as Bugzilla’s attachment-flag
> system does. However, the actions that a reviewer can perform in
> Differential are more straightforward: rather than setting “r+” or
> “r-”, Differential’s options include “resign as reviewer”, “request
> changes”, and “accept revision”, which roughly map to clearing a r?
> flag, setting r-, and setting r+, respectively. Differential also
> distinguishes between a reviewer and a “blocking reviewer”, which
> could be seen as requesting feedback versus requesting review.
> Finally, compared to Bugzilla, Differential has a clearer indication
> of the current state of the reviews on a revision and what needs to
> happen.
>
> We’re using the opportunity presented by switching to a new code
> review tool to try out this simpler approach, which will particularly
> benefit new contributors. This is possibly the biggest change in
> process, and we know it may take some time to get used to.
>
> * Will I be able to push to review?
>
> Although the ability to use hg/git’s “push” command to submit patches
> for review was one of the much-appreciated aspects of MozReview, it’s
> a tricky thing to get right. We never solved the problem of
> confidential patch review in MozReview, since mapping Bugzilla’s
> security groups to the Mercurial review repository is rather
> difficult. Our support for micro-/stacked commits was also a bit more
> confusing than we liked. We do have some ideas on how to fix both of
> these things, however, so if it’s a popular feature request we will
> try to work it into a future release.
>
> * Will there be alternative code review tools available?
>
> No single tool is going to make everyone happy but we don’t plan to
> support multiple tools.
>
> * What will the team do if they won’t be working on the tool itself?
>
> We have plenty of things lined up already. Here are a few:
>
> - Linting upon patch submission.
>
> - Fully automated landings. Signal your intention to land a patch when
> you submit it, and after it gets reviewed by authorized developers
> Autoland will land it automatically.
>
> - Automatic conflict detection, indicating if a patch can’t be cleanly
> applied to the tree before you try to land it, or even before it’s
> reviewed.
>
> - Automatic, intelligent Try runs based on what parts of the code were
> modified.
>
> - Tracking a patch through its lifecycle: review, landing, merges,
> uplifts, and any backouts along the way.
>
> The pattern here is that these are automated analyses and actions that
> are part of a pipeline from patch submission to review to landing to
> release. These are the steps in the engineering process that are
> largely unique to Firefox’s complex and highly scaled nature. Our team
> believes that the biggest positive impact to developer productivity
> lies in this area.
>
> * Did we consider building this automation out with MozReview?
>
> MozReview started out as an experiment and prototype that
> simultaneously delivered a new code review tool with some process
> automation. A lot of this automation was built into Review Board
> extensions and, later, into a custom fork. We also customized the tool
> to mimic the BMO patch review process as closely as we could.
>
> Making major changes to Review Board and tightly coupling process
> automation to it resulted in a heavily constrained development
> environment and increasing maintenance burdens. Last year we came to
> the realization that a major architectural change and a refocusing of
> priorities was overdue.
>
> At the same time we took a hard look at Review Board and concluded
> that, even without the added complexities of our customizations, it
> had some problems and deficiencies that were rather difficult to fix,
> including loading times, inaccuracies and omissions in diffs, and
> basic UI structure. Taking a look at alternative code-review
> solutions, of which none are perfect, it seems that Differential
> better meets Firefox engineering’s needs and already has some
> supporters at Mozilla.
>

[Randell Jesup]

>This was posted on mozilla.tools yesterday but doesn't seem to have made it
>to dev.platform. It is likely to be of interest to many people on this
>list. Note the comment "If you have questions or concerns beyond those
>addressed below, please
>use the mozilla.tools group and/or #phabricator on IRC and Slack to raise
>them. " in the message

I tried cross-posting my mozilla.tools response to here, but it appears
to not work. I had extensive comments in mozilla.tools about a number
of issues; I strongly advise anyone even slightly interested go there
and read them. Most important were issues around who trialed it,
security bug/patch access, control of data, archiving, ability to look
at old patches as more than a raw diff, and where review comments live.
(And more). (Mark responded, and I followed up with more comments)

They do plan to put up a Wiki page on all this soon.

--
Randell Jesup, Mozilla Corp
remove "news" for personal email