feat(key on repo url): support git hosts other than GitHub + multiple forks #1043
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1043 +/- ##
==========================================
+ Coverage 76.91% 80.21% +3.29%
==========================================
Files 55 62 +7
Lines 2270 2608 +338
Branches 255 308 +53
==========================================
+ Hits 1746 2092 +346
+ Misses 494 469 -25
- Partials 30 47 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -578,20 +578,24 @@ export default function Repositories(props) { | |||
| getGitHubRepository(); | |||
| }, [props.data.project, props.data.name]); | |||
|
|
|||
| // TODO add support for GitLab API: https://docs.gitlab.com/api/projects/#get-a-single-project | |||
There was a problem hiding this comment.
I've had some input from @StingRayZA @ Gitlab on this and will update the PR acordingly. after I've dealt with the test mentioned below.
|
Picked up a couple of test failures after merging main - will resolve (and start working on the additional tests needed). |
There was a problem hiding this comment.
I went through the approval/rejection flows with a pre-existing repo, and things work well!
There is an issue with backwards compatibility with older, invalid databases from previous versions of GitProxy (unique URL enforcement with repos). This may also cause issues with the other files (pushes, users).
I also tested the Add Repo flow which caused my server to crash, maybe because of something wrong on my end (invalid input maybe?).
src/service/routes/repo.js
Outdated
| const { proxy } = require('../index'); | ||
|
|
||
| // 2. Stop existing services | ||
| await proxy.stop(); |
There was a problem hiding this comment.
I got an error here when trying to add a new GitHub repository:
[0] API request to proxy repository https://github.com/jescalada/open-webui.git is for a new origin: true,
[0] existing origin list was: ["github.com"]
[0] creating new repo {"project":"jescalada","name":"open-webui","url":"https://github.com/jescalada/open-webui.git","maxUser":1,"users":{"canPush":[],"canAuthorise":[]}}
[0] Restarting the proxy to handle an additional origin
[0] Repository creation failed due to error: Cannot read properties of undefined (reading 'stop')
[0] node:_http_outgoing:655
[0] throw new ERR_HTTP_HEADERS_SENT('set');
[0] ^
[0]
[0] Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
[0] at ServerResponse.setHeader (node:_http_outgoing:655:11)
[0] at ServerResponse.header (/home/juan/Desktop/Juan/Projects/git-proxy/node_modules/express/lib/response.js:794:10)
[0] at ServerResponse.send (/home/juan/Desktop/Juan/Projects/git-proxy/node_modules/express/lib/response.js:174:12)
[0] at /home/juan/Desktop/Juan/Projects/git-proxy/src/service/routes/repo.js:168:13
[0] at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
[0] code: 'ERR_HTTP_HEADERS_SENT'
[0] }
[0]
[0] Node.js v20.19.2
[0] npm run server exited with code 1Interestingly, restarting the server does show the newly added repo in the dashboard:
There was a problem hiding this comment.
Thats got to be at least two errors in the proxy restart code - its made all the DB changes by that point and it just needs to restart the proxy if we have a new host to handle. In your example, it doesn't actually need to do that as the list of proxied hosts hasn't changed (you've just added another repo at a particular host). I'll need to take another look at that I guess!
There was a problem hiding this comment.
I've had a go at the relevant fixes, but can't test properly until after OSFF. If you end up giving it a go let me know @jescalada!
BTW I'm tempted to merge your proxy route test PR into this and update 9as the routes change slightly). It looks like you got a handle on testing the API routes with Chai http (although I haven't run them and did see your comment about issues). I was planning to try and test the getRouter() and its output, but your tests might be a better approach, as long as we add one that checks routes after adding a new repository host/origin and triggering a restart of the proxy.
Thoughts?
There was a problem hiding this comment.
That'd be super helpful if you don't mind the extra work with the tests! I was actually planning on covering any missing/extra code on a follow-up PR to get the overall coverage to 80%. Maybe we could go ahead and merge this as-is so you don't have to keep maintaining the PR?
This comment was marked as resolved.
This comment was marked as resolved.
| @@ -47,15 +46,6 @@ let _config = { ...defaultSettings, ...(_userSettings || {}) } as Configuration; | |||
| // Create config loader instance | |||
| const configLoader = new ConfigLoader(_config); | |||
|
|
|||
| // Get configured proxy URL | |||
| export const getProxyUrl = () => { | |||
| if (_userSettings !== null && _userSettings.proxyUrl) { | |||
There was a problem hiding this comment.
Instead of removing this function entirely, wouldn't it be useful to emit a warning indicating that this field is deprecated? It can be ignored from use but if it's present in _userSettings, we want to indicate that a config change is needed.
Lines 214 to 225 in d13686d
There was a problem hiding this comment.
Good point! As the function is no longer called, that would need to be done on load
I suppose we could suck up the warnings and copies for handling sslKeyPemPath and sslCertPemPath into the same spot if you thought it worth while.
I think catching and displaying a simple error message with the invalid entry/entries could be enough - so that the GitProxy administrator can quickly identify the issue and fix it manually. Thankfully, the error seems to occur on backend (db) startup, so end users wouldn't really have the app suddenly blowing up. |
Signed-off-by: Raimund Hook <rhook@gitlab.com>
…que as its our new primary key
Signed-off-by: Raimund Hook <rhook@gitlab.com>
…te non-specific db fns
Signed-off-by: Raimund Hook <rhook@gitlab.com>
Typescript wasn't working on the DB classes due to their dependency imports with require.
…making github fns optional
…file-based DB implementation
…file-based DB implementation
…tion and db pojos
As it causes parse errors where Cntent-Length is also set
kriswest
force-pushed
the
950-key-on-repo-urls
branch
from
802b61b to
92b801b
Compare
kriswest
force-pushed
the
950-key-on-repo-urls
branch
from
5502cdb to
e5cb9c9
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
kriswest
force-pushed
the
950-key-on-repo-urls
branch
from
c2aedf0 to
fe934df
Compare
kriswest
force-pushed
the
950-key-on-repo-urls
branch
from
8358124 to
e059206
Compare
|
I've implemented tests covering adding and removing an alternative origin (gitlab), although I need to select a smaller repo for the test (than Gitlab itself). In doing so I've finally dealt with the issue with restarting the proxy. @jescalada I had originally copied what you did in config loading - I don't think that approach is working. When you require the proxy and call I have some additional cleanup still to do and gitlab API calls to work into the UI, but otherwise I think this is working now. |
resolves #950
resolves #511
resolves #66
Refactor (api, proxy & UI) to remove the assumption of GitHub as the git repository host and the use of the repository
namefield as the id of the repository (as this prevents git-proxy instances from supporting multiple forks of a project or projects from multiple hosts with the same name).This PR:
namefield in the API with the _id field generated by the database adaptors,namesto be repeated (multiple forks or clashing names from different organisations/repository hosts)organisation/repoName.gitin the proxy URLs with the repository urlbecomes
https://myGitProxyInstance.com:8443/github.com/finos/git-proxy.git
To Do:
(contributed as part of a GitLab CoCreate collaboration with help from @StingRayZA)