Implement the set-environment command.

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)

[thumper, 2013-04-10 04:04:39]

Please take a look.

[fwereade, 2013-04-10 12:45:20]

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#ne...
cmd/juju/environment_test.go:130: }
test for more than one value?

https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment_test.go#ne...
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#newco...
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#newco...
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#newco...
environs/config/config.go:337: "agent-version":            
version.CurrentNumber().String(),
Unset should be valid

[thumper, 2013-04-10 23:17:23]

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#ne...
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#ne...
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#newco...
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#newco...
environs/config/config.go:138: // The schema always makes sure something is set.
On 2013/04/10 12:45:20, fwereade wrote:
> 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.)

As discussed on IRC, we'll leave this change here for now.

https://codereview.appspot.com/8610043/diff/1/environs/config/config.go#newco...
environs/config/config.go:337: "agent-version":            
version.CurrentNumber().String(),
On 2013/04/10 12:45:20, fwereade wrote:
> Unset should be valid

As above, keep this for now until your change appears.

[thumper, 2013-04-10 23:40:15]

Please take a look.

[dave_cheney.net, 2013-04-10 23:56:27]

LGTM with a few comments. Excellent work deduplicating the logic inside the
providers.

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#newc...
cmd/juju/environment.go:132: // Get the existing environment config from the
state.
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.

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#ne...
environs/config/config.go:336: "agent-version":            
version.CurrentNumber().String(),
hmm, why has this changed ?

[thumper, 2013-04-11 00:24:31]

*** 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#newc...
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#ne...
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.