Page MenuHomePhabricator

Plan to migrate code review from Gerrit to Phabricator
Closed, DuplicatePublic

Description

IMPORTANT: We are taking a more relaxed approach to address the #Code-Review project. First we want to complete the main Project Management features.

Read the task description of T584: Define main tasks (epics) for code review in Phabricator for now.

There is a proposal to build a first implementation at the Wikimedia-Hackathon-2015, see T94167: Opt-in Differential for projects needing code review but not Jenkins/Zuul for continuous integration

Details

Reference
fl42
ReferenceSource BranchDest BranchAuthorTitle
toolforge-repos/wikibugs2!17work/bd808/irc-formattingmainbd808Enhance Phorge event rendering
Customize query in GitLab

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I have scheduled a meeting on Monday, 20 October 2014, 20:00 UTC. I will be in San Francisco that week, ready to fight in rooms and corridors for the plan and the resourcing agreed in this meeting.

Goals:

The success of this meeting depends in big measure on the discussions that we can resolve or fine tune before. I have gone through all the tasks in the #Code-Review project, and I have filed under "Need Discussion" those welcoming your input.

https://phabricator.wikimedia.org/tag/code-review/board/

@chasemp, @mmodell, and @Aklapper, you are more than excused if you prefer not to get too involved while working on the RT and Bugzilla migrations. We are not going to approve anything relevant without your explicit consent. In an ideal world this planning work would happen after Day 1, but the time to define and seek resources is now.

In addition to them, @Chad and @hashar are also invited as required in order to assure expertise in our current Gerrit and Continuous Integration setups. Other contributors willing to be directly involved in this projects in the next months are welcome as well. Just let me know.

In T18#10247, @Qgil wrote:

I have scheduled a meeting on Monday, 20 October 2014, 20:00 UTC.

with whom?

Other contributors willing to be directly involved in this projects in the next months are welcome as well. Just let me know.

Add me please.

T560: Proof of concept of code review in Phabricator contains a draft description of the features expected. Feedback is welcome.

Qgil lowered the priority of this task from High to Medium.Oct 20 2014, 10:35 PM
Qgil updated the task description. (Show Details)

After some discussions today, we have concluded that we have not enough resources to have a complete assessment (including a proof of concept) and a full fledged project plan by the end of 2014.

The "stretch goal" proposed for the current quarter is a "Basic plan for Phabricator as code review tool". The exact content of this basic plan needs to be defined, but the main idea is to identify technical showstoppers and propose the next steps, keeping a stream of discussion and experimentation.

In other words, now that there is no deadline pressure and no official resource allocation, we can work in whatever areas we have a personal interest in working.

Main conclusions of our meeting today:

  • The main blocker for the migration is the lack of a specific plan for T31: [keyresult] Connect Differential code review with continuous integration. While this topic is unclear, there is no point in discussing the migration of projects deployed in Wikimedia servers. Still, we can start with projects that are not using our CI and are not planning to use it anytime soon.
  • There are social obstacles that will be only solved with proven facts and demos, and this is why it worth to start working on T560: Proof of concept of code review in Phabricator. There is a lot of low-hanging topics that can be demonstrated with Differential and Diffusion out of the box, a bit of imagination and some tweaks.
  • Mirroring all Gerrit repos in Phabricator is a goal in itself, not directly tied to code review, that would allow us to deprecate Gitblit (http://git.wikimedia.org/). We need to make sure that there is a way to turn these mirrored repositories into Phabricator local repositories. T616: Import all gerrit.wikimedia.org repositories with Diffusion needs to be updated to reflect the desired plan.
Qgil lowered the priority of this task from Medium to Low.Oct 28 2014, 2:22 PM
Qgil removed Qgil as the assignee of this task.Nov 3 2014, 12:52 PM

As said in the WMF Q3 planning page,

  • Phabricator for code review, phase out Gerrit.
    • Why maybe not: This may be premature as we'll still be in the early days of using Phabricator as a PM tool, and may not yet fully understand the requirements. It may also contend for resources with critical test infrastructure work.
    • Also, the team has been working full time at full speed with a high pressure for delivering, and now it's time to take it a bit easier, work on other things, and let Phabricator consolidate. The Code Review project may start, but slower, without being a top priority.--Qgil-WMF (talk) 07:33, 4 November 2014 (UTC)

Personally, I'm not planning to work on Code-Review until Project-Management is well on its way. If someone wants to get started, just go ahead.

Has anybody checked Releeph? (pull requests, protoype) Could this be relevant in our plans?

https://secure.phabricator.com/releeph/

https://phab-01.wmflabs.org/releeph/

Releeph seems to be a legacy / solely FB thing that mostly we were advised
to pretend doesn't exist.

Just adding a reminder about the need to discuss this plan in March if we want to work on this during the April-June quarter.

I just proposed code review in Phabricator as one of the main themes of the Wikimedia Hackathon in Lyon (23-25 May). T89084: Main themes of the Wikimedia Hackathon 2015

What do you think?

In T18#1027178, @Qgil wrote:

I just proposed code review in Phabricator as one of the main themes of the Wikimedia Hackathon in Lyon (23-25 May). T89084: Main themes of the Wikimedia Hackathon 2015

What do you think?

Can you help me understand what that means? Planned discussions / presentations ? I'm not sure what the result of it being embraced as a theme would be.

The Wikimedia Hackathon is more about getting things done and less about presentations. We could have two lines of work:

The goal being to crack enough nuts in 3 days as to have the basis for an agreed plan.

@chasemp, @mmodell, @Aklapper, and myself had a meeting about Phabricator misc topics yesterday. We agreed that if we want to work on T560: Proof of concept of code review in Phabricator in a hackathon. Wikimania is more suitable than Lyon because of dates and travel preferences.

Note that the four of us also tend to think that we can just enable Differential for whoever wants to try it out (probably starting in phab-01 to define the workflow and let people play). We have the impression that only by seeing it in action people will realize the differences in the user experience beyond the UI. We agreed that you don't even need a hackathon or an online sprint of sorts to work on this, although we do see the point of putting aside the daily routine to work as a team on this goal during a couple of days, face to face.

We are being asked to propose top priority projects for April - June 2015. For all what has been said, it doesn't look like code review migration is one to be pushed to that list. I will check again in a couple of months, but if someone wants to step in before please be my guest.

There is an interesting thread on the OpenStack-Infrastructure mailing list regarding their plans to replace gerrit:
http://lists.openstack.org/pipermail/openstack-infra/2014-March/001054.html

Just food for thought, for anyone with a few minutes to kill reading other people's opinions ;)

Okay, this task is about a plan and the current task description is terrible. I'm trashing it and starting over using some of Quim's more recent comments.

Nice, thanks for the task description edit, @Qgil! Shortly after I left my note here, I realized you'd already beat me to a detailed plan of course, so I just edited this task's description to point to that. :-)

