Add append/remove subcommands to `bin/omero config` #1905

sbesson · 2013-12-16T21:07:10Z

This PR addresses https://trac.openmicroscopy.org.uk/ome/ticket/11201

Changes included are:

addition of bin/omero config append and bin/omero config remove so that configuration using lists of strings can easily be manipulated, e.g.

bin/omero config append omero.web.apps webfigure
bin/omero config append omero.web.apps webtagging
…
bin/omero config remove omero.web.apps webfigure

addition of corresponding unit tests under test_prefs.py

/cc @dpwrussell, @will-moore, @manics

knabar · 2013-12-17T08:07:56Z

components/tools/OmeroPy/src/omero/plugins/prefs.py

+        append = parser.add(
+            sub, self.append, "Append value to a key in the current profile.")
+        remove = parser.add(
+            sub, self.remove, "Append value to a key in the current profile.")


Should say "remove" instead of "append"

sbesson · 2013-12-17T14:17:49Z

Last commits should fix the help/descriptions across all bin/omero config subcommands so that

bin/omero config -h and bin/omero config <command> -h both display the subcommand description
NAME, KEY, and value arguments have a proper description

mtbc · 2013-12-17T14:23:10Z

components/tools/OmeroPy/src/omero/plugins/prefs.py

-        load.set_defaults(func=self.load)
+        load = parser.add(
+            sub, self.load,
+            "Read into current profile from a file or standard in")


"standard input" perhaps, may as well expand both if at all

joshmoore · 2014-01-15T08:31:21Z

@knabar : do you want to take an initial look at this?

knabar · 2014-01-15T09:22:43Z

Types are handled inconsistently:

$ omero config set x [1]
$ omero config append x 2
$ omero config get x
[1, '2']
$ omero config append x "'3'"
[1, '2', "'3'"]

Should values passed to append and remove be interpreted as JSON, just like values passed to set and get?

sbesson · 2014-01-16T08:59:57Z

@knabar: agreed. There is a discrepancy between set/get and append/remove subcommands at the moment. My initial goal was to allow simple commands such as

bin/omero config append omero.web.apps omero.searcher

to be correctly interpreted. I will add commits to unify the behaviour with the existing config subcommands.

sbesson · 2014-01-23T11:45:25Z

@knabar: fixed in 91fafda

knabar · 2014-01-23T12:03:50Z

Values passed to append still seem to always be interpreted as strings, even when they are not quoted.

$ omero config set x 1
$ omero config get x
[1]
$ omero config append x 2
[1, "2"]     <-- expected [1, 2]
$ omero config append x '"3"'
[1, "2", "\"3\""]    <-- expected [1, 2, "3"]

sbesson · 2014-01-23T14:03:53Z

With the latest change, I now have

$ bin/omero config --source ~/config.xml set x '["1"]'
$ bin/omero config --source ~/config.xml get x
["1"]
$ bin/omero config --source ~/config.xml append x 2
$ bin/omero config --source ~/config.xml append x "3"
$ bin/omero config --source ~/config.xml append x '"4"'
$ bin/omero config --source ~/config.xml get x
["1", 2, 3, "4"]

knabar · 2014-01-23T14:22:18Z

Works as expected now.

$ omero config set x [1]
$ omero config append x [2]
$ omero config get x
[1, [2]]
$ omero config set x [1]
$ omero config append x 2
$ omero config append x '"3"'
$ omero config get x
[1, 2, "3"]
$ omero config set x '[{"a":"b"}]'
$ omero config append x '{"c":99}'
$ omero config get x
[{"a": "b"}, {"c": 99}]

sbesson · 2014-01-23T14:24:13Z

OK here's the next question for you @knabar

bin/omero config append x 1
bin/omero config append x 1
bin/omero config remove x 1

Are we expecting x to be unset or [1] (current behaviour)?

knabar · 2014-01-23T14:29:00Z

Two issues here:

I think remove should always only remove the first matching element
Once the last element is removed, should it be an empty list (my vote) or become unset?

