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

Some preliminary comments. I've only really looked at state.go.

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()
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.

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 {
s/Vaidate/Validate/
also, doesn't need to be exported does it?

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode264
state/state.go:264: // Build new config with attributes to add/update
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.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode274
state/state.go:274: if len(removeAttrs) != 0 {
And then since newConfig exists unconditionally:

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

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode304
state/state.go:304: func (st *State) SetEnvironConfig(attrs
map[string]interface{}) error {
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.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode327
state/state.go:327: oldConfig, err := st.EnvironConfig()
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.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode333
state/state.go:333: err = st.VaidateUpdateEnvironConfig(updateAttrs,
removeAttrs, oldConfig)
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

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode351
state/state.go:351: // Add/Update attributes.
These comments aren't really useful, the code speaks for itself.

https://codereview.appspot.com/70190050/