Adds support for vanilla Gerrit #1954

felipecrs · 2022-04-07T16:06:29Z

Description

I work with Gerrit every day, and today I realized how much time it could save if Gitlens worked for me. To my surprise, I found Gerrit in the list of supported providers, but when trying to use it, I stumbled upon some issues.

The current Gerrit provider implementation is very tied to Google Source, but Google Source is an implementation on top of the vanilla Gerrit, and many other orgs implements Gerrit in the vanilla way.

This PR turns the existing Gerrit provider into a generic/vanilla Gerrit one (not tied to Google Source), but adds back Google Source as its own provider, meant to be used in Android, Chromium, Gerrit itself, Golang, and other projects from them.

Also, the new provider comes with a new pre-configured domain, which is the GerritHub, a publicly known and freely available Gerrit instance.

Where I work, however, we host our own Gerrit instance, and I would configure Gitlens as follows:

"gitlens.remotes": [{ "domain": "gerrit.my-org.com", "type": "Gerrit" }]

Closes #1953
Closes #1787
Closes #1788
Closes #1902

Checklist

I have followed the guidelines in the Contributing document
My changes follow the coding style of this project
My changes build without any errors or warnings
My changes have been formatted and linted
My changes include any required corresponding changes to the documentation
My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

felipecrs · 2022-04-07T16:21:51Z

I tested against both Google Source repositories and vanilla Gerrit repositories. Everything seems to be working as expected.

Also, I don't think this is going to be a practical breaking change, as I doubt anyone had ever added custom Gerrit providers to their settings (because if they did, it didn't work, as the implementation to-date is only valid for Google Source, which should be covered by the existing regex -- and therefore would not require one to set a custom provider for it).

felipecrs · 2022-04-07T16:27:53Z

This is ready to review.

/cc @andrewsavage1 as you have worked in the previous Gerrit provider.

andrewsavage1 · 2022-04-07T17:53:13Z

No issues with this from me, thanks for the fix @felipecrs.

felipecrs · 2022-04-07T23:45:05Z

@Levistator, @adambadura as you both reported similar issues, I would kindly ask if this would solve your issues.

And separates the existing Gerrit provider to a new GoogleSource one. Closes gitkraken#1953

felipecrs · 2022-04-07T23:52:54Z

If you would like to test, I built a vsix that you can install in your VS Code:

gitlens-12.0.5.vsix.zip

It's not a zip file, simply rename to remove the .zip suffix after downloading (GitHub only allows to upload certain extensions in comments).

README.md

patches/tr46.js

acarlson1029 · 2022-04-08T01:30:25Z

src/git/remotes/google-source.ts

+	}
+
+	private get reviewDomain(): string {
+		const [subdomain, secondLevelDomain, topLevelDomain] = this.domain.split('.');


FWIW this doesn't work for domains with more than three levels.

e.g. project.git.source.company-tld.com

Can this just split and mutate the subdomain?

This is only for GoogleSource. Are you sure this is applicable for you?

Upon double-checking, I guess it's not strictly necessary.
The domain I was using also resolves with project.googlesource.com

I had to update my git config remote URL and restart VS Code, but it worked 👍

The domain I was using also resolves with project.googlesource.com

Not sure if I understood this part.

Anyway, after this PR, you would configure VS Code with:

"gitlens.remotes": [{ "domain": "project.git.source.company-tld.com", "type": "Gerrit" }]

@acarlson1029 If you have such an environment to test, I would advise you to test it before we merge. The .vsix is available here:

#1954 (comment)

adambadura · 2022-04-08T05:40:58Z

If you would like to test, I built a vsix that you can install in your VS Code:

Is there any document describing how to use your package? Especially in remote scenarios? I'm sorry, but I'm new to Visual Studio Code.

adambadura · 2022-04-08T06:13:36Z

If you would like to test, I built a vsix that you can install in your VS Code:

Is there any document describing how to use your package? Especially in remote scenarios? I'm sorry, but I'm new to Visual Studio Code.

Turns out on the "Extensions" tab, in the "..." menu there is "Install from VSIX..." and it works as expected, including my remote work scenario.

adambadura · 2022-04-08T06:23:31Z

@Levistator, @adambadura as you both reported similar issues, I would kindly ask if this would solve your issues.

The issue #1787 (on adding -review in domain) seems to indeed be closed.

However, my own issue #1902 is not closed. Now I have gerrit-wrsl1.int.net.nokia.com used as domain. This matches the remote of Git. However, links to changes etc. should (in our setup) be done for the main server (gerrit.ext.net.nokia.com) instead. Only fetching is done from the mirror server.

Or do I have to do some extra configuration in the plugin settings to have this covered?

Interestingly, this could perhaps be deduced (or wild-guessed) from Git configuration itself. My .gitconfig contains entries like:

[url "https://<USER>@gerrit-wrsl1.int.net.nokia.com:29418"]
        insteadOf = https://<USER>@gerrit.ext.net.nokia.com:29418
[url "https://<USER>@gerrit.ext.net.nokia.com:29418"]
        pushInsteadOf = https://<USER>@gerrit.ext.net.nokia.com:29418
        pushInsteadOf = https://<USER>@gerrit-wrsl1.int.net.nokia.com:29418

including also versions with ssh:// and without the <USER> (although I'm not sure if I indeed need all 4 variants...).

adambadura · 2022-04-08T11:16:34Z

For the record, my C:/Users/(...)/AppData/Roaming/Code/User/settings.json has the following:

    "gitlens.remotes": [
        {"domain": "gerrit.ext.net.nokia.com", "type": "Gerrit"},
        {"domain": "gerrit-wrsl1.int.net.nokia.com", "type": "Gerrit"}
    ],

felipecrs · 2022-04-08T12:29:09Z

@adambadura Would you mind running:

git remote -v

From within your cloned project?

felipecrs · 2022-04-08T12:35:32Z

Where I work, we have a comparable situation. But there is one thing weird in your case, the protocol is set as HTTPS, but the port 29148 (which is Gerrit's known SSH port).

If my suspicious is correct, you are using SSH in the git remote. Internally, we also need to fetch from mirror and push to master, but only when using SSH. For HTTPS, the load balancing from Gerrit side happens transparently.

I have switched from SSH to HTTPS since a long time ago and I never looked back. With GCM, I don't need to re-type my password either.

Perhaps it can work for you too, although I don't deny we can improve the extension to support such a mirror/master scenario.

adambadura · 2022-04-08T12:38:08Z

git remote -v

Sure:

origin  ssh://<USER>@gerrit-wrsl1.int.net.nokia.com:29418/MN/5G/NB/gnb (fetch)
origin  ssh://<USER>@gerrit.ext.net.nokia.com:29418/MN/5G/NB/gnb (push)

felipecrs · 2022-04-08T12:49:40Z

@adambadura, I tested here, locally, and it seems to be working as expected. There is one point, however:

{
  "gitlens.remotes": [
    {"domain": "gerrit.ext.net.nokia.com", "type": "Gerrit"},
  ],
}

Do NOT add the mirror to the list of Gitlens Remotes. Can you please check if that works for you?

Code_VbrM0yfowG.mp4

adambadura · 2022-04-08T12:54:14Z

Can you please check if that works for you?

I will get back to you on this on Monday.

Levistator · 2022-04-08T16:55:26Z

Yep it works for me. Thanks @felipecrs !!

adambadura · 2022-04-11T07:41:08Z

Can you please check if that works for you?

I will get back to you on this on Monday.

Indeed, using only the main server:

    "gitlens.remotes": [
        {"domain": "gerrit.ext.net.nokia.com", "type": "Gerrit"}
     ],

helped in that the proper server is reached (at least from the Change-Id link).

However, as it turned out, this is not enough in my case. The server is OK, but the web access is done through gerrit.ext.net.nokia.com/gerrit/ instead. So, the Change-Id link should be

https://gerrit.ext.net.nokia.com/gerrit/q/<hash>

while the plugin makes it

https://gerrit.ext.net.nokia.com/q/<hash>

I know this goes beyond what I described in #1902 and I would close #1902 with this one and open a more specific issue afterward. However, perhaps you do know already how to address this configuration?

I did try using "gerrit.ext.net.nokia.com/gerrit" for the "domain", but unsurprisingly it didn't work. Moreover, the entire Change-Id linking functionality ceased to work (no link made at all), so I expect this must be some configuration error.

felipecrs · 2022-04-11T12:31:50Z

@adambadura what would be the Gitiles homepage for your server then?

Is it any of the above?

As an example:

https://review.gerrithub.io/plugins/gitiles/

adambadura · 2022-04-11T16:18:16Z

@adambadura what would be the Gitiles homepage for your server then?

https://gerrit.ext.net.nokia.com/

https://gerrit.ext.net.nokia.com/plugins/gitiles

https://gerrit.ext.net.nokia.com/gerrit/plugins/gitiles

Is it any of the above?

It is the last one, although I had to add the trailing / as otherwise I was getting "Not Found" error.

felipecrs · 2022-04-11T16:29:41Z

Got it. So, your Gerrit is running under a reverse proxy.

That's a little odd, because the domain itself has Gerrit in its name, so I wonder what the need for such an extra identifier in the URI was. Anyway, this does not change the fact that you run through a reverse proxy, and ideally this should be supported.

Having that said, it's a different feature than what this PR is trying to solve. So, I would suggest you open a different issue, targeting something like Support Git remote providers running behind a reverse proxy. I haven't checked, but I suspect this isn't only valid for Gerrit.

I have some ideas on how that can be solved, but an issue would be helpful so that we could discuss with the maintainers the preferred approach.

adambadura · 2022-04-11T16:39:31Z

OK then, let's close #1902 along with this one.

felipecrs · 2022-04-11T16:40:52Z

OK then, let's close #1902 along with this one.

Right. I added #1902 back to the list of closable issues by this PR.

felipecrs · 2022-04-11T19:44:43Z

@eamodio just to say this is ready to review now.

eamodio · 2022-04-13T04:39:49Z

Wow, @felipecrs, this is awesome. Thank you so much for your contribution! And thank you to all the testers too!

felipecrs force-pushed the vanilla-gerrit branch from d911d45 to 4735b7e Compare April 7, 2022 16:07

felipecrs changed the title ~~Add support for vanilla Gerrit providers~~ Adds support for vanilla Gerrit Apr 7, 2022

felipecrs force-pushed the vanilla-gerrit branch 2 times, most recently from 7e9dcd2 to 2531553 Compare April 7, 2022 16:13

felipecrs marked this pull request as ready for review April 7, 2022 16:19

felipecrs marked this pull request as draft April 7, 2022 16:22

felipecrs force-pushed the vanilla-gerrit branch 3 times, most recently from 6ffa581 to dcc37f0 Compare April 7, 2022 16:27

felipecrs marked this pull request as ready for review April 7, 2022 16:27

felipecrs mentioned this pull request Apr 7, 2022

Closes #1787 - Remove '-review' from reviewDomain() #1788

Closed

7 tasks

felipecrs force-pushed the vanilla-gerrit branch from dcc37f0 to b6ba826 Compare April 7, 2022 23:40

This was referenced Apr 7, 2022

Support replacing mirror/replica domain with main domain for remote provider #1902

Closed

Remove '-review' from Gerrit Remote reviewDomain() #1787

Closed

Adds support for vanilla Gerrit

7350e8b

And separates the existing Gerrit provider to a new GoogleSource one. Closes gitkraken#1953

felipecrs force-pushed the vanilla-gerrit branch from b6ba826 to 7350e8b Compare April 7, 2022 23:50

felipecrs commented Apr 8, 2022

View reviewed changes

README.md Show resolved Hide resolved

patches/tr46.js Show resolved Hide resolved

acarlson1029 reviewed Apr 8, 2022

View reviewed changes

eamodio self-assigned this Apr 13, 2022

eamodio added this to the Soon™ milestone Apr 13, 2022

eamodio merged commit 517e9f0 into gitkraken:main Apr 13, 2022

adambadura mentioned this pull request Apr 13, 2022

Support Git remote providers running behind a reverse proxy #1964

Open

felipecrs deleted the vanilla-gerrit branch April 13, 2022 13:43

eamodio modified the milestones: Soon™, June 2022 May 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for vanilla Gerrit #1954

Adds support for vanilla Gerrit #1954

felipecrs commented Apr 7, 2022 •

edited

Loading

felipecrs commented Apr 7, 2022 •

edited

Loading

felipecrs commented Apr 7, 2022 •

edited

Loading

andrewsavage1 commented Apr 7, 2022

felipecrs commented Apr 7, 2022

felipecrs commented Apr 7, 2022

acarlson1029 Apr 8, 2022

felipecrs Apr 8, 2022

acarlson1029 Apr 8, 2022 •

edited

Loading

felipecrs Apr 8, 2022 •

edited

Loading

adambadura commented Apr 8, 2022

adambadura commented Apr 8, 2022

adambadura commented Apr 8, 2022

adambadura commented Apr 8, 2022

felipecrs commented Apr 8, 2022

felipecrs commented Apr 8, 2022 •

edited

Loading

adambadura commented Apr 8, 2022

felipecrs commented Apr 8, 2022 •

edited

Loading

adambadura commented Apr 8, 2022

Levistator commented Apr 8, 2022

adambadura commented Apr 11, 2022

felipecrs commented Apr 11, 2022 •

edited

Loading

adambadura commented Apr 11, 2022

felipecrs commented Apr 11, 2022 •

edited

Loading

adambadura commented Apr 11, 2022

felipecrs commented Apr 11, 2022

felipecrs commented Apr 11, 2022

eamodio commented Apr 13, 2022

Adds support for vanilla Gerrit #1954

Adds support for vanilla Gerrit #1954

Conversation

felipecrs commented Apr 7, 2022 • edited Loading

Description

Checklist

felipecrs commented Apr 7, 2022 • edited Loading

felipecrs commented Apr 7, 2022 • edited Loading

andrewsavage1 commented Apr 7, 2022

felipecrs commented Apr 7, 2022

felipecrs commented Apr 7, 2022

acarlson1029 Apr 8, 2022

Choose a reason for hiding this comment

felipecrs Apr 8, 2022

Choose a reason for hiding this comment

acarlson1029 Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

felipecrs Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

adambadura commented Apr 8, 2022

adambadura commented Apr 8, 2022

adambadura commented Apr 8, 2022

adambadura commented Apr 8, 2022

felipecrs commented Apr 8, 2022

felipecrs commented Apr 8, 2022 • edited Loading

adambadura commented Apr 8, 2022

felipecrs commented Apr 8, 2022 • edited Loading

adambadura commented Apr 8, 2022

Levistator commented Apr 8, 2022

adambadura commented Apr 11, 2022

felipecrs commented Apr 11, 2022 • edited Loading

adambadura commented Apr 11, 2022

felipecrs commented Apr 11, 2022 • edited Loading

adambadura commented Apr 11, 2022

felipecrs commented Apr 11, 2022

felipecrs commented Apr 11, 2022

eamodio commented Apr 13, 2022

felipecrs commented Apr 7, 2022 •

edited

Loading

felipecrs commented Apr 7, 2022 •

edited

Loading

felipecrs commented Apr 7, 2022 •

edited

Loading

acarlson1029 Apr 8, 2022 •

edited

Loading

felipecrs Apr 8, 2022 •

edited

Loading

felipecrs commented Apr 8, 2022 •

edited

Loading

felipecrs commented Apr 8, 2022 •

edited

Loading

felipecrs commented Apr 11, 2022 •

edited

Loading

felipecrs commented Apr 11, 2022 •

edited

Loading