Skip to content
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

Merged

Conversation

muffato
Copy link
Contributor

@muffato muffato commented Sep 4, 2022

(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:

  1. The shpc code was sometimes using https:// to test the remote URLs, sometimes http, and I've kept the same dichotomy, i.e. ssh:// and ssh. Any particular reason why ? I'd prefer only seeing https:// (and ssh://)
  2. My change is essentially piggy-backing on the existing Provider class, which can only be instantiated as GitHub or GitLab. 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 even ftp:// ! 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 !

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"
@muffato
Copy link
Contributor Author

muffato commented Sep 4, 2022

(I'm aware that black doesn't like the long lines. I'll fix these later)

@vsoch
Copy link
Member

vsoch commented Sep 4, 2022

The shpc code was sometimes using https:// to test the remote URLs, sometimes http, and I've kept the same dichotomy, i.e. ssh:// and ssh. Any particular reason why ? I'd prefer only seeing https:// (and ssh://)

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.

My change is essentially piggy-backing on the existing Provider class, which can only be instantiated as GitHub or GitLab. Since there is now a fallback for when Pages are not available, remote registries can be anything else that these two forges.

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!

Furthermore, the transport protocol can be anything that git supports, incl. git:// and even ftp:// ! Remote registries could perhaps be recognised as the ones that contain :// (and are not valid local paths) ?

Indeed the transport protocol could be any of those, and perhaps we should be flexible to that.

shpc/main/registry/provider.py Outdated Show resolved Hide resolved
shpc/main/registry/remote.py Outdated Show resolved Hide resolved
@muffato
Copy link
Contributor Author

muffato commented Sep 6, 2022

New version coming in with a lot of changes, incl. some I rescued from #579 !

  • Currently I allow any remote as long as it contains ://. That could easily be changed to the regex as you were suggesting because ...
  • ... the numerous tests for http, https, os.path.exists, etc, have been consolidated into just two locations: Filesystem for local paths, and VersionControl for remote paths.
  • I added config options in VersionControl to define the URLs of library.json and container.yaml based on the domain name (i.e. github.com and gitlab.com)
  • I tidied up the constructors and accessors of Filesystem and VersionControl. In particular, VersionControl has url for the URL, and source for the clone on disk, which allows moving iter_modules to the parent class

Copy link
Member

@vsoch vsoch left a 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!

shpc/main/registry/remote.py Outdated Show resolved Hide resolved
shpc/main/registry/remote.py Outdated Show resolved Hide resolved
shpc/main/registry/remote.py Show resolved Hide resolved
shpc/main/registry/remote.py Show resolved Hide resolved
shpc/main/registry/remote.py Show resolved Hide resolved
shpc/main/registry/remote.py Show resolved Hide resolved
shpc/main/registry/remote.py Outdated Show resolved Hide resolved
shpc/main/settings.py Show resolved Hide resolved
Copy link
Member

@vsoch vsoch left a 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 !)
shpc/main/registry/remote.py Outdated Show resolved Hide resolved
@muffato
Copy link
Contributor Author

muffato commented Nov 7, 2022

I've moved the cleanup call a level up into _sync

Copy link
Member

@vsoch vsoch left a 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!

shpc/main/registry/__init__.py Show resolved Hide resolved
shpc/main/registry/remote.py Outdated Show resolved Hide resolved
shpc/main/registry/remote.py Outdated Show resolved Hide resolved
shpc/main/registry/remote.py Show resolved Hide resolved
shpc/main/registry/remote.py Show resolved Hide resolved
@vsoch
Copy link
Member

vsoch commented Nov 7, 2022

@muffato this was my contribution (and mistake) - I think self._cache is a FIlesystem so instead of:

  if self._clone and os.path.exists(self._clone)

We need to either have an exists function on that (e.g., self._clone.exists()) or reference the path attribute for it directly.

@muffato
Copy link
Contributor Author

muffato commented Nov 7, 2022

My bad as well. I should have noticed before committing the change

@muffato
Copy link
Contributor Author

muffato commented Nov 7, 2022

Yeah, the tests pass 😎 !

Copy link
Member

@vsoch vsoch left a 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!

shpc/main/registry/__init__.py Outdated Show resolved Hide resolved
Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>
@muffato
Copy link
Contributor Author

muffato commented Nov 8, 2022

One quick question - where does the ssh come in (have you tested and it works for you?)

It still works. I've tried shpc sync-registry, shpc show, shpc install, and shpc docgen

The Registry has to be given as ssh://git@github.com/singularityhub/shpc-registry.git which is not one what you get by default from the GitHub UI. It needs the extra leading ssh:// and a / instead of : after the hostname.

There isn't any obvious code to support ssh because 1) it's still a URL, though on a different scheme, so urllib works as usual, and 2) the URL is passed on to git clone which supports all.
Note that even with ssh, shpc still tries to access a library.json, which in this case works. But when the library.json is not accessible, then it falls back to the clone and a local traversal.

I need to add some tests to make it obvious ssh is supported.

@muffato
Copy link
Contributor Author

muffato commented Nov 8, 2022

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 main has to be handled.

@vsoch
Copy link
Member

vsoch commented Nov 8, 2022

oh boop, I totally forgot this was a nested PR! Thanks for your patience with me @muffato !

@vsoch vsoch merged commit d059bf9 into singularityhub:add/private-remotes Nov 8, 2022
@muffato muffato deleted the add/private-remotes/sshh branch November 8, 2022 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants