Code review comment for lp:~axwalk/juju-core/cmd-juju-unset-env

Please take a look.

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

https://codereview.appspot.com/80380043/diff/1/cmd/juju/environment.go#newcode192
cmd/juju/environment.go:192: Set one or more the environment
configuration attributes to its default value
On 2014/03/26 06:13:43, wallyworld wrote:
> s/Set/Reset ?

Done.

https://codereview.appspot.com/80380043/diff/1/cmd/juju/environment.go#newcode193
cmd/juju/environment.go:193: in a running Juju instance. Attributes
without defaults are removed.
On 2014/03/26 06:13:43, wallyworld wrote:
> Perhaps include an example to show they are space separated? Or
mention that in
> the help?

Clarified in documentation.

https://codereview.appspot.com/80380043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/80380043/diff/1/state/api/params/params.go#newcode594
state/api/params/params.go:594: type EnvironmentUnset struct {
On 2014/03/26 06:13:43, wallyworld wrote:
> I wish there were a generic string args struct. Maybe there is, not
sure.

Nope, that I can see. Anyway, having API-specific ones means the struct
can be updated with additional fields if necessary.

https://codereview.appspot.com/80380043/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/80380043/diff/1/state/apiserver/client/client_test.go#newcode1579
state/apiserver/client/client_test.go:1579: c.Assert(err,
gc.ErrorMatches, "type: expected string, got nothing")
On 2014/03/26 06:13:43, wallyworld wrote:
> I don't like this error message. I realise it's the output of validate
but I'd
> expect to see "cannot unset blah" or something if i were a user.
Perhaps we
> could just wrap the error so it reads `cannot unset environment
attribute
> "type": blah blah`

I agree the error sucks. The operaton isn't that granular, though. We're
saying "make all these changes to the config", and then checking if the
result is any good.

You can't necessarily remove them one at a time, either. It's feasible
that there are co-requisite attributes.

Perhaps the right thing to do is to have a method on EnvironProvider
that returns a config with defaults for all attributes that have
defaults. Then the attribute removal code can just reset them back to
those values, or error if there's no default and it's required. That's a
big change, though, and not something I want to drag into this.

https://codereview.appspot.com/80380043/