-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Support accessing remote registries via ssh #589
Support accessing remote registries via ssh #589
Conversation
The URL has to be given as "ssh://[user@]host.xz[:port]/path/to/repo.git" rather than the shorter version "[user@]host.xz:path/to/repo.git"
(I'm aware that |
At first I was just checking for http to be able to hit http or https, and I agree it would make sense to require https, however then we'd have to do an updated check to also check for http, and update to https if provided (with a warning). The second check I believe I had originally set the https prefix and assumed it would be there, but with all the changes this might be wrong now.
Nice!! I really appreciate you taking initiative on adding ssh - it would have been my next TODO but there is a lot in the queue so I'm a bit slow!
Indeed the transport protocol could be any of those, and perhaps we should be flexible to that. |
…ng iter_modules in the base class
…GitLab Also supports all transport protocols Git understands.
b7abd36
to
7c5c1e9
Compare
New version coming in with a lot of changes, incl. some I rescued from #579 !
|
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.
And possibly would be good to add an example of each registry type (ssh, git, https, etc) to the docs - I forget these formats all the time!
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.
Loving these changes! 🥳 I made a few final comments and I think after those we should be ready for merge,
…accessible from outside (and not all callers want it !)
ec22dbe
to
417e737
Compare
…r than using the "source" path (which is undefined for remote registries anyway)
… the registries Otherwise, it wastes time finding modules that we know are there
…n, optimised, version
I've moved the cleanup call a level up into |
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.
ok we're close!
Co-authored-by: Vanessasaurus <[email protected]>
…se self expects the cleanup may happen
…s knows about GitHub and GitLab
@muffato this was my contribution (and mistake) - I think self._cache is a FIlesystem so instead of:
We need to either have an exists function on that (e.g., self._clone.exists()) or reference the path attribute for it directly. |
My bad as well. I should have noticed before committing the change |
Yeah, the tests pass 😎 ! |
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.
One quick question - where does the ssh come in (have you tested and it works for you?)
After that, just a note in a the CHANGELOG and version bump and we are good to go!
Co-authored-by: Vanessasaurus <[email protected]>
It still works. I've tried The Registry has to be given as There isn't any obvious code to support ssh because 1) it's still a URL, though on a different scheme, so I need to add some tests to make it obvious ssh is supported. |
I've added a SSH url to the tests. The changelog and version bump will have to happen in the parent PR: #585 , where the merge conflict with |
oh boop, I totally forgot this was a nested PR! Thanks for your patience with me @muffato ! |
(extension of #585 )
Note: the URL has to be given as
ssh://[user@]host.xz[:port]/path/to/repo.git
rather than the shorter version[user@]host.xz:path/to/repo.git
.Comments:
https://
to test the remote URLs, sometimeshttp
, and I've kept the same dichotomy, i.e.ssh://
andssh
. Any particular reason why ? I'd prefer only seeinghttps://
(andssh://
)Provider
class, which can only be instantiated asGitHub
orGitLab
. Since there is now a fallback for when Pages are not available, remote registries can be anything else that these two forges. Furthermore, the transport protocol can be anything that git supports, incl.git://
and evenftp://
! Remote registries could perhaps be recognised as the ones that contain://
(and are not valid local paths) ?Happy to do that refactoring if you like !