Page MenuHomePhabricator

Get GitLab to render `T{\d}+` in MR overviews, comments, etc. as links to Phabricator
Open, Stalled, MediumPublicFeature

Description

It'd be hugely helpful if e.g. the Task reference on https://gitlab.wikimedia.org/repos/abstract-wiki/wikifunctions/function-orchestrator/-/merge_requests/18 was a link back to Phabricator, the way it is on gerrit, so that I don't have to copy-paste it to look up the context.

Event Timeline

Aklapper changed the subtype of this task from "Task" to "Feature Request".May 26 2023, 4:41 PM
Aklapper removed a project: Phabricator.

This is relevant to Phabricator, as without this kind of integration we'll need to dump it and use GitLab issues.

A potential solution would be to work upstream to implement a Phabricator external issue tracker.

After removing Phabricator code, metrics, and documentation I'm not sure how open upstream GitLab would be for including such an implementation though.

dduvall triaged this task as Medium priority.

A potential solution would be to work upstream to implement a Phabricator external issue tracker.

I experimented with this approach yesterday, and it works. The "trick" to rendering all T\d+ words is to re-implement the BaseIssueTracker#reference_pattern method.

There are two outstanding challenges to this approach.

First, the Integration model (base class for all types of integration implementations) hardcodes the names of all derived integrations as a frozen array class constant (:/) making it impossible to add to the set of available integrations without monkey patching. Fortunately, this is ruby, and we can drop this into a config initializer if we so choose:

Integration.module_exec do
  def self.integration_names
    Integration::INTEGRATION_NAMES + ["phabricator"]
  end
end

Alternatively we could maintain a patch in either a fork or a custom .deb. Regardless, we should work with upstream to make the set of available integrations less coupled with the base class and more extensible.

The second challenge is: How do we get our integration implementation into the Rails autoload path? One option is to just drop the Integrations::Phabricator class definition into the app/models/integrations directory, via puppet, a fork, or again via a custom .deb.

Another option might be to use a config initializer to append the Rails.application.config.autoload_paths and install our integration into a separate directory, however, I've been unsuccessful so far since autoload_paths is frozen at a certain point during initialization. GitLab development docs mention this limitation and suggest using their config/initializers_before_autoloader directory instead. However, I was unsuccessful is going that way as the array was still frozen.

One other option might be to implement this as a Rails plugin/engine and (somehow) append the Gemfile to get that plugin loaded earlier.

Anyway, the integration class implementation seems to be the best way forward so far, and it could open the door for us to add even more external integrations. Integrations are not limited to issue trackers and can listen/respond to a number of different events in the system.

After removing Phabricator code, metrics, and documentation I'm not sure how open upstream GitLab would be for including such an implementation though.

I think that was generally done in response to https://admin.phacility.com/phame/post/view/11/phacility_is_winding_down_operations/ based on what I see in tasks like https://gitlab.com/groups/gitlab-org/-/epics/1197. Adding a Phorge integration might be seen differently upstream.

Maybe it's possible to add a new GitLab flavored markdown (GLFM) extension which understands Phabricator task ids:

https://docs.gitlab.com/ee/development/gitlab_flavored_markdown/specification_guide/index.html

It seems GitLab uses official supported flavored markdown extensions and some internal ones. The internal ones are stored in /opt/gitlab/embedded/service/gitlab-rails/scripts/glfm?

Maybe it's possible to add a new GitLab flavored markdown (GLFM) extension which understands Phabricator task ids:

https://docs.gitlab.com/ee/development/gitlab_flavored_markdown/specification_guide/index.html

It seems GitLab uses official supported flavored markdown extensions and some internal ones. The internal ones are stored in /opt/gitlab/embedded/service/gitlab-rails/scripts/glfm?

That might work for the links. It does, however, seem like we could benefit from a formal integration here—provide links for each project to create new issues with the right project tags, e.g. and respond to other events supported by integrations to update things in Phab/Phorge.

I'll check out the GLFM approach and also keep hacking on my local GitLab dev environment to see if I can hijack the Gemfile/Gemfile.lock via bundler config and load additional engines. :)

I've created a proof of concept for an approach to using our own Gemfile and locally installed railties/engines.

https://gitlab.wikimedia.org/dduvall/gitlab-wmf

From the "How it works" section of the README:

See the included Docker Compose configuration and corresponding Dockerfile for details. In conjunction, they:

  • Installs gems from our local [gems directory](./gems/) into the environment.
  • Install a Gemfile.wmf into the gitlab-rails service's working directory that:
    1. Loads upstream's Gemfile
    2. Defines an entry for each of our gems to be loaded as part of the default bundler group (Rails loads the default group in every environment).
  • Copies upstream's Gemfile.lock to Gemfile.wmf.lock
  • Executes bundle lock against Gemfile.wmf to update Gemfile.wmf.lock with entries for our gems. This is necessary because gems have been frozen in upstream's bundler config.
  • Adds the following configuration to /etc/gitlab/gitlab.rb to ensure our Gemfile.wmf file is used when Rails loads gems.
