cmd/juju: added new command unset

cmd/juju: added new command unset

As a part of the issue lp:1194945 juju now gets an
own command to set service options back to their
default values (aka unset). This later allows in
a second Cl to use "set <SVC> option=" to set a
string option to an empty string. Today this call
leads to the unsetting of that option.

https://code.launchpad.net/~themue/juju-core/038-cmd-unset/+merge/181248

(do not edit description out of merge proposal)

[mue, 2013-08-21 10:45:26]

Please take a look.

[fwereade, 2013-08-21 16:04:20]

NOT LGTM, but should be simple to fix.

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

https://codereview.appspot.com/12752044/diff/1/cmd/juju/config_test.go#newcod...
cmd/juju/config_test.go:190: command = &UnsetCommand{}
I don't really like this -- the tests get pretty unwieldy, I think. Would you
break it out into two test cases for the two commands, please?

https://codereview.appspot.com/12752044/diff/1/cmd/juju/unset.go
File cmd/juju/unset.go (right):

https://codereview.appspot.com/12752044/diff/1/cmd/juju/unset.go#newcode80
cmd/juju/unset.go:80: settings[option] = defaultSetting
This is completely wrong. This *sets*, not unsets, and invalidates the point of
having an unset command in the first place.

Just construct a map of names, with all values nil, and do the update.

[mue, 2013-08-22 10:01:35]

On 2013/08/21 16:04:20, fwereade wrote:
> NOT LGTM, but should be simple to fix.
> 
> https://codereview.appspot.com/12752044/diff/1/cmd/juju/config_test.go
> File cmd/juju/config_test.go (right):
> 
>
https://codereview.appspot.com/12752044/diff/1/cmd/juju/config_test.go#newcod...
> cmd/juju/config_test.go:190: command = &UnsetCommand{}
> I don't really like this -- the tests get pretty unwieldy, I think. Would you
> break it out into two test cases for the two commands, please?

Had it before but I need the ability to set options to test the unsetting. So
the unset test would get like this one (or very similar). That's why I combined
it.

> https://codereview.appspot.com/12752044/diff/1/cmd/juju/unset.go
> File cmd/juju/unset.go (right):
> 
> https://codereview.appspot.com/12752044/diff/1/cmd/juju/unset.go#newcode80
> cmd/juju/unset.go:80: settings[option] = defaultSetting
> This is completely wrong. This *sets*, not unsets, and invalidates the point
of
> having an unset command in the first place.
> 
> Just construct a map of names, with all values nil, and do the update.

unset is just the chosen name to set the values back to default, what's one here
explicitly.The other way would be more implicit and rely on setting values back
to default when the argument is empty. But we want to allow empty arguments
later in the set command to set string options to empty strings (when their
default is not empty). So that implicit behavior will change.

[mue, 2013-08-23 12:25:12]

Please take a look.

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

https://codereview.appspot.com/12752044/diff/1/cmd/juju/config_test.go#newcod...
cmd/juju/config_test.go:190: command = &UnsetCommand{}
On 2013/08/21 16:04:20, fwereade wrote:
> I don't really like this -- the tests get pretty unwieldy, I think. Would you
> break it out into two test cases for the two commands, please?

Done.

https://codereview.appspot.com/12752044/diff/1/cmd/juju/unset.go
File cmd/juju/unset.go (right):

https://codereview.appspot.com/12752044/diff/1/cmd/juju/unset.go#newcode80
cmd/juju/unset.go:80: settings[option] = defaultSetting
On 2013/08/21 16:04:20, fwereade wrote:
> This is completely wrong. This *sets*, not unsets, and invalidates the point
of
> having an unset command in the first place.
> 
> Just construct a map of names, with all values nil, and do the update.

Done.

[fwereade, 2013-08-26 15:06:21]

LGTM modulo agreement on whether to show nil values with "default: true" in get.
Were we already doing that?

https://codereview.appspot.com/12752044/diff/8001/cmd/juju/get_test.go
File cmd/juju/get_test.go (right):

https://codereview.appspot.com/12752044/diff/8001/cmd/juju/get_test.go#newcode42
cmd/juju/get_test.go:42: "default":     true,
I think it's more correct to consider a nil default as "no default", and hence
to skip that field if the value is nil (because a nil default is the only way to
get an actual nil value, right?).

https://codereview.appspot.com/12752044/diff/8001/cmd/juju/unset_test.go
File cmd/juju/unset_test.go (right):

https://codereview.appspot.com/12752044/diff/8001/cmd/juju/unset_test.go#newc...
cmd/juju/unset_test.go:51: 
May as well split this test case here.

[mue, 2013-08-27 10:39:26]

Please take a look.

https://codereview.appspot.com/12752044/diff/8001/cmd/juju/get_test.go
File cmd/juju/get_test.go (right):

https://codereview.appspot.com/12752044/diff/8001/cmd/juju/get_test.go#newcode42
cmd/juju/get_test.go:42: "default":     true,
On 2013/08/26 15:06:21, fwereade wrote:
> I think it's more correct to consider a nil default as "no default", and hence
> to skip that field if the value is nil (because a nil default is the only way
to
> get an actual nil value, right?).

As discussed will be done in an extra CL when these two are in.

https://codereview.appspot.com/12752044/diff/8001/cmd/juju/unset_test.go
File cmd/juju/unset_test.go (right):

https://codereview.appspot.com/12752044/diff/8001/cmd/juju/unset_test.go#newc...
cmd/juju/unset_test.go:51: 
On 2013/08/26 15:06:21, fwereade wrote:
> May as well split this test case here.

Done.