cmd/juju: get- and set-environment using the API

cmd/juju: get- and set-environment using the API

Implemented EnvironmentGet and EnvironmentSet in
the client API and changed the get-environment
and set-environment commands to use that instead
of connecting to state directly.

https://code.launchpad.net/~dimitern/juju-core/200-cli-get-set-environment/+merge/194082

(do not edit description out of merge proposal)

[dimitern, 2013-11-06 09:14:00]

Please take a look.

[dimitern, 2013-11-06 09:17:59]

Please take a look.

[fwereade, 2013-11-06 09:33:54]

A couple of thoughts -- and tbh I think I'd be happiest without Key/Keys at all.

Also, tests.

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

https://codereview.appspot.com/22210044/diff/40001/state/api/params/params.go...
state/api/params/params.go:434: Key string
Shouldn't this be Keys, really?

https://codereview.appspot.com/22210044/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/22210044/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:603: return
c.api.state.SetEnvironConfig(newProviderConfig)
Wouln't it be better to just SetConfig on that environment than muck around with
the provider?

(And I wish we could make this not totally suck, but, ehh. At least we're moving
it behind the api.)

[dimitern, 2013-11-06 10:53:43]

Please take a look.

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

https://codereview.appspot.com/22210044/diff/40001/state/api/params/params.go...
state/api/params/params.go:434: Key string
On 2013/11/06 09:33:54, fwereade wrote:
> Shouldn't this be Keys, really?

As discussed online. Removed.

https://codereview.appspot.com/22210044/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/22210044/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:603: return
c.api.state.SetEnvironConfig(newProviderConfig)
On 2013/11/06 09:33:54, fwereade wrote:
> Wouln't it be better to just SetConfig on that environment than muck around
with
> the provider?
> 
> (And I wish we could make this not totally suck, but, ehh. At least we're
moving
> it behind the api.)

The provider is used to validate the new config, and there are tests about this
behavior (immutable settings). If I remove it, we'll have the possibility of
some invalid settings slipping through the validation.

[fwereade, 2013-11-06 12:04:23]

On 2013/11/06 10:53:43, dimitern wrote:
> The provider is used to validate the new config, and there are tests about
this
> behavior (immutable settings). If I remove it, we'll have the possibility of
> some invalid settings slipping through the validation.

SetConfig on the old environ should do that anyway; if not, it's broken. But
that's by the by, looking at the rest now.

[fwereade, 2013-11-06 12:10:38]

LGTM with the below. Discussion encouraged :).

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

https://codereview.appspot.com/22210044/diff/60001/state/api/params/params.go...
state/api/params/params.go:433: Results map[string]interface{}
Config maybe?

https://codereview.appspot.com/22210044/diff/60001/state/api/params/params.go...
state/api/params/params.go:439: Items map[string]interface{}
ditto

https://codereview.appspot.com/22210044/diff/60001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/22210044/diff/60001/state/apiserver/client/cli...
state/apiserver/client/client.go:571: func (c *Client) EnvironmentSet(args
params.EnvironmentSet) error {
Would be good to badge this with lp:1167616 for the convenience of whoever ends
up fixing it in the end.

https://codereview.appspot.com/22210044/diff/60001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/22210044/diff/60001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1258: }
Explicit test that agent-version can't be change would be good.

[dimitern, 2013-11-06 14:14:32]

Please take a look.

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

https://codereview.appspot.com/22210044/diff/60001/state/api/params/params.go...
state/api/params/params.go:433: Results map[string]interface{}
On 2013/11/06 12:10:39, fwereade wrote:
> Config maybe?

Done.

https://codereview.appspot.com/22210044/diff/60001/state/api/params/params.go...
state/api/params/params.go:439: Items map[string]interface{}
On 2013/11/06 12:10:39, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/22210044/diff/60001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/22210044/diff/60001/state/apiserver/client/cli...
state/apiserver/client/client.go:571: func (c *Client) EnvironmentSet(args
params.EnvironmentSet) error {
On 2013/11/06 12:10:39, fwereade wrote:
> Would be good to badge this with lp:1167616 for the convenience of whoever
ends
> up fixing it in the end.

Done.

https://codereview.appspot.com/22210044/diff/60001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/22210044/diff/60001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1258: }
On 2013/11/06 12:10:39, fwereade wrote:
> Explicit test that agent-version can't be change would be good.

Done.

[fwereade, 2013-11-06 14:56:55]

Thanks -- still LGTM :).

https://codereview.appspot.com/22210044/diff/80001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/22210044/diff/80001/state/apiserver/client/cli...
state/apiserver/client/client.go:582: return fmt.Errorf("agent-version cannot be
changed")
Explicit agent-version setting *might* be ok so long as it's not a *change*.
Depends on usage I guess -- probably fine to keep it as it is until it bothers
someone.