gitlab_rails['env'] = {
  "BUNDLE_GEMFILE" => "/opt/gitlab/embedded/service/gitlab-rails/Gemfile.wmf",
}

This probably warrants discussion before attempting to productionize. It's not modifying any of the upstream source files, so that's great, meaning we could package up the solution as a deb or puppetize it without too much hassle. It does, however, touch a very core part of Rails initialization. The loading via Bundler part of Rails has been the same since at least Rails 3.0, so I don't see it changing that much, but upstream could potentially filter out the overwriting of BUNDLE_GEMFILE in its config.

It's worth noting that there's a comment in the upstream issue—where I got the gitlab.rb part of this implementation—that suggests they would consider a contribution around loading custom Gemfiles, so if this does work out for us, we should consider a contribution. Submitting one and shepherding it through review would take a large time investment, as we've seen, but having a stable way to accomplish extensibility would be worth it.

I've submitted gitlab-org/omnibus-gitlab#7101 upstream that formalizes this approach in the gitlab.rb config, allowing for any number of locally managed directories containing additional gems to load during initialization.

gitlab_rails['gems'] = {
  '/a/local/dir/of/vendored/gems' => ['some_gem', 'another_gem'],
}

I've no idea how this will be received by upstream or when it will even get attention, so let's keep moving ahead with our own rollout as well.

Change 951580 had a related patch set uploaded (by Dduvall; author: Dduvall):

[operations/puppet@production] gitlab: Support loading of local gems

https://gerrit.wikimedia.org/r/951580

Change 951580 merged by Jelto:

[operations/puppet@production] gitlab: Support loading of local gems

https://gerrit.wikimedia.org/r/951580

Change 953968 had a related patch set uploaded (by Jelto; author: Jelto):

[operations/puppet@production] gitlab: enable local_gems in devtools test instance

https://gerrit.wikimedia.org/r/953968

Change 953968 merged by Jelto:

[operations/puppet@production] gitlab: enable local_gems in devtools test instance

https://gerrit.wikimedia.org/r/953968

Usage of local gems is enabled on the test instance now. Two additional files were created /opt/gitlab/embedded/service/gitlab-rails/Gemfile.local and /opt/gitlab/embedded/service/gitlab-rails/Gemfile.local.lock.

On the other hosts that change was noop.

Change 957803 had a related patch set uploaded (by Dduvall; author: Dduvall):

[operations/puppet@production] gitlab: Fix Gemfile.local permissions and use absolute path in gitlab.rb

https://gerrit.wikimedia.org/r/957803

I've installed the gitlab-phorge gem on devtools by manually cloning the repo to /srv/gitlab-wmf-gems/gitlab-phorge and specifying the following project level hiera:

profile::gitlab::local_gems:
  /srv/gitlab-wmf-gems:
  - gitlab_phorge
profile::gitlab::local_gems_enabled: true

The Phorge integration appears under project Settings -> Integrations and the rendering of T* links seems to work as expected.

Screen Shot 2023-09-15 at 12.09.10 PM.png (255×977 px, 40 KB)

The final step will be to Puppetize the gem installation. @Jelto how should we accomplish this in Puppet? We could clone the gitlab-phorge repo like I did manually, but that would introduce a chicken-egg scenario for provisioning production from scratch. Given the nature of code hosting as a central dependency and the unlikelihood of ever having to provision production from scratch, maybe that's acceptable? What do you think?

Change 957803 merged by Jelto:

[operations/puppet@production] gitlab: Fix Gemfile.local permissions and use absolute path in gitlab.rb

https://gerrit.wikimedia.org/r/957803

Change 961917 had a related patch set uploaded (by Jelto; author: Jelto):

[operations/puppet@production] Revert "gitlab: enable local_gems in devtools test instance"

https://gerrit.wikimedia.org/r/961917

Change 961917 merged by Jelto:

[operations/puppet@production] Revert "gitlab: enable local_gems in devtools test instance"

https://gerrit.wikimedia.org/r/961917

Usage of a local gemfile causes problems when doing a GitLab version upgrade. This happened on the GitLab test instance. The error log is P52738.

We disabled usage of a local gemfile for the test instance for now to unblock a mandatory GitLab version upgrade (T347629).

I guess we just have to re-create the gemfile (delete the old one and let puppet create a new one). It would be nice if the puppet code can handle this situation and we don't have to manually delete and re-create the gemfile.

I'd like to see proper links to Phabricator in Gitlab, this would be a nice usability feature. Is the usage of a custom Gem still the preferred implementation for this? Currently the usage of local gems is disabled since Sep 2023 due to issues on upgrades. If we want to proceed with the custom gems we would need a solution to make sure the custom gem file survives a version upgrade or gets disabled and re-created after the upgrade.

