Code review comment for lp:~waigani/juju-core/get-oldConfig-from-state

Reviewers: mp+209569_code.launchpad.net, axw,

https://codereview.appspot.com/70190050/diff/1/state/state.go
File state/state.go (left):

https://codereview.appspot.com/70190050/diff/1/state/state.go#oldcode282
state/state.go:282: _, err = settings.Write()
On 2014/03/06 04:19:17, axw wrote:
> Write isn't really good enough here; this does not address the deleted
TODO. The
> update to settings should assert that there has been no change to
settings since
> st.EnvironConfig() was called; otherwise the validation is not
complete.

fwereade wanted a txn retry loop, ensuring the underlying settings had
not been changed, in client.EnvironmentSet
(https://codereview.appspot.com/63790045/diff/40001/state/apiserver/client/client.go#newcode791).
As this is a WIP, I have not done it yet. I can make a TODO in
client.EnvironmentSet and write the txn retry loop in a follow up
branch?

https://codereview.appspot.com/70190050/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode259
state/state.go:259: func (st *State)
VaidateUpdateEnvironConfig(updateAttrs map[string]interface{},
removeAttrs []string, oldConfig *config.Config) error {
On 2014/03/06 04:19:17, axw wrote:
> s/Vaidate/Validate/
> also, doesn't need to be exported does it?

Good catch!

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode264
state/state.go:264: // Build new config with attributes to add/update
On 2014/03/06 04:19:17, axw wrote:
> You can get rid of the comments, if statement, forward declared
newConfig/err
> vars, and just replace with:

> newConfig, err := oldConfig.Apply(updateAttrs)
> if err != nil {
> return err
> }

> Apply doesn't care if updateAttrs is nil or empty.

Done.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode274
state/state.go:274: if len(removeAttrs) != 0 {
On 2014/03/06 04:19:17, axw wrote:
> And then since newConfig exists unconditionally:

> newConfig, err = newConfig.Remove(removeAttrs)
> if err != nil {
> return err
> }

Done.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode304
state/state.go:304: func (st *State) SetEnvironConfig(attrs
map[string]interface{}) error {
On 2014/03/06 04:19:17, axw wrote:
> Why do we still need SetEnvironConfig? This still has the old race
conditions.
> *If* it's needed, then SetEnvironConfig and UpdateEnvironConfig should
use a
> common method that takes a *state.Settings as input and does the
> no-concurrent-change check on that.

It was needed by testing.ChangeEnvironConfig. ChangeEnvironConfig is
only used in two places, but it is actually easier to use
UpdateEnvironConfig directly instead. So I would be in favour of
deleting SetEnvironConfig and ChangeEnvironConfig.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode327
state/state.go:327: oldConfig, err := st.EnvironConfig()
On 2014/03/06 04:19:17, axw wrote:
> This does a readSettings, so you're reading settings twice. I think
it'd be
> preferable to just do a readSettings and use its Map() to create a
config.

Done.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode333
state/state.go:333: err = st.VaidateUpdateEnvironConfig(updateAttrs,
removeAttrs, oldConfig)
On 2014/03/06 04:19:17, axw wrote:
> Since ValidateUpdateEnvironConfig updates the config with attributes
to
> validate, I wonder if it's worthwhile even having a separate method.
You're
> duplicating work here by updating settings.

> I think it would be simpler to just:
> 1. get settings, and create a config from them
> 2. apply changes to get a new config, check validity
> 3. do something *like* replaceSettingsOp (see settings.go), but
passing in
> the *state.Settings

Do you think something *like* replaceSettingsOp should happen within
state.UpdateEnvironConfig or client.EnvironmentSet?

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode351
state/state.go:351: // Add/Update attributes.
On 2014/03/06 04:19:17, axw wrote:
> These comments aren't really useful, the code speaks for itself.

Done.

Description:
WIP new state function UpdateEnvironConfig

This is a work in progress. The main business
logic that has changed is in state.
UpdateEnvironConfig replaces the old SetEnvironConfig.
As validation now happens in state, a lot of tests fail
and need to be updated.

https://code.launchpad.net/~waigani/juju-core/get-oldConfig-from-state/+merge/209569

(do not edit description out of merge proposal)

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

Affected files (+417, -404 lines):
   A [revision details]
   M cmd/juju/environment.go
   M cmd/juju/environment_test.go
   M cmd/juju/upgradejuju_test.go
   M cmd/jujud/machine_test.go
   M environs/config/config.go
   M environs/statepolicy.go
   M environs/testing/tools.go
   M juju/conn.go
   M juju/conn_test.go
   M juju/testing/repo.go
   M juju/testing/utils.go
   M state/api/common/testing/environwatcher.go
   M state/api/firewaller/state_test.go
   M state/apiserver/client/client.go
   M state/apiserver/client/client_test.go
   M state/apiserver/keymanager/keymanager.go
   M state/apiserver/provisioner/provisioner_test.go
   M state/apiserver/rsyslog/rsyslog.go
   A state/configvalidator_test.go
   M state/conn_test.go
   M state/initialize_test.go
   M state/machine_test.go
   M state/policy.go
   M state/state.go
   M state/state_test.go
   M state/testing/config.go
   M worker/environ_test.go
   M worker/firewaller/firewaller_test.go
   M worker/instancepoller/observer_test.go
   M worker/machineenvironmentworker/machineenvironmentworker_test.go
   M worker/provisioner/provisioner_test.go
   M worker/uniter/uniter_test.go