manics · 2014-01-24T09:53:56Z

It depends on whether we're going to change the handling of default values in future. omero.web.ui.right_plugins is a good example:

unset implies '[["Acquisition",...],["Preview",...]]'
[] implies no tabs at all

So at present if I want to add omero_searcher to omero.web.ui.right_plugins I'll have to specify the entire list '[["Acquisition",...],["Preview",...],["Searcher",...]]',unfortunately a simple config append won't work in this situation. In the case of omero.web.apps unset implies [] anyway, so it doesn't matter, both cases work the same way. So I guess it should be []. With an RFE from me for better handling of the omero.web.ui.right_plugins default unset case.

knabar · 2014-01-24T10:05:47Z

Going to [] also keeps it symmetrical: since you cannot append to an unset value, removing the last element from a list should not go to unset. Also, you can undo any remove by running the corresponding append again (apart from the item order in the list possibly).

sbesson · 2014-01-24T11:23:21Z

Note at the moment, appending to an unset value is possible as I thought the following would be not ideal:

bin/omero config set omero.web.apps []
bin/omero config append omero.web.apps '"webfigure"'

But I can revert this possibility if that's the general consensus. @will-moore: any strong feeling about the last removal discussion? If you are okay with @manics and @knabar, I can push an extra commit that sets [] as the result of the final removal.

manics · 2014-01-24T11:51:35Z

Note at the moment, appending to an unset value is possible as I thought the following would be not ideal
...

Hmmm, good point. I guess the ideal situation would be for append to append to the default values if it's unset, but I imagine that's not possible without significant changes to the web config system.

will-moore · 2014-01-24T12:15:43Z

I want to test this myself, but haven't had time to build latest server yet...
But it would be nice if you could do this (one set of quotes).

bin/omero config append omero.web.apps "webfigure"

will-moore · 2014-01-24T12:26:35Z

I think that removing the last item should give you an empty list (not unset).
But everything changes if we could take into account the default settings.
E.g. you might want to...

# remove all the existing right plugins
omero config set omero.web.ui.right_plugins []
# add just your own
omero config append omero.web.ui.right_plugins '["Tagging"...]'
# now you want to go back to the 3 defaults (unset)
omero config set omero.web.ui.right_plugins
# now you want to add a 4th plugin to the 3 defaults
omero config append omero.web.ui.right_plugins '["Tagging"...]'

So this would be different from the 'appending to unset value' that we have now.
But that's OK I think.

joshmoore · 2014-01-24T12:31:33Z

Hmmm, good point. I guess the ideal situation would be for append to append to the default values if it's unset, but I imagine that's not possible without significant changes to the web config system.

