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

start of work to add private remotes #585

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Aug 31, 2022

This is start of work to support a remote that doesn't have a library.json exposed, and will partially address #581.

Signed-off-by: vsoch [email protected]

@vsoch
Copy link
Member Author

vsoch commented Aug 31, 2022

@muffato here is a start so a "private" (meaning non library.json exposed) registry would work. I don't have your registry so I can't test, but if you want to give it a spin (and let me know what doesn't work, etc.) it would be appreciated!

Copy link
Contributor

@muffato muffato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works for me 🤝 !

I've also managed to make it work over ssh with very little modifications. Can I make a tiny PR onto this PR ?

Comment on lines 181 to 182
if not self._cache:
self._update_cache()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this check here as well ? _update_cache already checks this:

def _update_cache(self, force=False):
"""
Update local cache from a registry.
"""
if self._cache and not force:
return

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes absolutely! I responded in your PR - I really like your idea of making the protocol more general (perhaps a regular expression with a named group to match at the start, and then saving to the remote for later use) if you are up for that! We'd want to make sure that change still works with your ssh registry.

* Support accessing remote registries via ssh
* Factored out a function that tells whether a registry path is local
* Make sure the URL is used, not self.source which could be a local path

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"

* Make self.source include the subdir from the start. Allows implementing iter_modules in the base class
* The check already happens in _update_cache()
* Moved is_path_local to shpc.utils
* Added a safeguard to prevent cloning multiple times
* clone() is actually only supported by VersionControl
* No need to yield self.source in iter_modules since it's constant and accessible from outside (and not all callers want it !)
* It's more practical to yield the the registry object (provider) rather than using the "source" path (which is undefined for remote registries anyway)
* Optimised the "update all" mode by directly using Result objects from the registries. Otherwise, it wastes time finding modules that we know are there
* Clones only ever exist within a function
* Optimised iter_modules method for remote registries (using the cache)
* Moved back iter_modules to Filesystem since VersionControl has its own, optimised, version
* Stopped using self.source in VersionControl, to avoid confusion with Filesystem
* url, not source, is to be used for remote registries
* Cannot do these heuristics as we need to report unexisting local paths
* str.split can limit the number of splits
* The main Registry object, not the settings, should decide whether there is a local registry or not
* To avoid duplicating the code that assesses whether a path or local or not, check which sub-class of Provider is used
* The parent class shouldn't know that much about the subclasses
* Restored back the automatic addition of https://
* Restructured to avoid an unnecessary else
* shpc convention: no else when the then ends with a return
* Unnecessary due to operator precedence rule
* Added a cache in `library_url`
* Fixed the implementation of the cache in VersionControl.exists
* exists has its own implementation in VersionControl, so this implementation is in fact specific to Filesystem
* iter_registry is basically iter_modules with an extra filter
* Yield relative paths rather than full paths since *all* consumers need relative paths
* Proper method to cleanup a clone
* Removed a cleanup() call that was expected to do nothing
* Increased the symmetry to simplify the maintainability
* NotImplementedError is more useful than pass
* The tuplized version is not the preference here
* Easier to understand
* Made the clone return a Filesystem object independent from VersionControl
* Extra comment
* Back to a subclass of VersionControl for each forge
* Pre-parse the URL
* VersionControl should not be directly used
* Renamed the variable for clarity
* Removing yaml because it's the only file we have for a container
* Defensive programming: local could still be None
* bugfix: iter_modules needs to yield paths to container.yaml
* Moved the cleanup call up to _sync()
* bugfix: iter_modules now returns path to the container.yaml
* Need to check here too that the clone still exists
* Also need to reset self._clone if the directory is gone
* More checks on local and remote
* The temp directory may have been deleted in the meantime
* It makes more sense to cleanup tmplocal than self, and it works because self expects the cleanup may happen
* Moved this to the parent class
* Another implementation that doesn't make it too obvious the base-class knows about GitHub and GitLab
* Silly typo: self._clone is a Filesystem object, not a string
* No colon
* You shall use American spelling
* Added a test to showcase ssh

* Revert "bugfix: iter_modules needs to yield paths to container.yaml"

This reverts commit f069f48.

* Revert "bugfix: iter_modules now returns path to the container.yaml"

This reverts commit c5b4cb9.
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.

2 participants