Introducing version_naming property that makes modulefile as version …#679
Introducing version_naming property that makes modulefile as version …#679vsoch merged 12 commits intosingularityhub:mainfrom
Conversation
|
Looks like there are issues with the updated runner - let me see what I can do in another PR. |
|
I think you probably want to try: client.settings.set("version_naming", True)
assert client.settings.get("version_naming") == True |
shpc/tests/test_version_naming.py
Outdated
|
|
||
| client.settings.set("version_naming", False) | ||
| assert client.settings.set("version_naming", False) is False | ||
| assert client.settings.set("version_naming") == False |
There was a problem hiding this comment.
You need to first set it, and then use get to check the setting is what you set it to.
|
Nicely done! You'll want to (as a final step) bump the version in |
shpc/main/modules/base.py
Outdated
| template = self.template.load(self.templatefile) | ||
| module_path = os.path.join(module.module_dir, self.modulefile) | ||
| if self.settings.version_naming: | ||
| self._modulefile = name.split(":")[1] |
There was a problem hiding this comment.
What is self._modulefile used for? I only see it in this small set of statements. It might make sense to just use name.split(":")[1] in the one place it is needed and not save a private class variable.
|
Hi, it is always great to see contributions coming from users! As per @muffato 's comment in #678 , I think it would be good to know whether Views can work in your user case or not, to try and not duplicate functionalities if not needed. Either way, discussing these aspects with users of the software has a great deal of added value, in making sure the software is useful to the community, both in terms of functionality and documentation on how to use them. My 2 cents :) |
|
I agree it's not good to duplicate functionality, but in that views adds complexity (and this change makes it possible to generate the default with versions) I'm wondering if this isn't a good addition regardless. |
shpc/settings.yml
Outdated
| podman_shell: /bin/sh | ||
| docker_shell: /bin/sh | ||
|
|
||
| #Experimental version naming tag so that modulefile is changed to version number . Default value is false. Issue number 26 |
There was a problem hiding this comment.
I suggest having this right after default_version option, line 39, as they are related
|
Good point @vsoch :) I will not have the chance to test this PR. One comment I have is: is the PR been designed and tested while taking into account the implementation and options of the |
The version in shpc/version.py is You want me to bump it to "0.1.30"? |
shpc/main/modules/base.py
Outdated
| template = self.template.load(self.templatefile) | ||
| module_path = os.path.join(module.module_dir, self.modulefile) | ||
| if self.settings.version_naming: | ||
| module._module_dir = os.path.dirname(module.module_dir) |
There was a problem hiding this comment.
Can you please walk me through why we need a private class variable here?
There was a problem hiding this comment.
Sorry, forgot to clean it up, its not needed and have been fixed in new commit
|
@Amjadhpc you'll need to go up to 0.1.31 now - we had another PR merge in this morning. |
|
Looks like it has reached docker pull rate limit WARNING: Couldn't use cached digest for registry: GET https://index.docker.io/v2/library/python/manifests/sha256:7dd8962ad2a63403428d652a64d814a5002f1386379355edf5970e40557fe4e6: TOOMANYREQUESTS: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit |
|
@Amjadhpc it's an ephemeral error, OK to run again. I'm not going to put credentials in there just for these rare cases. |
|
Thanks for your help and guidance @vsoch |
4 participants
This is an attempt to fix issue number 26 raised by me.
I have not tested all settings