I've wondered if we don't want to "extend" the bin/omero config command with bin/omero web-config (or whatever) to take the existing settings into account. Certainly, what web is doing with config.xml is somewhat different from the earlier intent (since it's only used locally, not remotely)

manics · 2014-01-24T12:39:56Z

👍 Would make web configuration a lot easier.

- Update corresponding unit tests - Add test for removal of multi-defined item

joshmoore · 2014-01-27T11:04:33Z

Sorry, previous comment was a misunderstanding. This was "No JSON object" when setting 'a'.

joshmoore · 2014-01-29T11:27:31Z

@knabar : I assume this is feature complete in terms of #2028. Does the fact that the defaults are now being read change the requirement for remove to go to an empty list rather than unset?

knabar · 2014-01-29T12:02:14Z

@joshmoore It should probably still go to an empty list. If you remove all the default elements one by one, it would be counter-intuitive if they pop all back up once the last element is removed.

sbesson · 2014-01-29T12:07:59Z

Seconding @knabar's answer. Arguably, we could simply extend Andrea's testRemoveWithDefault to remove all items and make sure the property go to an empty list.

sbesson · 2014-01-30T09:41:10Z

Closing in favour of #2030

sbesson added 9 commits December 5, 2013 07:36

Use monkeypatch for bin/omero config envvar unittests

b732e47

Add subcommands help tests to test_prefs.py

9729aee

Add failing tests for bin/omero config drop and set subcommands

e94b041

Add first implementation of remove and append subcommand with unit test

da99986

Fix remove with only one item and add error handling in append/remove

5e8b5bc

Add unit tests for failing append/remove calls

0b02588

Optimize duplicate line

b6406c9

Only support file input for omero config set subcommand

84a35dc

Review failing append/remove unit tests

fa11f08

knabar reviewed Dec 17, 2013
View reviewed changes

sbesson added 3 commits December 17, 2013 12:01

Fix omero config subcommands description by using parser.add()

077fe52

Add append and remove to the subcommand list in test_prefs.py

efcd371

Fix description and help of various omero config commands/arguments

bc90743

mtbc reviewed Dec 17, 2013
View reviewed changes

Expand "standard in" to "standard input"

6331ab3

joshmoore mentioned this pull request Jan 15, 2014

Additional parameters for the installation script ome/omero_searcher#19

Merged

Use JSON to read/write config lists

91fafda

Also load passed values as JSON in append/remove commands

4f55b47

Defaults config value to empty list after last item removal

727b29f

- Update corresponding unit tests - Add test for removal of multi-defined item

This was referenced Jan 27, 2014

Add and remove items from omeroweb settings #2028

Closed

Load default when appending to/removing from an unset omero.web property #2030

Merged

sbesson closed this Jan 30, 2014

sbesson deleted the 11201_config branch February 4, 2014 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add append/remove subcommands to `bin/omero config` #1905

Add append/remove subcommands to `bin/omero config` #1905

sbesson commented Dec 16, 2013

knabar Dec 17, 2013

sbesson commented Dec 17, 2013

mtbc Dec 17, 2013

joshmoore commented Jan 15, 2014

knabar commented Jan 15, 2014

sbesson commented Jan 16, 2014

sbesson commented Jan 23, 2014

knabar commented Jan 23, 2014

sbesson commented Jan 23, 2014

knabar commented Jan 23, 2014

sbesson commented Jan 23, 2014

knabar commented Jan 23, 2014

manics commented Jan 24, 2014

knabar commented Jan 24, 2014

sbesson commented Jan 24, 2014

manics commented Jan 24, 2014

will-moore commented Jan 24, 2014

will-moore commented Jan 24, 2014

joshmoore commented Jan 24, 2014

manics commented Jan 24, 2014

joshmoore commented Jan 27, 2014

joshmoore commented Jan 29, 2014

knabar commented Jan 29, 2014

sbesson commented Jan 29, 2014

sbesson commented Jan 30, 2014

Add append/remove subcommands to bin/omero config #1905

Add append/remove subcommands to bin/omero config #1905

Conversation

sbesson commented Dec 16, 2013

knabar Dec 17, 2013

Choose a reason for hiding this comment

sbesson commented Dec 17, 2013

mtbc Dec 17, 2013

Choose a reason for hiding this comment

joshmoore commented Jan 15, 2014

knabar commented Jan 15, 2014

sbesson commented Jan 16, 2014

sbesson commented Jan 23, 2014

knabar commented Jan 23, 2014

sbesson commented Jan 23, 2014

knabar commented Jan 23, 2014

sbesson commented Jan 23, 2014

knabar commented Jan 23, 2014

manics commented Jan 24, 2014

knabar commented Jan 24, 2014

sbesson commented Jan 24, 2014

manics commented Jan 24, 2014

will-moore commented Jan 24, 2014

will-moore commented Jan 24, 2014

joshmoore commented Jan 24, 2014

manics commented Jan 24, 2014

joshmoore commented Jan 27, 2014

joshmoore commented Jan 29, 2014

knabar commented Jan 29, 2014

sbesson commented Jan 29, 2014

sbesson commented Jan 30, 2014

Add append/remove subcommands to `bin/omero config` #1905

Add append/remove subcommands to `bin/omero config` #1905