Jump to content

Requests for comment/Migrate code review and management to Phabricator from Gerrit

From mediawiki.org
Tracked in Phabricator
Task T119908
Request for comment (RFC)
Migrate code review and management to Phabricator from Gerrit
Component General
Creation date
Author(s) Chad Horohoe, Mukunda Modell, Greg Grossmeier
Document status declined
See Phabricator.

This is an RFC to migrate code-review from Gerrit to Phabricator's Differential.

Background

[edit]

We have used Gerrit for code review since the migration from Subversion to Git, circa early 2012. In 2015, we migrated from Bugzilla to Phabricator for our bug tracking. Phabricator also includes code management and review tools, and in fact these tools were a large part of the reason we chose Phabricator for our development platform.

Problem

[edit]

Gerrit has a very high learning curve for new developers (cf: task T114419), is difficult to maintain and does not integrate well with Phabricator. Continued maintenance of Gerrit has a very high cost and is yet another software tool to maintain by a small team.

Process

[edit]

See also:

Some milestones along the way

[edit]

(not all in a strict dependency order)

  1. Deprecate Gitblit (ie: git.wikimedia.org) and host all repositories in Phabricator ("Diffusion")
  2. Import all repos from Gerrit - task T616 - Yes Done
  3. Get early adopters using Differential for "small" self-contained projects - task T129301
    • Doing this will give us real-world examples of code-review in Phabricator
  4. Prototype CI with Differential - task T103127 - Yes Done
  5. Integrate CI with Differential - task T31
  6. Broadcast Differential activity to IRC - task T116330
  7. Document code review workflows - task T117058
  8. Update Code Review related wiki pages - task T207
  9. Have Phabricator take over Github replication - task T115624
  10. Stop taking in new projects to Gerrit
  11. Migrate open changes from Gerrit to Differential - task T122979
  12. Provide static dump of Gerrit - task T617
  13. SWITCH OVER (things after this are not absolute blockers/needed before switch over)
  14. Make MetricsGrimoire/korma support gathering Code Review statistics from Phabricator's Differential - task T118753

Pros and Cons of migrating to Phabricator/Differential

[edit]

see also the original Phabricator RFC

Pros

[edit]
  • Everything would be in one place
    • This benefit can not be understated
    • Single login for everything (it uses your Wikimedia SUL account)
    • Built in integration of subsystems like bug tracker, code-review, etc
    • Single interface across subsystems (reduced cognitive load/confusion)
    • Integrates design and designers (via Pholio)
    • Conversations allowed to stay in one place (instead of spread across multiple platforms)
    • Reduce technical-debt maintained by our community by no longer maintaining a family of home-grown integration bots (see: [| grrrit-wm] and potentially wikibugs2) and instead use/extend the Phabricator Bot.
  • Reducing the technical debt maintained by the WMF by:
    • Decommissioning Gitblit
    • Decommissioning Gerrit
  • Mobile-friendly
    • Login from any mobile device and see what you can read and do. Now, open Bugzilla or Gerrit...
  • Support for code auditing, with automatic notification based on herald rules and conditions
    • Audit cowboy commits triggered automatically and appropriate teams can be notified.
    • Raise concern about a commit that is already past code review.
    • Similar UI to differential but simplified and with different states for a commit:
      • "Needs audit" for commits that bypassed code review
      • "Concern raised" for manually flagged commits
      • Must be accept by someone with "ownership" of the code in question.
  • Faster and much more enjoyable code review experience (especially large diffs), and improved commit/review procedures
    • arc patch D123 vs git review -d 239028
    • differential comments are actually readable. vs gerrit is completely illegible, and if that weren't bad enough, no amount of css can fix it because the markup is pathological. Gerrit code review: Intentionally 99% css-resistant in the 21st century.
    • Multi-commit single-branch review support instead of the approach in Gerrit where each change is a single commit that is amended for each new patch set
      • But phabricator supports the single amended commit workflow for those that prefer it.
    • Better visibility of requested reviews - patches awaiting review are easier to notice thanks to dashboards, an outstanding review counter on the phabricator main menu, and phabricator notifications.
  • Possibility to let users create their own repositories
  • ease of creating feature branches?
  • Wide adoption among our peers.
    • Phabricator is used by prominent open-source projects and and non-profit, free-knowledge organizations such as:
    • These and other phabricaor users have created numerous utilities, integrations, workflows and other resources which add a lot of value to the phabricator ecosystem. Many of these resources are documented on Phabricator's wiki: Community Resources
  • We have a good working relationship with the upstream project and Phacility have gone out of their way, on multiple occasions, to accommodate Wikimedia's needs and workflows.

Cons

[edit]
  • Code review workflow differs of the Gerrit workflow when several persons want to improve a single commit: Gerrit allows to smoothly add a new revision, with author still being the initial proponent. Differential requires you commandeer the revision: you become author, and so you can't approve it as you can't self review (side effect: we so need to enable self review in the config, even if we don't socially use it). It "steals" the revision from the author, which becomes a review subscriber, and the revision is now officially yours. In pratice, this means people won't do that and will block a review for an extraneous space instead of fix it quickly, approve it, and send it to Zuul for merge.
  • Arcanist required
    • not for much longer
    • Arcanist is easy to install on every platform, ArchLinux excepted, and is also available as a Docker container
  • Transition learning curve (inherent to any migration)
  • Single Point of Failure
    • isn't gerrit a single point of failure now?
    • Phabricator supports repo mirroring and high availability clustering
    • Can be mitigated
  • Repository callsigns. This is being addressed with task T110607
  • Rewrite a bunch of bots relating to Gerrit
    • Gerritbot is no more needed (Phabricator has a built-in IRC notifier)
      • The notifier code has been broken since May 2015. Yet, something custom could be built around Doorkeeper.
    • wikibugs needs to be updated, maybe? (see pros)
  • Impact on CI, needs the middleware glue to Jenkins/Nodepool

Alternatives

[edit]

What, if any, alternatives are there to moving to Phabricator/Differential?

Status quo: Gerrit

[edit]

We could stick with Gerrit, but the general consensus has always been with Gerrit that the learning curve is very steep. It's a terrible maintenance burden.

Other tools

[edit]

Basically, the existing code review tools that aren't Phabricator can boil down to a couple of groups:

Outside hosted / closed source (Github, Gitlab, Cruicable, etc)
Generally involve close-sourced platforms. We cannot deploy from an outside service, so we would still require mirroring back to Phabricator or elsewhere internally for deployments. Gitlab does have a self-hosted option that is free, but is a scaled back version of their "Enterprise" version, which has closed source components we would need. Generally use a Pull Request model which is familiar to most people.
Gerrit-esque tools (Reitveld, Reviewboard, etc)
Basically everything else worth using that is free software is in the "Gerrit" sphere. Either they're directly related (eg: Reitveld), or they go out of their way to make their UI and workflows Gerrit-esque. See the status quo.

There's also a ton of weird one-off tools that are half-developed, abandoned, incredibly workflow-specific or technically unsuitable (platform requirements, etc).

Investigate

[edit]
Retrieved from "https://www.mediawiki.org/w/index.php?title=Requests_for_comment/Migrate_code_review_and_management_to_Phabricator_from_Gerrit&oldid=2731834"