fix: add gitlab support #545

msagi · 2024-05-04T23:22:15Z

Fix #511 .

This is my first stab at adding multi host support to GitProxy. The main idea behind it is: not to lose the original git host during repointing the repo to GitProxy via git remote set-url command but keep the it in the repointed url as the first path segment, e.g.

$git remote -v
origin	git@github.com:finos/git-proxy.git (fetch)
origin	git@github.com:finos/git-proxy.git (push)

$git remote set-url origin https://localhost:8000/github.com/finos/git-proxy.git

$git remote -v
origin	http://localhost:8000/github.com/finos/git-proxy.git (fetch)
origin	http://localhost:8000/github.com/finos/git-proxy.git (push)

Since the original git host is preserved, specific routes can be configured (in proxy.config.json) connecting the first path segment to the original host, e.g.

  "proxyConfig": [
    {
      "path": "/github.com",
      "url": "https://github.com",
      "enabled": true
    },
    {
      "path": "/gitlab.com",
      "url": "https://gitlab.com",
      "enabled": true
    }
  ],

I've added GitHub and GitLab repos to the authorisedList and tested git push with both host, works fine.

GitHub.com is hardcoded in several places, and whilst the proxy now supports GitHub and GitLab too, perhaps we shall sit down and chat how we can evolve the UI component (e.g., GitHub account is hardcoded, etc.) forward.

Note: due to multi host support, the project/name is not unique ID any more. A repo can uniquely identified only by its URL, e.g. https://gitlab.com/finos/git-proxy.git

@abinash2512 @JamieSlome - let me know your thoughts

netlify · 2024-05-04T23:22:34Z

✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name	Link
🔨 Latest commit	`e56f894`
🔍 Latest deploy log	https://app.netlify.com/sites/endearing-brigadeiros-63f9d0/deploys/6645da30848d6a0009a54104

src/model/Repo.js

codecov · 2024-05-04T23:24:18Z

Codecov Report

Attention: Patch coverage is 56.91057% with 53 lines in your changes are missing coverage. Please review.

Project coverage is 58.31%. Comparing base (fe59198) to head (e56f894).

Files	Patch %	Lines
src/proxy/routes/index.js	31.14%	42 Missing ⚠️
.../processors/push-action/checkUserPushPermission.js	0.00%	5 Missing ⚠️
src/db/file/pushes.js	0.00%	2 Missing ⚠️
src/db/file/repo.js	80.00%	2 Missing ⚠️
src/proxy/processors/pre-processor/parseAction.js	89.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #545      +/-   ##
==========================================
+ Coverage   56.96%   58.31%   +1.35%     
==========================================
  Files          46       48       +2     
  Lines        1566     1605      +39     
==========================================
+ Hits          892      936      +44     
+ Misses        674      669       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JamieSlome · 2024-05-10T14:13:12Z

@msagi - since the wonderful @eddie-knight contributed automated releases, are you able to take a look at the latest status check failure? All PRs will not require a valid label prior to merge. This ensures that we can automate our semantic versioning based on the chosen and agreed titles of improvements, i.e. fix, feature, etc.

msagi · 2024-05-10T19:38:10Z

@msagi - since the wonderful @eddie-knight contributed automated releases, are you able to take a look at the latest status check failure? All PRs will not require a valid label prior to merge. This ensures that we can automate our semantic versioning based on the chosen and agreed titles of improvements, i.e. fix, feature, etc.

fixed @JamieSlome

msagi · 2024-05-15T19:39:37Z

hi @JamieSlome , would you like me to make any further changes?

JamieSlome · 2024-05-16T10:16:16Z

config.schema.json

