Page MenuHomePhabricator

Stop using Differential for code review
Open, Stalled, LowPublic

Description

IMHO the current state is the worst possible outcome. In addition to what we already had for most (gerrit) or some (github) of our repos, we now have a third place for some others (some tools on toolforge, Scap and a few more host their code on diffusion and do code review on differential).
Especially from a new contributors perspective, this is bad. Depending on which project I want to contribute to I now have to familiarise myself with github, gerrit and/or differential. Personally, when I started contributing here about a year ago, I didn't know about any of those and wouldn't have cared which one to learn. But by now, I had to dig into differential as well as gerrit, because I wanted to contribute to different projects over time.
This is an acceptable state for a (gerrit->differential) transition period. It isn't when the transition was declined. One can argue a lot about whether gerrit or differential fits our purpose better (which was done) - but I hardly can imagine how keeping both isn't much worse than having either of them.
tl;dr: The migration gerrit->differential being declined and gerrit being supposed to stay around, we should consider to shutdown differential and migrate existing projects from there to gerrit.

(Originally posted as T119908#4096033)

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Aklapper reopened this task as Open.Apr 6 2018, 1:01 PM

I explained in T191182#4103647 why I think this is wrong. So I am reopening this task. If Differential has no future in Wikimedia development then projects should get migrated and it should be turned off. Leaving yet another lingering system around just adds to technical debt, confusion, fragmentation.

I've received a pull request for a tool forge tool on GitHub. Honestly, I forgot this tool source code were there and I suspect it was there because Gerrit doesn't have by repo deploy keys. So I concur if fragmentation venues can be confusing for maintainers, it can really be annoying for contributors.

demon removed a subscriber: demon.Apr 11 2018, 6:46 PM
Dzahn awarded a token.Jun 13 2018, 7:21 AM
Dzahn added a subscriber: Dzahn.Jun 13 2018, 7:24 AM

If you mean internal WMF dev-ops stuff that only WMF employees care about, then perhaps that matters less

Not really, it still matters for us as well. I also would like to have to use only 1 system and not multiple ones. If something is hosted on differential that makes it less likely for me to want to upload a quick typo-fix kind of patch.

I explained in T191182#4103647 why I think this is wrong. So I am reopening this task. If Differential has no future in Wikimedia development then projects should get migrated and it should be turned off. Leaving yet another lingering system around just adds to technical debt, confusion, fragmentation.

Would turning off Differential loose the content it already has and break links to it?

I explained in T191182#4103647 why I think this is wrong. So I am reopening this task. If Differential has no future in Wikimedia development then projects should get migrated and it should be turned off. Leaving yet another lingering system around just adds to technical debt, confusion, fragmentation.

Would turning off Differential loose the content it already has and break links to it?

Yes, "turning off" in the sense of clicking "Uninstall" in the settings would break links and hide the content (no more publicly accessable type of lost - but not deleted). I agree that doing so is a bad idea, and I assume we wouldn't do that. The proposal here is to stop using differential. There's probably other ways (maybe one can revoke the right of creating new diffs?) to achieve that without breaking what's already there.

HappyDog renamed this task from Consider disabling differential to Stop using Differential for code review.Jun 13 2018, 8:28 AM

The proposal here is to stop using differential.

This has come up a couple of times - I've updated the ticket title, to hopefully avoid further confusion.

The monthly Phab summary emails on wikitech-l@ show that we've had {20,23,19,22,19,28,29,30,19,26,24,24} Differential users in the last 12 months.
I'm clueless how to get a complete list of projects using Differential though.

I guess SELECT r.name FROM phabricator_repository.repository r JOIN phabricator_differential.differential_revision d WHERE r.phid = d.repositoryPHID AND FROM_UNIXTIME(d.dateModified,'%Y%m')=date_format(NOW() - INTERVAL 1 MONTH,'%Y%m'); should give us a list of repos that used Differential in the last calendar month, if anyone feels like running that.

Dzahn added a comment.Jun 21 2018, 5:16 AM
mysql:phuser@m3-slave.eqiad.wmnet [(none)]> SELECT r.name FROM phabricator_repository.repository r JOIN phabricator_differential.differential_revision d WHERE r.phid = d.repositoryPHID AND FROM_UNIXTIME(d.dateModified,'%Y%m')=date_format(NOW() - INTERVAL 1 MONTH,'%Y%m');
+------------------------+
| name                   |
+------------------------+
| Differential Testing   |
| Scap                   |
| tool-quickstatements   |
| Scap                   |
| tool-quickstatements   |
| Scap                   |
| Scap                   |
| Scap                   |
| Scap                   |
| Scap                   |
| Scap                   |
| Scap                   |
| Scap                   |
| Scap                   |
| Scap                   |
| Scap                   |
| Scap                   |
| Scap                   |
| Scap                   |
| phabricator-extensions |
| tool-quickstatements   |
| Scap                   |
| Blubber                |
| tool-quickstatements   |
| Blubber                |
| phabricator-extensions |
| Blubber                |
| Blubber                |
| Blubber                |
| Blubber                |
| Blubber                |
| Blubber                |
| wikimedia-iegreview    |
| Blubber                |
+------------------------+
34 rows in set (0.02 sec)

As for Scap there's T191373: Move Scap development from Differential to Gerrit.

As for the others, I haven't looked if there's a proposed migration task.

@Dzahn could you re-run the query to see if there are any other repos (excepting Toolforge ones) we've missed using Differential?

Dzahn added a comment.Dec 20 2018, 6:43 AM

@Legoktm here you go

MariaDB [(none)]> SELECT r.name FROM phabricator_repository.repository r JOIN phabricator_differential.differential_revision d WHERE r.phid = d.repositoryPHID AND FROM_UNIXTIME(d.dateModified,'%Y%m')=date_format(NOW() - INTERVAL 1 MONTH,'%Y%m');
+--------------------------------+
| name                           |
+--------------------------------+
| MediaWiki Release Tools        |
| Scap                           |
| Scap                           |
| Metrolook                      |
| Scap                           |
| Phabricator                    |
| Scap                           |
| Scap                           |
| Scap                           |
| Scap                           |
| tool-ws-google-ocr             |
| Wikibase Data Model JavaScript |
| Scap                           |
| tool-texbot                    |
| wikimedia-iegreview            |
| wikimedia-iegreview            |
+--------------------------------+
16 rows in set (0.01 sec)

The way i got this: 2 terminal windows, in one ssh to phab1001.eqiad.wmnet ; mysql -u phuser -p -h m3-slave.eqiad.wmnet -P 3323 . and in the other: ssh puppetmaster1001.eqiad.wmnet ; view /srv/private/modules/passwords/manifests/init.pp and look for $app_pass in class passwords::mysql::phabricator and paste that password to login. Then run the query above.

hashar added a subscriber: hashar.Feb 7 2019, 9:23 AM

Apparently all repositories have been migrated. I guess we can now sunset Differential/disable it somehow?

We need to migrate phabricator source code to gerrit (including phabricator/deployment).

bd808 added a comment.Feb 7 2019, 6:19 PM

Apparently all repositories have been migrated. I guess we can now sunset Differential/disable it somehow?

Wikimedia Foundation production deployed software may no longer be dependent on Differential (or almost per T191182#4934766), but we do still have a use case for Differential/Diffusion in Toolforge. It is quite likely that Diffusion is the important part here with Differential being a little used add-on.

Today Striker (https://toolsadmin.wikimedia.org/) makes creating new Diffusion repos a 1-click process for any Toolforge tool. When that is the way that a tool chooses to version and publish its source then Differential is the only code review system available. I very deliberately did not choose to build the integration with Gerrit instead of Diffusion for 2 reasons. One of those reasons is now moot; namely that Diffusion is no longer intended as the global replacement for Gerrit. The other reason however is that the Gerrit/Zuul/Jenkins workflow is complex for new users to understand. It is a powerful system, but not something that we have invested in making easy. We instead invest in making it a good way to support the very specific needs of the movement's most sensitive software. This is not a bad thing, but I do not believe this is a one size fits all problem space. Fragmentation makes discovery and by extension reuse more difficult, but there are portions of our technical community, like Toolforge, where we are much earlier in the software development maturity cycle and added complexity in adoption of basic recommended practices will inhibit adoption.

The other reason however is that the Gerrit/Zuul/Jenkins workflow is complex for new users to understand. It is a powerful system, but not something that we have invested in making easy. We instead invest in making it a good way to support the very specific needs of the movement's most sensitive software. This is not a bad thing, but I do not believe this is a one size fits all problem space. Fragmentation makes discovery and by extension reuse more difficult, but there are portions of our technical community, like Toolforge, where we are much earlier in the software development maturity cycle and added complexity in adoption of basic recommended practices will inhibit adoption.

I think we can have toolforge Gerrit repos created by striker be open push by default, so they're just like any other git host where you just push. Users who want to do pre-merge code review with CI, etc. are free to do so, but otherwise I think we can get a very similar user experience that Diffusion currently has, but on Gerrit.

Probably this should be split to a separate task? 1) Allowing Striker to create Gerrit repos and 2) Migrating existing Striker repos to Gerrit.

As a tool author, I can confirm that the only reason I use Diffusion for hosting my tools’ source code and recommend it to others in cookiecutter-toolforge is how Striker makes it completely frictionless to create new repositories. On the other hand, personally I intensely dislike Differential for contributions both as a patch author and as a tool maintainer, and recently decided to mirror my tools to GitHub as well. If Striker was changed to create open-push Gerrit repositories instead of Diffusion repositories (well, I suppose at least for a while it would have to be “in addition to”, not “instead of”, to ease the transition?), I would gladly migrate to that.

Requesting a Gerrit repo is just a matter of asking at https://www.mediawiki.org/wiki/Gerrit/New_repositories/Requests. QChris and myself handle most of them in due time (more him than me). Perhaps Striker could be configured to create standard labs/tools/$toolname repos with standard settings though, but that'd also require creating a group (ie: tool-$toolname) gerrit group to grant them ownership of the repo mentioned before. A discussion about migrating the creation request page is happening at https://www.mediawiki.org/wiki/Talk:Gerrit/New_repositories/Requests.

Requesting a Gerrit repo is just a matter of asking at https://www.mediawiki.org/wiki/Gerrit/New_repositories/Requests. QChris and myself handle most of them in due time

I don’t doubt that (and thank you!), but to me that’s still a higher barrier than clicking a button in Striker :)

