start of work to add private remotes #585
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: vsoch <[email protected]>
|
@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! |
muffato
reviewed
Sep 4, 2022
shpc/main/registry/remote.py
Outdated
Comment on lines
181
to
182
| if not self._cache: | ||
| self._update_cache() |
There was a problem hiding this comment.
Why this check here as well ? _update_cache already checks this:
singularity-hpc/shpc/main/registry/remote.py
Lines 165 to 170 in e4c8650
There was a problem hiding this comment.
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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]