Merge lp://staging/~thumper/juju-core/set-environment into lp://staging/~juju/juju-core/trunk

Reviewers: mp+157986_code.launchpad.net,

Message:
Please take a look.

Description:
Implement the set-environment command.

This change introduces a new command called set-environment. It has an
alias
of set-env, and is the pair to get-environment.

Key/value pairs are taken on the command line as positional parameters.

The environment is retrieved from state, updated with the new values,
and then
validated using the provider Validate method. The resulting
configuration is
passed bach through to the state.SetEnvironConfig.

There are some standard config values that we really don't want
changing. These rules have been encoded in a new function
config.Validate.
This is now called from each of the provider validate methods.

I added a non-exported helper method on Config to do the type coersion
for
getting values from the known map. This is now used in many internal
places.

Since the agent-version was always added in the config.New to be the
default
value, I instead add this to the schema default values. The existing
tests
make sure this still works as intended.

Added an extra test for backslashes in names. Moved the firewall-mode
default
behaviour up into the base class, as it was identical in all three
provider
validate methods.

https://code.launchpad.net/~thumper/juju-core/set-environment/+merge/157986

Requires:
https://code.launchpad.net/~thumper/juju-core/get-environment/+merge/157968

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8610043/

Affected files:
   A [revision details]
   M cmd/juju/environment.go
   M cmd/juju/environment_test.go
   M cmd/juju/main.go
   M cmd/juju/main_test.go
   M environs/config/config.go
   M environs/config/config_test.go
   M environs/dummy/environs.go
   M environs/ec2/config.go
   M environs/openstack/config.go

LGTM modulo comments below

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

https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment.go#newcode117
cmd/juju/environment.go:117: c.values[key] = bits[1]
Hmm, do we have some key=value parsing code elsewhere? Not a blocker,
but maybe worth a TODO.

https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment.go#newcode132
cmd/juju/environment.go:132: stateConfig, err :=
conn.State.EnvironConfig()
oldConfig, to go with newConfig below?

https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment.go#newcode147
cmd/juju/environment.go:147: // Now try to apply the new validate
config.
s/validate/validated/

https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment.go#newcode148
cmd/juju/environment.go:148: return
conn.State.SetEnvironConfig(newProviderConfig)
I'm still concerned about the behaviour of SetEnvironConfig, but that's
orthogonal.

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

https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment_test.go#newcode130
cmd/juju/environment_test.go:130: }
test for more than one value?

https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment_test.go#newcode140
cmd/juju/environment_test.go:140: for name, value := range
immutableConfigTests {
I would still prefer a whitelist to a blacklist, but we don't need to do
that this CL (and I might be argued out of it anyway).

https://codereview.appspot.com/8610043/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/8610043/diff/1/environs/config/config.go#newcode122
environs/config/config.go:122: // it if necessary, and returns the
validated configuration. If old is not
Doesn't return a validated config

https://codereview.appspot.com/8610043/diff/1/environs/config/config.go#newcode138
environs/config/config.go:138: // The schema always makes sure something
is set.
This shouldn't be the case -- it should be ok to bootstrap without an
explicit agent-version, and have juju select appropriate tools. (That's
the one case I am happy with slightly-fuzzy tools selection.)

https://codereview.appspot.com/8610043/diff/1/environs/config/config.go#newcode337
environs/config/config.go:337: "agent-version":
version.CurrentNumber().String(),
Unset should be valid

https://codereview.appspot.com/8610043/

Will repropose too.

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

https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment.go#newcode117
cmd/juju/environment.go:117: c.values[key] = bits[1]
On 2013/04/10 12:45:20, fwereade wrote:
> Hmm, do we have some key=value parsing code elsewhere? Not a blocker,
but maybe
> worth a TODO.

Done.

https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment.go#newcode132
cmd/juju/environment.go:132: stateConfig, err :=
conn.State.EnvironConfig()
On 2013/04/10 12:45:20, fwereade wrote:
> oldConfig, to go with newConfig below?

Done.

https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment.go#newcode147
cmd/juju/environment.go:147: // Now try to apply the new validate
config.
On 2013/04/10 12:45:20, fwereade wrote:
> s/validate/validated/

Done.

https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment.go#newcode148
cmd/juju/environment.go:148: return
conn.State.SetEnvironConfig(newProviderConfig)
On 2013/04/10 12:45:20, fwereade wrote:
> I'm still concerned about the behaviour of SetEnvironConfig, but
that's
> orthogonal.

Yes I agree. I think that we should potentially pass in both the old
and new config to SetEnvironConfig in order to determine changes. Then
we could add conditions to the update to make sure that we aren't
updating something that someone else has tweaked in the mean time, and
that we wouldn't wipe out someone else's changes. Filed bug lp:1167616
and added a card to the backlog.

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

https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment_test.go#newcode130
cmd/juju/environment_test.go:130: }
On 2013/04/10 12:45:20, fwereade wrote:
> test for more than one value?