We could do something similar with Gerrit. Roughly:

  • create the git repository under a Gerrit namespace such as labs/tools/ or wmcs/toolforge or whatever name really
  • create a Gerrit group named upon the repository (eg labs-tools-Foobar-owner or wmcs-toolforge-Foobar-owner ) which contains the user that created the repository
  • add that group as the project owner which should grant all permissions on the repo, including push? If not we could enable push right to project owners on the parent repo and that right will be inherited to the other.

Of course, that requires some amount of work to happen on Striker side which is some decent amount of work.

@bd808 has a good point about the CI configuration. Our system is more geared toward supporting the production deployed software and protect production. It is not that self serve and far from being a one click configuration. That being, we made it slightly easier to configure, albeit it still require a patch to be send (or a task to be filled) + a manual deployment.

We could do something similar with Gerrit. Roughly:

  • create the git repository under a Gerrit namespace such as labs/tools/ or wmcs/toolforge or whatever name really
  • create a Gerrit group named upon the repository (eg labs-tools-Foobar-owner or wmcs-toolforge-Foobar-owner ) which contains the user that created the repository
  • add that group as the project owner which should grant all permissions on the repo, including push? If not we could enable push right to project owners on the parent repo and that right will be inherited to the other.

Even if you reduce the friction involved with getting a gerrit repo that won't reduce the friction for actually using gerrit which is still quite significant for newcomers.