+      "type": "object",
+      "properties": {
+        "path": { "type": "string"},
+        "url": { "type": "string"},


Do we have type url?

JamieSlome · 2024-05-16T10:18:05Z

proxy.config.json

+      "path": "/github.com",
+      "url": "https://github.com",
+      "enabled": true
+    },
+    {
+      "path": "/gitlab.com",
+      "url": "https://gitlab.com",
+      "enabled": true


I like your proposed configuration. Would it not be better to recommend /github and /gitlab? I realise that this is configurable so doesn't really matter and gives control and choice to the developer.

JamieSlome · 2024-05-16T10:20:26Z

website/docs/quickstart/intercept.mdx

@@ -58,7 +58,7 @@ By default the clone of your repository will communicate with GitHub. To change

 ```bash
 git remote -v
-git remote set-url origin http://localhost:8000/<YOUR-GITHUB-USERNAME>/git-proxy.git
+git remote set-url origin http://localhost:8000/github.com/<YOUR-GITHUB-USERNAME>/git-proxy.git


Suggested change

git remote set-url origin http://localhost:8000/github.com/<YOUR-GITHUB-USERNAME>/git-proxy.git

git remote set-url origin http://localhost:8000/github/<YOUR-GITHUB-USERNAME>/git-proxy.git

git remote set-url origin http://localhost:8000/gitlab/<YOUR-GITLAB-USERNAME>/git-proxy.git

JamieSlome · 2024-05-16T10:26:57Z

@msagi - getting some breaches in the UI (admin/push). We need to think about how this implementation will break existing databases already using previous models. How do we translate between the two and/or support both?

This is a fabulous PR and love the way you've implemented multiple endpoint support at the same point. This is very desirable. The updates to the UI should be fairly trivial as there only a few hyperlinks here and there that directly reference GitHub.com. Still need to do a more in-depth review of the PR but a great start for now!

JamieSlome · 2024-05-16T10:35:15Z

src/db/file/pushes.js

@@ -94,8 +94,8 @@ const cancel = async (id, logger) => {
 const canUserCancelPush = async (id, user) => {
  return new Promise(async (resolve) => {
    const pushDetail = await getPush(id);
-    const repoName = pushDetail.repoName.replace('.git', '');
-    const isAllowed = await repo.isUserPushAllowed(repoName, user);
+    const repoUrl = pushDetail.repo.url;


What about pre-existing repositories using the repoName property in the database? We need to think about how the update propagates to the previous data model and its impact on already stored properties in DB.

JamieSlome · 2024-05-16T10:37:27Z

src/db/file/repo.js

@@ -34,6 +34,22 @@ exports.getRepo = async (name) => {
  });
 };

+exports.getRepoByUrl = async (url) => {


Keep me honest, but can we not enforce uniqueness with organization, name and provider, i.e. zsh-org, zsh and gitlab.

Can we take advantage of the fact that both GitHub and GitLab follow the same naming convention for repositories and enforce uniqueness this way?

GitHub: finos, git-proxy, github
GitLab: zsh-org, zsh, gitlab

JamieSlome · 2024-05-16T10:37:57Z

src/db/file/repo.js

  return new Promise(async (resolve, reject) => {
-    const repo = await exports.getRepo(name);
+    const repo = await exports.getRepoByUrl(url);


Again, same point here - this is fine moving forward for updated data properties but what about previous stored data? Do we need a transformer?

JamieSlome · 2024-05-16T10:38:28Z

src/db/mongo/pushes.js

+    const repoUrl = pushDetail.repo.url;
+    const isAllowed = await repo.isUserPushAllowed(repoUrl, user);


Same point as the above comments here.

JamieSlome · 2024-05-16T10:40:21Z

src/db/mongo/repo.js

@@ -15,6 +15,11 @@ exports.getRepo = async (name) => {
  return collection.findOne({ name: { $eq: name } });
 };

+exports.getRepoByUrl = async (url) => {
+  const collection = await connect(cnName);
+  return collection.findOne({ url: { $eq: url}});


return collection.findOne({ org: ..., repo: ..., platform: ... })

Thoughts? We should probably step back a little and define a data model for repository together that will serve us well moving forward and isn't too opinionated. I am not sure about URL as only as this is quite un-atomic in its nature and includes content that we don't need in DB, i.e. https, :// - I feel like it can be broken down but happy to hear otherwise of course.

JamieSlome · 2024-05-16T10:40:59Z

src/model/Repo.js

+        const parsedUrl = url?.match(GIT_URL_REGEX);
+        if (parsedUrl) {
+            this.url = url;
+            this.protocol = parsedUrl[1];


Why is this required?

JamieSlome · 2024-05-16T10:41:38Z

src/proxy/actions/Action.js

+    this.project = repo.project;
+    this.repoName = repo.name;
+    this.url = repo.url;


msagi added 2 commits May 4, 2024 23:59

feat: add gitlab support

a0e4d64

Merge branch 'finos:main' into add-gitlab-support

360c7c7

github-advanced-security bot found potential problems May 4, 2024

View reviewed changes

src/model/Repo.js Fixed Show fixed Hide fixed

src/model/Repo.js Fixed Show fixed Hide fixed

src/model/Repo.js Fixed Show fixed Hide fixed

src/model/Repo.js Fixed Show fixed Hide fixed

src/model/Repo.js Fixed Show fixed Hide fixed

src/model/Repo.js Fixed Show fixed Hide fixed

msagi and others added 2 commits May 5, 2024 09:22

fix: regex escaping in repo model

c5e8659

Merge branch 'main' into add-gitlab-support

43f348d

Merge branch 'finos:main' into add-gitlab-support

b5cc493

msagi changed the title ~~Add gitlab support~~ fix: add gitlab support May 10, 2024

github-actions bot added the fix label May 10, 2024

Merge branch 'main' into add-gitlab-support

e56f894

JamieSlome self-requested a review May 16, 2024 10:15

JamieSlome reviewed May 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add gitlab support #545

fix: add gitlab support #545

msagi commented May 4, 2024

netlify bot commented May 4, 2024 •

edited

codecov bot commented May 4, 2024 •

edited

JamieSlome commented May 10, 2024

msagi commented May 10, 2024

msagi commented May 15, 2024

JamieSlome May 16, 2024

JamieSlome May 16, 2024

JamieSlome May 16, 2024

JamieSlome commented May 16, 2024 •

edited

JamieSlome May 16, 2024

JamieSlome May 16, 2024

JamieSlome May 16, 2024

JamieSlome May 16, 2024

JamieSlome May 16, 2024

JamieSlome May 16, 2024

JamieSlome May 16, 2024

-git remote set-url origin http://localhost:8000/github.com/<YOUR-GITHUB-USERNAME>/git-proxy.git
+git remote set-url origin http://localhost:8000/github/<YOUR-GITHUB-USERNAME>/git-proxy.git
+git remote set-url origin http://localhost:8000/gitlab/<YOUR-GITLAB-USERNAME>/git-proxy.git

		const repoUrl = pushDetail.repo.url;
		const isAllowed = await repo.isUserPushAllowed(repoUrl, user);

fix: add gitlab support #545

Are you sure you want to change the base?

fix: add gitlab support #545

Conversation

msagi commented May 4, 2024

netlify bot commented May 4, 2024 • edited

✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

codecov bot commented May 4, 2024 • edited

Codecov Report

JamieSlome commented May 10, 2024

msagi commented May 10, 2024

msagi commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieSlome commented May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented May 4, 2024 •

edited

codecov bot commented May 4, 2024 •

edited

JamieSlome commented May 16, 2024 •

edited