Sure... if you feel the need. I do have a test above for the parsing of
the options given multiple values, but we can do this...

https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment_test.go#newcode140
cmd/juju/environment_test.go:140: for name, value := range
immutableConfigTests {
On 2013/04/10 12:45:20, fwereade wrote:
> I would still prefer a whitelist to a blacklist, but we don't need to
do that
> this CL (and I might be argued out of it anyway).

I think that given we already had a way for validations to be done it
was reasonable to use it. Black lists don't need as much updating as
new values are added. Black lists are obviously easier to implement,
but I am open to an alternative if we need to change it later.

https://codereview.appspot.com/8610043/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/8610043/diff/1/environs/config/config.go#newcode122
environs/config/config.go:122: // it if necessary, and returns the
validated configuration. If old is not
On 2013/04/10 12:45:20, fwereade wrote:
> Doesn't return a validated config

Done.

https://codereview.appspot.com/8610043/diff/1/environs/config/config.go#newcode138
environs/config/config.go:138: // The schema always makes sure something
is set.
On 2013/04/10 12:45:20, fwe...

Please take a look.

https://codereview.appspot.com/8610043/

*** Submitted:

Implement the set-environment command.

This change introduces a new command called set-environment. It has an
alias
of set-env, and is the pair to get-environment.

Key/value pairs are taken on the command line as positional parameters.

The environment is retrieved from state, updated with the new values,
and then
validated using the provider Validate method. The resulting
configuration is
passed bach through to the state.SetEnvironConfig.

There are some standard config values that we really don't want
changing. These rules have been encoded in a new function
config.Validate.
This is now called from each of the provider validate methods.

I added a non-exported helper method on Config to do the type coersion
for
getting values from the known map. This is now used in many internal
places.

Since the agent-version was always added in the config.New to be the
default
value, I instead add this to the schema default values. The existing
tests
make sure this still works as intended.

Added an extra test for backslashes in names. Moved the firewall-mode
default
behaviour up into the base class, as it was identical in all three
provider
validate methods.

R=fwereade, dfc
CC=
https://codereview.appspot.com/8610043

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

https://codereview.appspot.com/8610043/diff/8001/cmd/juju/environment.go#newcode132
cmd/juju/environment.go:132: // Get the existing environment config from
the state.
On 2013/04/10 23:56:27, dfc wrote:
> This logic is moderately subtle, and I have a memory that we do the
same thing
> elsewhere, when uploading/updating the secrets in the environment
configuration.
> Could you please add a TODO or card to rationalise this.

Done.

https://codereview.appspot.com/8610043/diff/8001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/8610043/diff/8001/environs/config/config.go#newcode336
environs/config/config.go:336: "agent-version":
version.CurrentNumber().String(),
On 2013/04/10 23:56:27, dfc wrote:
> hmm, why has this changed ?

Just rationalizing the default process, was set in config.New before.

https://codereview.appspot.com/8610043/