We could do something similar with Gerrit. Roughly:

  • create the git repository under a Gerrit namespace such as labs/tools/ or wmcs/toolforge or whatever name really
  • create a Gerrit group named upon the repository (eg labs-tools-Foobar-owner or wmcs-toolforge-Foobar-owner ) which contains the user that created the repository
  • add that group as the project owner which should grant all permissions on the repo, including push? If not we could enable push right to project owners on the parent repo and that right will be inherited to the other.

Even if you reduce the friction involved with getting a gerrit repo that won't reduce the friction for actually using gerrit which is still quite significant for newcomers.

That's been improved in newer gerrit releases.

That's been improved in newer gerrit releases.

Only slightly.

https://phabzilla.wmflabs.org/file/data/fhrpawbwgmwsrxuludwl/PHID-FILE-43hi2yjjgqyuytqnvf7x/Screenshot_2019-03-12_at_23.45.23.png :)

Helps navigate users to create there first change.

Also the Gerrit Frontend team are always listening and i can bring up any feedback (as i'm on the team as a non googles now).

Dalba removed a subscriber: Dalba.Mar 19 2020, 5:50 AM

Even if you reduce the friction involved with getting a gerrit repo that won't reduce the friction for actually using gerrit which is still quite significant for newcomers.

If we set the repositories to open push by default (which allows maintainers to just push changes without needing review) like most other git hosts, then I think for the primary audience we're discussing, Toolforge tool maintainers, there's no extra friction as compared to other git hosts, including Phab. Using Gerrit's review interface only comes into play if someone wants to submit a patch, in which case I think the friction of setting up Gerrit/git-review is roughly the same as Phab/arcanist. But when it comes to needing help, I think we have much better Gerrit docs and people who can help others using Gerrit that it seems like a better choice for the community as a whole.

Dzahn changed the task status from Open to Stalled.May 20 2020, 10:48 AM

This is apparently stalled because T252910 has been declined with a reason that there were only few commits in a specific repo. That seems to cause a dependency cycle though which i find pretty unfortunate because what EddieGP describes here is totally legit and really the worst possible outcome.

This is apparently stalled because T252910 has been declined with a reason that there were only few commits in a specific repo. That seems to cause a dependency cycle though which i find pretty unfortunate because what EddieGP describes here is totally legit and really the worst possible outcome.

I agree that this is stalled, but @Dzahn's explanation ignores all of T191182#4935787 and the literal fact that there are nearly 350 Diffusion repos for Toolforge tools which have been created through Striker. My declining moving 1 of these 350 repos to gerrit was not the root cause of the blockage here.

Yes, fixing Striker appears necessary given the terminal status of our support for Differential. That would be T224676 plus migration for the existing repos.

Dzahn added a comment.May 21 2020, 6:37 AM

@Dzahn's explanation ignores all of T191182#4935787 and the literal fact that there are nearly 350 Diffusion repos for Toolforge tools which have been created through Striker. My declining moving 1 of these 350 repos to gerrit was not the root cause of the blockage here.

Ok, sorry about that comment @bd808. I understand better now.

Demian added a subscriber: Demian.May 25 2020, 7:26 AM
bd808 changed the status of subtask T224676: Add Gerrit support to Striker (toolsadmin) from Open to Stalled.Jul 21 2020, 11:19 PM

Uhm.

As already said in T256455 we can understand some of these concerns. For example, it's acceptable to avoid to promote GitHub while it has nothing to do with Wikimedia and can create technical and political confusion.

But... is disabling Differential our solution?

«The majority used Gerrit. The majority does not migrate to Differential. I dislike Differential. Let's disable Differential».

Are these neutral and comprehensive reasons?

We should not try to change community workflows without strong reasons. A strong reason may be: security concerns, instability, incompatibility with our vision, etc.

We should not change community workflows... and not in the name of "new contributors perspective" (= different from our perspective) because the missing point of view here is that a newcomer may not like to learn both Gerrit and Phabricator, while we know that everything can be done with just Phabricator.

Is disabling Differential for everyone a 100% good idea?

We should not try to change community workflows without strong reasons.

Things change most of the time, I'd say. We migrated from SVN to Git, from the CodeReview extension to Gerrit, from Bugzilla to Phabricator, etc.

(However, the word "change" feels a bit strange when you have not used Differential before but have used Gerrit? Maybe I misunderstand your point here.)

Not using three or more different code hosting and review tools (Gerrit, Differential, Github, random other tools prefered by tool maintainers such as Sourceforge or Bitbucket, pasting code on wiki pages, etc) to confuse most people about which project is where and how each of these tools work if I plan to contribute to more than one project feels like a valid reason to me.

a newcomer may not like to learn both Gerrit and Phabricator, while we know that everything can be done with just Phabricator.

In the first case, I'd have to learn two things: Gerrit and Maniphest (task tracking part of Phab).
In the second case, I'd have to learn two things: Differential (code review part of Phab, e.g. fiddling with arc) and Maniphest (task tracking part of Phab)?

valerio.bozzolan added a comment.EditedAug 2 2020, 3:32 PM

when you have not used Differential before but have used Gerrit? Maybe I misunderstand your point here.)

