Reviews.llvm.org read-only mode

Project Infrastructure Code Review
1

GitHub pull requests have been enabled. Update on GitHub pull requests announced “October 1, 2024: Phabricator server shutdown”.

Before the complete shutdown, we have a read-only mode of reviews.llvm.org . I believe there are quite a few unclear things about the read-only mode that are worth discussing.

  • Allow user registration? No.
  • Allow new differentials? Currently yes, but should be no at some point. There is currently a red banner to discourage new differentials (Update on GitHub pull requests - #76 by MaskRay) Can someone figure out how to patch src/applications/differential/conduit/DifferentialConduitAPIMethod.php to forbid new differentials?
  • Allow updating existing differentials? Currently yes, but should be no at some point (Oct 1?)
  • Allow “Add Action”(Accept/Request Changes/Close/Abandon/etc)?
  • Allow comments on Differentials (/Dxxxxx)? Seems useful to me if comments can work a bit longer.
  • /rGxxxxx pages? Redirect to GitHub? What about old comments?

With existing information I have, the read-only mode is changing this variable:

phab@llvm-reviews:/srv/http/phabricator$ grep read-only conf/local/local.json
  "cluster.read-only": false,

Phabricator has limited capability of read-only mode: :anchor: T4571 Allow Phabricator to run in Read-Only Mode . Hopefully someone familiar with PHP or is willing to investigate can figure out how much the code can achieve our goals. If not, we may need lightweight patches or workarounds (e.g. Apache RewriteRule).

1 Like
2

I can to investigate (and maybe patch it), but since 13 Sep. If no one else will.

1 Like
3

Why not? It’s useful for people to be able to ping on a revision.

I was looking at this earlier, something simple like this may be enough but I don’t have a way to test it right now of course:

diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php
index 74fee8221956..836e25fe51eb 100644
--- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php
+++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php
@@ -128,6 +128,10 @@ final class DifferentialRevisionEditEngine
     }
 
     $is_create = $this->getIsCreate();
+    if ($is_create) {
+      throw new Exception('New revision creation is disabled, please use GitHub Pull-Requests.');
+    }
+
     $is_update = ($diff && !$is_create);
 
     $fields = array();

I would look into some data-driven approach: after we stop accepting new revision, we should be able to know how many revision are still updated every day/week and take it from there!

4

Thanks for the solution. I’ll try this when it’s decided that we shall disable new differentials.

Sounds good!

2 Likes
5

I think there are two main reasons to disable functionality:

  1. To redirect developers to use PRs instead of Phab
  2. To reduce the load and maintenance burden of Phabricator

I think blocking creation of new differentials is enough to drive folks to migrate to pull requests. Disabling the other functionality is mostly a way to shed load and reduce maintenance cost. So, broadly speaking, I am in favor interpreting “make Phab readonly on Oct 1” as meaning “disable the creation of new diffs on Oct 1”, assuming we have that option, which wasn’t clear at the time that we discussed and agreed on the timeline.

One other suggestion I could add to reduce Phab load would be to stop importing new revisions from GitHub. This will mean folks can’t navigate to /rGxxx for new revisions and start adding new post-commit review comments on those changes. That would encourage folks to start linking to github.com/llvm/llvm-project/commit/xxx instead.

3 Likes
6

Disabling creation of new Differentials on Oct 1 looks good to me.

I am unclear how to stop importing new revisions, but the functionality is certainly related to auto closing a differential. Disabling importing it now will disable auto closing, which can be confusing by itself.

Redirecting /rGxxx pages to GitHub will disable discussions on these pages. Perhaps someone can figure out whether it is feasible to back up the pages, otherwise I will assume that the data is not worth backing up.

7

I’d like if we can continue dealing with open reviews on Phabricator and only block opening new diffs. My deadline for dealing with Phabricator is this Friday and I have somewhere around 50 open outgoing revisions which I certainly do not have time to get through

3 Likes
8

I proposed a new timeline here, hopefully it addresses your concerns. (Oct 1 no new differentials, but existing reviews may continue)

9

New differential creation is now disabled. I have tested that updating an existing differential (arc diff 'HEAD^') still works.

% arc diff 'HEAD^'
Launching editor "nvim"...
Provide the details for a new revision, then save and exit.
Linting...
No paths are lintable.
Running unit tests...
No unit test engine is configured for this project.
 SKIP STAGING  No staging area is configured for this repository.
 Exception
ERR-CONDUIT-CORE: <differential.revision.edit> New revision creation is disabled. Please use GitHub Pull Requests instead.
(Run with `--trace` for a full exception trace.)
2 Likes