Other options could be a custom GitLab bot which goes over MRs and replaces Bug: T123456 with [Bug: T123456](https://phabricator.wikimedia.org/T123456).

I'd like to see proper links to Phabricator in Gitlab, this would be a nice usability feature. Is the usage of a custom Gem still the preferred implementation for this? Currently the usage of local gems is disabled since Sep 2023 due to issues on upgrades. If we want to proceed with the custom gems we would need a solution to make sure the custom gem file survives a version upgrade or gets disabled and re-created after the upgrade.

I haven't revisited the local gem hack in quite a while. I was a bit preoccupied with Zuul/GitLab experimentation. However, the latter is now on pause, so I'd like to circle back to getting local gems working again.

Also, I've submitted local gem support upstream as PR 7101. It wasn't accepted outright, but it seems to have spurred some discussion about the idea more broadly among GitLab product teams, so that's a start.

It would be ideal of course to have this ability as a formal upstream feature. That said, I think we can support it ourselves with some additional Puppet wrangling to gracefully handle upgrades.

Other options could be a custom GitLab bot which goes over MRs and replaces Bug: T123456 with [Bug: T123456](https://phabricator.wikimedia.org/T123456).

I think this could work for MR descriptions and comments. I'm not sure if it could work for commit messages or other places where task numbers are rendered from immutable sources. It would also incur latency and some extra noise, but perhaps those are acceptable tradeoffs.

Other options could be a custom GitLab bot which goes over MRs and replaces Bug: T123456 with [Bug: T123456](https://phabricator.wikimedia.org/T123456).

I think this could work for MR descriptions and comments. I'm not sure if it could work for commit messages or other places where task numbers are rendered from immutable sources. It would also incur latency and some extra noise, but perhaps those are acceptable tradeoffs.

Thinking about other implementation possibilities here... do we have any way to inject custom javascript into the GitLab UI? If so the use case of turning mentions of T\d+ into links to Phab seems like something that could be done with a script that is the equivalent of a MediaWiki Gadget. We could also go in an interesting direction of creating a Greasemonkey script or even a Wikimedia GitLab browser extension that did this kind of client side enhancement.

Thinking about other implementation possibilities here... do we have any way to inject custom javascript into the GitLab UI? If so the use case of turning mentions of T\d+ into links to Phab seems like something that could be done with a script that is the equivalent of a MediaWiki Gadget. We could also go in an interesting direction of creating a Greasemonkey script or even a Wikimedia GitLab browser extension that did this kind of client side enhancement.

I'm not aware of any formal/sanctioned way of doing this. You can add additional paths to the Rails asset pipeline, but that's another solution that would require loading a custom gem and if we're going to do that, IMO the most maintainable solution here is still a custom integration class.

It might also be worth submitting a simple issue and/or MR upstream that would allow users to specify the delimiter for issue numbers in custom bug tracker integrations. Right now, it's limited to # or a project slug prefix.

We could also start using #<number> notation instead of T<number> notation. That would work with the builtin GitLab custom bug tracker integration.

We could also start using #<number> notation instead of T<number> notation. That would work with the builtin GitLab custom bug tracker integration.

That sounds like a good idea to me.

We could also start using #<number> notation instead of T<number> notation. That would work with the builtin GitLab custom bug tracker integration.

Some relevant prior discussion about this starting here: T330792#8658059

There was concern about using this in bug trailers because of a prior pattern for Bugzilla. Since this is about merge request descriptions, comments, etc., it might be fine. CodeReviewBot would probably need updated and I don't know if there's a good way to search for both patterns without doing two searches.

(I don't have strong feelings about it at this point, do whatever seems good, I just remembered we landed where we are for reasons.)

I went ahead and implemented/submitted a Phorge integration upstream as well as a feature request to allow the custom issue tracker ID delimiter to be configured. Let's see how they are received. If they won't take either, I will revisit the local gem setup.

I went ahead and implemented/submitted a Phorge integration upstream

This seems to have traction and is currently in a 3rd round of review.

brennen added a subscriber: dduvall.

This seems to have traction and is currently in a 3rd round of review.

I applied the documentation suggestions from the last (hopefully final?) round of review. I also accidentally merged in upstream changes, which I think cleared a prior approval, but in theory it seems like it could merge now. Temporarily reassigning to me since @dduvall is out for a while.

brennen changed the task status from Open to In Progress.Mar 28 2024, 10:36 PM
brennen changed the task status from In Progress to Stalled.Thu, Apr 4, 5:35 PM
brennen moved this task from Needs/Waiting Review to Radar on the User-brennen board.

The upstream integration has merged, so hopefully that'll be available in a release before long. Marking this as stalled until we can use it.

Also noting after some slack discussion with @bd808 that !7101: Support loading of locally installed gems remains open, but seems probably stalled out.

I can make some noise about that if it still seems useful, although the extent to which I am Not A Rails Person will probably be obvious if it gets into the nitty-gritty details.

Also noting after some slack discussion with @bd808 that !7101: Support loading of locally installed gems remains open, but seems probably stalled out.

Tyler poked at that thread. Based on the response, I think we can pretty well write that one off, although it's always possible that this:

An alternate route that we think might work is to enable an "add-on" mechanism which would let users build self-contained packages that can interoperate with GItLab on a low-level API. @dorrino is currently OOO, but when he comes back I'm going to get the ball rolling on the feature request process, and we'll link this MR to it as part of the motivation for it.

...may eventually exist and be useful.