There is an interesting thread on the OpenStack-Infrastructure mailing list regarding their plans to replace gerrit:

unfortunately, quote from there "replacing gerrit is a bit of a potential giant rathole"

In T18#1192681, @Dzahn wrote:

There is an interesting thread on the OpenStack-Infrastructure mailing list regarding their plans to replace gerrit:

unfortunately, quote from there "replacing gerrit is a bit of a potential giant rathole"

For what it's worth, the same guy in the same thread writes:

Before I expand on that - I'd like to point out that gerrit is TERRIBLE 
at dealing with the volume of reviews we all have. I believe the nova 
core team recently was discussing putting attempts at new UI shims on 
top of gerrit to try to deal with queuing and prioritization better. I, 
for one, cannot deal with the mass of stuff I'm supposed to be reviewing 
worth a crap. I'd love a better UI.

And the rest of http://lists.openstack.org/pipermail/openstack-infra/2014-March/001060.html goes on to basically say that he's not in love with Gerrit. I think many Wikimedians probably feel similarly.

tldr; gerrit sucks because it causes friction which demotivates and discourages contributions from volunteers as well as foundation employees.

I really dislike gerrit. It would be difficult to dream up a worse UI even if I dedicated a lot of time to building something intentionally terrible.

To name just a few of it's glaring problems:

  • Inline commenting is inconvenient
  • Side by side diffs are ugly and fixed width.
  • When there are long lines of code, the horizontal scroll bars don't appear until I resize my browser window slightly.
  • It's not obvious how to add reviewers, or who you should add. This part is still inconvenient even after using the tool for many months.
  • I've seen several instances where it could not merge a change no matter what you do and it offers no clues about how to resolve this.
  • Reading comments, and replying to them is incredibly uncomfortable. The tiny lines of text that have to be expanded to read the comment are terrible. The reply button is way up at the top but the comments are at the bottom.
  • Not entirely gerrits fault but I find it very annoying that +2 merges. This makes me very weary of giving a +2 on any change. Just because I approve of a change doesn't mean I want to be responsible for merging and deploying it. Why oh why do we do it this way? I may never know...

Phabricator does a much better job with inline diffs and comments. The arcanist workflow feels a lot more natural to me. Phabricator also has a lot of nice features that can be used to indicate which parts of the code base you are comfortable with so that appropriate people get notified to do reviews on changes that they are well qualified to review. It's difficult for anyone who isn't intimately familiar with the history and organizational structure of wikimedia to select appropriate reviewers for a submission. This is a problem for new contributors, and new hires. Even after nearly a year I still have a hard time getting code reviewed, it usually takes days and sometimes weeks for code to get merged. A big part of this is due to not knowing who to CC on a change. Gerrit acts as a significant barrier to contributing code. If I was a volunteer contributor it seems fairly likely that I would give up before getting my first patch merged. As an employee it still took more than a month (close to 2 months I believe) before my first patch got merged and I remember that it was very demotivating at the time.

I could go on but there's not much use in beating a dead code review tool...

It's certainly frustrating if you feel compelled to notify the people comfortable with reviewing your code, but that is based on a misconception on how Gerrit works: Potential reviewers watch projects and files either in Gerrit itself or via the notification bot; they get notified about changes they feel qualified to review. They read comments expanded in their mail clients.