Apologies if I admit I do not feel much confortable with this comment. Maybe because my contributions are not interesting at all in this task. Anyway, to keep it clear, I just love Phabricator. My private Phabricator instance lives at gitpull.it and the company I work for in real life uses Phabricator, so I started appreciating Phabricator and its workflows.

What I'm saying here is that we have a powerful tool and that may be appreciated by the community. It's not like deprecating SVN or Bugzilla (with all respect for these technologies, we know they made their story).

Aklapper added a comment.EditedAug 2 2020, 3:39 PM

Ah, thanks for the clarification! I apologize for not having thought of experience in other organizations; it was not my intention to make you feel uncomfortable.

What I'm saying here is that we have a powerful tool and that may be appreciated by the community.

The community has had plenty of time to appreciate the tool, in my opinion, and it hasn’t happened. According to this query, unless I’m mistaken, there have been just 28 Differential revisions this whole year. Twenty-eight. Some of my own tools have had more activity than that just in one repository. (Admittedly, that’s not a fair comparison, since those commits weren’t peer-reviewed. A better comparison would be reviewed tool changes on Gerrit in the same time; I don’t know how to get a total number for those – there doesn’t seem to be a common prefix for “tools” projects to filter by – but Pywikibot alone seems to have some 660 reviewed changes this year, many of them not self-merges by the looks of it.)

It would probably be possible to invest some effort to make the community appreciate Differential, but I don’t see the point, to be honest.

valerio.bozzolan added a comment.EditedAug 3 2020, 8:42 AM

The community has had plenty of time to appreciate the tool, in my opinion, and it hasn’t happened.

Yep. I'm part of a irrelevant (or not-so-relevant, you know) minority interested in cultivating this feature. Also, I'm clearly in the wrong point in the timeline: I found out too late the possibility in Wikimedia to use Differential and Diffusion but I cannot hide my smile when I discovered the workboard called "New Phab repo" in Diffusion-Repository-Administrators, while I only knew that everything in the documentation was pointing to Gerrit.

With these premises, I'm aware I probably cannot defend this feature now, without looking like the only one with this interest.

Dzahn added a comment.Aug 3 2020, 4:07 PM

Not using three or more different code hosting and review tools (Gerrit, Differential, Github, random other tools prefered by tool maintainers such as Sourceforge or Bitbucket, pasting code on wiki pages, etc) to confuse most people about which project is where ...

Which is why there are concerns about adding a 4th one (Gitlab) soon with the reason that it's supposed to make it easier for new contributors. We keep adding new things but rarely ever fully undeploy anything.