View install commands #596

muffato · 2022-10-10T01:57:58Z

Hi.

Here is a pull-request to address #590 (comment) and #590 (comment)

shpc install doesn't accept a --view argument anymore.
shpc view install now honours the default view, as set in the settings.
I've renamed the view and disable_view arguments of the install method to resp. extra_view and disable_default_view to clarify what they refer to.
I've updated the CI to test views on tcsh too, so that both shells test (almost¹) entirely the same thing.
Both CI tests clean up their modules / views to ensure nothing bleeds from the first one to the next

tcsh still doesn't run shpc test. I've tried to add it, but couldn't manage to make it work: it complains that module is not found even though the module commands work. I found something weird: shpc test always runs the test script with bash even if the user requested a different test shell. I tried using the test shell throughout but it didn't help. And anyway, tcsh is not allowed as a shell. ↩

as per singularityhub#590 (comment)

…quested views Renamed the variables to make it clear what is a view and what is a view name.

vsoch

Thanks @muffato ! Some comments below!

shpc/main/modules/base.py

shpc/tests/test_views.py

muffato · 2022-10-10T03:10:41Z

The latest version makes most of the comments obsolete

shpc/main/modules/base.py

vsoch · 2022-10-10T06:58:32Z

okay I started a rough idea - too tired to finish tonight but will try to soon this week!

vsoch · 2022-10-10T17:29:12Z

muffato#4 (comment)

This will help to carry around some common data attributes and functions, and reduce redundancy within the ModuleBase class. It also allows for providing fewer variables to the rendered templates since they can come from the module. Signed-off-by: vsoch <[email protected]>

Signed-off-by: vsoch <[email protected]>

muffato · 2022-10-11T01:42:37Z

shpc/client/view.py

@@ -52,7 +52,10 @@ def create_from_file(

    # Extra modules to install
    for install_module in install_modules:
-        cli.install(install_module, view=view_name, disable_view=False, force=force)
+
+        # TODO: can we cut out early if already installed?


OK, I'll keep that in mind when implementing reinstall and upgrade. I could split force into specific flags (like ignore-missing and uninstall-missing in upgrade)

…nstall

muffato · 2022-10-11T02:04:01Z

Here it is, I've split view_uninstall from uninstall like I did for install

muffato

I've annotated the occurrences of force. I'll probably tidy some of them up in #590

shpc/main/modules/base.py

vsoch

This is a heck of a lot neater - I really like it! Some small tweaks, and then given you've taken the basic changes for a spin and are happy, I'm good to merge this into main and keep working. I think your other PR #590 is going to add some really good functionality, and we can finish up there and ping Marco to see what he thinks before doing a release.

And I'm also down to keep iterating on improving the new Module class to better handle some of the logic - let me know if you see / think of something that can be improved and I'll be on it!

shpc/main/modules/base.py

shpc/main/modules/template.py

shpc/main/modules/base.py

Co-authored-by: Vanessasaurus <[email protected]>

config.name was not wrong Co-authored-by: Vanessasaurus <[email protected]>

muffato · 2022-10-11T21:36:25Z

Looks good to me ! I'll rebase #590 once this is merged.

vsoch

OK this is a lot of changes - but I'm good as well! Let's merge this! 🥳

muffato added 4 commits October 10, 2022 02:26

Removed the --view option of shpc install

df5589e

as per singularityhub#590 (comment)

bugfix: always install in the default view, alongside additionally re…

5ba70a2

…quested views Renamed the variables to make it clear what is a view and what is a view name.

Print what bash is running

7e93b13

Test views on tcsh too

7677670

vsoch reviewed Oct 10, 2022

View reviewed changes

Moved the code to install a module into a view to a separate function

05697df

vsoch reviewed Oct 10, 2022

View reviewed changes

shpc/main/modules/base.py Show resolved Hide resolved

vsoch reviewed Oct 10, 2022

View reviewed changes

shpc/main/modules/base.py Outdated Show resolved Hide resolved

vsoch added 2 commits October 11, 2022 02:36

ensure we maintain parsed name

2a1331a

Signed-off-by: vsoch <[email protected]>

muffato commented Oct 11, 2022

View reviewed changes

Moved the view uninstallation to a separate function to mirror view_i…

b3121e9

…nstall

muffato force-pushed the view_install branch from 2fea99c to b3121e9 Compare October 11, 2022 02:02

muffato commented Oct 11, 2022

View reviewed changes

shpc/main/modules/base.py Show resolved Hide resolved

shpc/main/modules/base.py Show resolved Hide resolved

shpc/main/modules/base.py Show resolved Hide resolved

shpc/main/modules/base.py Show resolved Hide resolved

vsoch reviewed Oct 11, 2022

View reviewed changes

shpc/main/modules/base.py Outdated Show resolved Hide resolved

shpc/main/modules/base.py Show resolved Hide resolved

shpc/main/modules/template.py Outdated Show resolved Hide resolved

shpc/main/modules/base.py Show resolved Hide resolved

muffato and others added 3 commits October 11, 2022 22:22

No need to make this local variable

b89b823

Co-authored-by: Vanessasaurus <[email protected]>

Use parsed_name for consistency

eec370c

config.name was not wrong Co-authored-by: Vanessasaurus <[email protected]>

Document the "force" flags

7e76b6b

vsoch approved these changes Oct 12, 2022

View reviewed changes

vsoch merged commit ceca803 into singularityhub:main Oct 12, 2022

muffato deleted the view_install branch October 12, 2022 09:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

View install commands #596

View install commands #596

muffato commented Oct 10, 2022

vsoch left a comment

muffato commented Oct 10, 2022

vsoch commented Oct 10, 2022

vsoch commented Oct 10, 2022

muffato Oct 11, 2022

muffato commented Oct 11, 2022 •

edited

Loading

muffato left a comment

vsoch left a comment

muffato commented Oct 11, 2022

vsoch left a comment

View install commands #596

View install commands #596

Conversation

muffato commented Oct 10, 2022

Footnotes

vsoch left a comment

Choose a reason for hiding this comment

muffato commented Oct 10, 2022

vsoch commented Oct 10, 2022

vsoch commented Oct 10, 2022

muffato Oct 11, 2022

Choose a reason for hiding this comment

muffato commented Oct 11, 2022 • edited Loading

muffato left a comment

Choose a reason for hiding this comment

vsoch left a comment

Choose a reason for hiding this comment

muffato commented Oct 11, 2022

vsoch left a comment

Choose a reason for hiding this comment

muffato commented Oct 11, 2022 •

edited

Loading