Putting the burden of getting code reviewed on the submitters does not work as they do not have the ability to accomplish that goal.

And that is a problem that does not get solved by moving to another tool: The barrier to reviewing changes is not some improvable UI, but that the code review itself takes time and effort that apparently is not provided by those who can (because they probably have other things to do).

But the motivation for moving the code review process to Phabricator IIRC was not to improve the results, but to harmonize the environment with the goal of using one tool for all work.

The Phabricator RfC and the subsequent planning includes the migration of our code review process to Phabricator. The main argument was and is the integration of tools. Currently the open questions are the specific plan for continuous integration and when / how we start. All in all a problem of resources and priorities.

The blockers of the code review migration are identified above. If someone finds more blockers of this migration, please create the corresponding tasks.

Gerrit and Phabricator will always have their fans and detractors, creating online literature to read. :) However, for sanity here we should distinguish between the specific points that may change our plans and all the rest.

In T18#1193372, @scfc wrote:

It's certainly frustrating if you feel compelled to notify the people comfortable with reviewing your code, but that is based on a misconception on how Gerrit works: Potential reviewers watch projects and files either in Gerrit itself or via the notification bot; they get notified about changes they feel qualified to review. They read comments expanded in their mail clients.

I didn't know gerrit had the ability to watch projects/files. Phabricator certainly does have this feature.

$gerrit_gripes--

Putting the burden of getting code reviewed on the submitters does not work as they do not have the ability to accomplish that goal.

I agree, though I still feel like there is significant friction to getting code reviews. Anecdote but I have seen quite a few patches that have sat for a long time without any response.

And that is a problem that does not get solved by moving to another tool: The barrier to reviewing changes is not some improvable UI, but that the code review itself takes time and effort that apparently is not provided by those who can (because they probably have other things to do).

Reviewing code in gerrit is painful, in large part because it doesn't present the changes in a friendly way - it's inconvenient to simply view the diffs and even more inconvenient to discuss the changes because of gerrit's horrible commenting UI. This friction adds work to the review process which exacerbates the problem of reviewers having limited time and motivation to do reviews. When the tools work well and are enjoyable to use, we can all get more done with less effort and improved morale.

But the motivation for moving the code review process to Phabricator IIRC was not to improve the results, but to harmonize the environment with the goal of using one tool for all work.

I would argue that there are multiple motivations, which will differ significantly, depending on who you ask. :)

In T18#1193529, @Qgil wrote:

Gerrit and Phabricator will always have their fans and detractors, creating online literature to read. :) However, for sanity here we should distinguish between the specific points that may change our plans and all the rest.

ok I will refrain from further gerrit-bashing on this and related tasks.

fwiw

Gerrit is not very well liked by anyone I've talked to. I think we nearly all agree that phabricator + differential is the way forward. Currently there is not really anything major holding us back from moving to differential and we have a rough idea of how that process can proceed as a natural progression rather than a huge migration involving organization-wide disruption.

The strategy that I have advocated for a while now, and I think it is the most obvious way forward now, is to simply switch people from gerrit to differential, one team at a time, and in multiple phases:

  1. Get a few early adopters to try differential with phabricator slaved to gerrit as an upstream repository host. Good candidates for this first phase would be teams who have minimal dependence on our CI infrastructure.
  2. Fix bugs and make configuration adjustments to solidify the best-practices and workflows that meet the needs of early adopters.
  3. Start porting parts of the CI infrastructure to integrate with phabricator with the goal of phasing out jenkins & zuul interacting with gerrit directly.
  4. Move more teams to Phabricator as the CI infrastructure can accommodate.
  5. Prepare the configuration and infrastructure for phabricator to become the authoritative host of our git repositories - this is quite a bit of setup and testing involved.
  6. Finally switch gerrit to read only mode and turn off syncing from gerrit to phabricator's git repos - at this point everyone would talk to phabricator directly for all of their git needs and gerrit would become nothing but a legacy archive for historic documentation purposes only.
  7. Optionally, migrate some content from gerrit review comments into phabricator - this step will be a lot of work that probably isn't worth the effort, we could just maintain gerrit in read only mode for as long as it is considered valuable and save a ton of work. But this isn't really my decision to make.

I posit that this task has become mostly non-useful and instead people who are interested in 'what's the plan to migrate to Differential from Gerrit' should look at the workdboard: https://phabricator.wikimedia.org/tag/gerrit-migration/

T584 actually gives a better high level view of the plan (as Quim had defined it back in Oct 2014). It's mostly correct but a bit outdated. It is only outdated because Chad and Mukunda from RelEng are the new technical leaders of this work and it will become one of WMF RelEng's goals for next quarter (Oct-Dec '15, aka "FY1516Q2"). That task will be updated (or replaced, with clear/obvious links to it's replacement) as our roadmap/plan solidifies.

In summary, I recommend simply closing this task.

At this point lists of blockers / blocked by in this task are quite arbitrary. If T584 is a good starting point then yes, by all means, let's close this task and let's be all subscribed to that one.