Code review comment for lp://staging/~thumper/juju-core/set-environment

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, 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#newcode337
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.

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