cmd/set: flag to set default value

cmd/set: flag to set default value

This is the first of two CL for issue lp:1194945. Goal is a behavior
change in setting an option to its default value. which is today done
by calling

juju set <svc> option=

and with this CL by

juju set <svc> --default option

This allows to do another change in a second CL where the first
way of executing the set command will set a string option to an
empty string. This may brake compatibility, so a query is sent to
the mailing list.

https://code.launchpad.net/~themue/juju-core/036-set-default/+merge/178279

(do not edit description out of merge proposal)

[mue, 2013-08-02 12:12:32]

Please take a look.

[mue, 2013-08-02 12:18:00]

Please take a look.

[dimitern, 2013-08-02 15:53:54]

A few thoughts

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/config_test.go
File cmd/juju/config_test.go (right):

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/config_test.go#new...
cmd/juju/config_test.go:133: about: "set to default value nil (aka remove
setting)",
"set to a nil default value (aka remove a setting)"

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/config_test.go#new...
cmd/juju/config_test.go:142: }, {
Please more tests:
--default validoption=
--default validoption=value

Assuming --default is a flag, then tests like this as well:
--default validoption1 validoption2
--default validoption1 invalidoption validoption2
(in the last case, as a user I assume validoption1 and 2 will be set to default,
but the invalidoption will be reported as error perhaps)?

where validoption is a valid service config option.

Also add tests mixing --default and other key=value pairs, like:
key1=val1 key2=val2 --default key3 key4
and make sure the suggested error is returned (cannot mix --default and pairs).

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/set.go
File cmd/juju/set.go (right):

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/set.go#newcode30
cmd/juju/set.go:30: Args:    "<service> name=value ...",
Args: "<service> [name=value ...] | [--default name name]"

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/set.go#newcode31
cmd/juju/set.go:31: Purpose: "set service config options",
Purpose: "set service config options or reset to defaults"

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/set.go#newcode32
cmd/juju/set.go:32: Doc:     "Set one or more configuration options for the
specified service.",
Doc: "There are two ways to invoke this command:

juju set <service> name1=value2 name2=value2 ...
Sets one of more configuration options for the specified service. Options to set
are specified as one or more name=value pairs.

juju set <service> --default name1 name2 ...
Reset the specified configuration options (name1, name2) to their default
values."

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/set.go#newcode90
cmd/juju/set.go:90: settings, err =
ch.Config().ParseSettingsStrings(c.SettingsStrings)
if c.Default {
    return fmt.Errorf("--default cannot be used with name=value pairs.")
}

.. before reading c.SettingsStrings

[mue, 2013-08-02 17:23:46]

Please take a look.

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/config_test.go
File cmd/juju/config_test.go (right):

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/config_test.go#new...
cmd/juju/config_test.go:133: about: "set to default value nil (aka remove
setting)",
On 2013/08/02 15:53:54, dimitern wrote:
> "set to a nil default value (aka remove a setting)"

Done.

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/config_test.go#new...
cmd/juju/config_test.go:142: }, {
On 2013/08/02 15:53:54, dimitern wrote:
> Please more tests:
> --default validoption=
> --default validoption=value
> 
> Assuming --default is a flag, then tests like this as well:
> --default validoption1 validoption2
> --default validoption1 invalidoption validoption2
> (in the last case, as a user I assume validoption1 and 2 will be set to
default,
> but the invalidoption will be reported as error perhaps)?
> 
> where validoption is a valid service config option.
> 
> Also add tests mixing --default and other key=value pairs, like:
> key1=val1 key2=val2 --default key3 key4
> and make sure the suggested error is returned (cannot mix --default and
pairs).

Done.

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/set.go
File cmd/juju/set.go (right):

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/set.go#newcode30
cmd/juju/set.go:30: Args:    "<service> name=value ...",
On 2013/08/02 15:53:54, dimitern wrote:
> Args: "<service> [name=value ...] | [--default name name]"

Done.

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/set.go#newcode31
cmd/juju/set.go:31: Purpose: "set service config options",
On 2013/08/02 15:53:54, dimitern wrote:
> Purpose: "set service config options or reset to defaults"

Done.

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/set.go#newcode32
cmd/juju/set.go:32: Doc:     "Set one or more configuration options for the
specified service.",
On 2013/08/02 15:53:54, dimitern wrote:
> Doc: "There are two ways to invoke this command:
> 
> juju set <service> name1=value2 name2=value2 ...
> Sets one of more configuration options for the specified service. Options to
set
> are specified as one or more name=value pairs.
> 
> juju set <service> --default name1 name2 ...
> Reset the specified configuration options (name1, name2) to their default
> values."

Done.

https://codereview.appspot.com/12347043/diff/4001/cmd/juju/set.go#newcode90
cmd/juju/set.go:90: settings, err =
ch.Config().ParseSettingsStrings(c.SettingsStrings)
On 2013/08/02 15:53:54, dimitern wrote:
> if c.Default {
>     return fmt.Errorf("--default cannot be used with name=value pairs.")
> }
> 
> .. before reading c.SettingsStrings

Found a different way signalling the error.

[dimitern, 2013-08-02 17:28:11]

LGTM, thank you!

https://codereview.appspot.com/12347043/diff/9001/cmd/juju/config_test.go
File cmd/juju/config_test.go (right):

https://codereview.appspot.com/12347043/diff/9001/cmd/juju/config_test.go#new...
cmd/juju/config_test.go:145: err:   "error: cannot mix default and value setting
of option \"username\"\n",
s/default/--default/ (all cases below and the error itself).

https://codereview.appspot.com/12347043/diff/9001/cmd/juju/set.go
File cmd/juju/set.go (right):

https://codereview.appspot.com/12347043/diff/9001/cmd/juju/set.go#newcode110
cmd/juju/set.go:110: return fmt.Errorf("cannot mix default and value setting of
option %q", parts[0])
s/default/--default/

[jameinel, 2013-08-08 16:56:35]

The fact that '--default' changes how we parse the rest of the arguments makes
me wonder if we would be happier with a "juju reset foo bar baz" command or
"juju unset foo bar baz".

However the code is generally fine, and if it has been discussed, I'm not really
trying to paint the bike shed.

I'd like to see that we handle mixed args (you already handle the --default
foo=bar case, so checking the --default --config x case is sane would be nice.)

LGTM

https://codereview.appspot.com/12347043/diff/9001/cmd/juju/config_test.go
File cmd/juju/config_test.go (left):

https://codereview.appspot.com/12347043/diff/9001/cmd/juju/config_test.go#old...
cmd/juju/config_test.go:123: }, {
It feels like we are missing a test about what happens if you pass both
"--default" and "--config".

I think the answer is that we see it and consider the request invalid. I'd like
to be explicit about it, though.