Skip to content

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Jul 8, 2022

Signed-off-by: vsoch vsoch@users.noreply.github.com

vsoch added 2 commits July 8, 2022 17:19
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@muffato
Copy link
Contributor

muffato commented Jul 10, 2022

Hi @vsoch . Speaking about settings, here is something that I found a a little bit weird. When writing the tests for #548, to change the wrapper_scripts:templates settings I couldn't set this key to <value>, I had to set wrapper_scripts to templates:<value>, cf https://github.com/singularityhub/singularity-hpc/blob/main/shpc/tests/test_wrappers.py#L45-L48

In this PR, it looks like I should have been able to put the whole key (joined by a colon) as the first parameter ?

@vsoch
Copy link
Member Author

vsoch commented Jul 10, 2022

@muffato I'm glad you brought this up! So my intuition would be to do set <name> <value> but it looks like the current design is intended to parse each extra arg of "set" separately, allowing you to do:

$ shpc confg set field1:value1 field2:value2

But perhaps we should limit it to setting one thing, and then always require a group of two?

$ shpc confg set field1 value1

So then your example would work like:

$ shpc config set wrapper_scripts:templates value

Would that be a better interaction?

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch
Copy link
Member Author

vsoch commented Jul 10, 2022

okay check out 105747d - it should work for both! Let me know if you want me to add more tests.

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@muffato
Copy link
Contributor

muffato commented Jul 11, 2022

I've done some tests. The particular behaviour I was talking about ( https://github.com/singularityhub/singularity-hpc/blob/main/shpc/tests/test_wrappers.py#L45-L48 ) was actually already fixed by 676660a. client.settings.set("wrapper_scripts:templates", "/path/to/templates") now works.

Besides changing the CLI, 105747d improves the way the changes are reported

before:

$ shpc config set wrapper_scripts:templates:/path/to/wrappers_templates
Updated wrapper_scripts to be templates:/path/to/wrappers_templates

after:

$ shpc config set wrapper_scripts:templates:/path/to/wrappers_templates
Updated wrapper_scripts:templates to be /path/to/wrappers_templates

Regarding the CLI itself, shpc set key:subkey value is more natural, but I didn't see it as a priority to be changed.
Could you change the help message too ? Currently it's still advising the old syntax

$ shpc config set --help
usage: shpc config [-h] [--central] [params ...]

update configuration settings. Use set or get to see or set information.

positional arguments:
  params      Set or get a config value, edit the config, add or remove a list variable, or create a user-specific config.
              shpc config set key:value
              shpc config get key
              shpc edit
              shpc config inituser
              shpc config add registry:/tmp/registry
              shpc config remove registry:/tmp/registry

optional arguments:
  -h, --help  show this help message and exit
  --central   make edits to the central config file.

@vsoch
Copy link
Member Author

vsoch commented Jul 11, 2022

yep on it!

…t docs

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch
Copy link
Member Author

vsoch commented Jul 11, 2022

All set! a335931

I also added tests (for both formats) for add/remove.

@vsoch vsoch merged commit 4f1cd88 into main Jul 11, 2022
@vsoch vsoch deleted the bugfix/config-set-get branch July 11, 2022 07:50
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