-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(git-providers): support docker url and http connections for git providers #1718
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: canary
Are you sure you want to change the base?
feat(git-providers): support docker url and http connections for git providers #1718
Conversation
const giteaUrl = provider.giteaUrlAlt || provider.giteaUrl; | ||
const baseUrl = giteaUrl.replace(/\/+$/, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we really always apply this replace regardless of one property or the other, I think it would not be necessary to maintain another property, since we apply the same operation for both properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing. Not sure I understand the question. That regex simply removes any trailing slashes in case the user added them, which avoids a possible duplicate (the next line is const url =
${baseUrl}/api/v1/user/repos;` which hardcodes a slash. I didn't write that, it was already in the code. Are you suggesting to strip it off when the user first submits the form? That could make sense as long as we know it won't affect anything else.
As far as maintaining the new property (I think you mean the new database field giteaUrlAlt right?), yes I think its the only way to achieve this PR. In my case, I need two separate URLs:
- http://localhost:port - this is needed to bring up the authorization page in the browser or perform git commands that node spawns like git clone
- http://gitea:3000 - this is the new field/property that is needed for fetch requests if Dokploy is running from a container and can't reach the above url
Again, this is only needed in the case where Gitea is not exposed to the internet.
If you can think of a good way to achieve this some other way I'd be happy to try it. Otherwise if I'm making sense, I/we still need to test this with Dokploy running from a container. I'm not sure if the current NPM commands are up to date on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looked again and noticed that the trailing slash stripping isn't always done. Like:
https://github.com/Dokploy/dokploy/blob/canary/packages/server/src/utils/providers/gitea.ts#L57
Again, these were already there (I think in the diff because I changed the var names).
If you want to make it more consistent, I think either we should strip them before adding to the database or possibly remove it and leave it to the user to discover the error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but I mean in this operation you basically say if this property doesn't exist use the other one and apply the same replace operation, so really you will always apply the same operation for both properties, I don't see any other place where you really do a different operation than giteaUrl either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm still not understanding if you're only taking issue with the actual replace command that removes a possible trailing slash, or the bigger picture of adding and using the new field giteaUrlAlt.
If the former, again, I just kept what was already there. But it's trivial to change it so it matches how the urls are constructed in other places if you like. But I think it sounds like you're questioning more than just that.
I don't see any other place where you really do a different operation than giteaUrl either.
I'm not 100% sure what you're saying here, because there are differences. If the user defines a giteaUrlAlt in the advanced options, it will get used for certain operations, but not for others in packages/server/src/utils/providers/gitea.ts.
Imagine we have these two urls:
giteaUrl: http://localhost:3000
giteaUrlAlt: http://giteea:3000
For operations where node spawns a shell to run commands like git clone, the normal giteaUrl is always used because the command is being called from the host. giteaUrl would also be used for the initial oauth browser authorization (you can't launch a browser tab with a docker url).
But likewise, the localhost url won't work in a fetch request (assuming Dokploy is running inside a container, which it normally is in production). Thus the need for two different urls in this situation.
All of this said, if you don't want to add this or want to change it I'd understand. In the cloud version it's going to be redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, when you have a chance any update on this? I'm not sure why you're saying it's a problem to remove the trailing slash in both cases of user input (giteaUrl or giteaUrlAlt). That's as intended to to avoid duplicate slashes (the next line in the code hard codes a slash). But I can either close this if you don't want it, or we can also limit the scope to at least allow http in the giteaUrl... which would in my case at least allow me to keep using Gitea with Dokploy (although that's a hacky solution that requires manually changing the url after granting auth).
const giteaUrl = provider.giteaUrlAlt || provider.giteaUrl; | ||
const baseUrl = giteaUrl.replace(/\/+$/, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but I mean in this operation you basically say if this property doesn't exist use the other one and apply the same replace operation, so really you will always apply the same operation for both properties, I don't see any other place where you really do a different operation than giteaUrl either.
This addresses #1591 to allow Gitea and Gitlab (not yet implemented) providers to use local and/or http connections.
So far Gitea should be complete, although I could use some advice for how to build and use the docker container locally so I can test the new feature using a docker url. I tried running pnpm docker:build:canary and got errors, and the Dockerfile at the root seems to want to work with /usr/src/app.