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

A few minor things, then it should be good to land.

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go
File environs/statepolicy.go (right):

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#newcode43
environs/statepolicy.go:43: return provider.(state.ConfigValidator), nil
No need to type assert, since EnvironProvider implements
state.ConfigValidator. Just "return provider, nil".

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#newcode44
environs/statepolicy.go:44: return nil,
errors.NewNotImplementedError("ConfigValidator")
Lose the second return

https://codereview.appspot.com/70190050/diff/100001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/70190050/diff/100001/juju/conn_test.go#newcode143
juju/conn_test.go:143: //Use a state without a nil policy, which will
allow us to set an invalid config
Please leave a space after //, preferably end sentence with a .

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go
File state/api/common/testing/environwatcher.go (right):

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go#newcode92
state/api/common/testing/environwatcher.go:92: err =
s.st.UpdateEnvironConfig(map[string]interface{}{}, []string{"foo"}, nil)
check error

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go#newcode94
state/api/common/testing/environwatcher.go:94: c.Logf("!!!!! %#v",
cf.AllAttrs()["logging-config"])
leftover logging

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

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode279
state/state.go:279: type ValidateFn func(updateAttrs
map[string]interface{}, removeAttrs []string, oldConfig *config.Config)
error
This could do with a better name. Out of context (e.g. in godoc), it
wouldn't really be meaningful. I suggest ValidateConfigFunc (Func,
rather than Fn, to be consistent with function types in Go's standard
library).

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode316
state/state.go:316: //use validCfg to update settings
Just drop this comment

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

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go#newcode1689
state/state_test.go:1689: if v, found := updateAttrs["agent-version"];
found {
All we really care about is: did the callback get called with the
appropriate arguments, and does its result have the appropriate effect?

So there's some missing coverage there: there's no validation of the
input, and there's no test for a non-nil validator with a nil result
(i.e. has no effect on normal operation).

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go#newcode1818
state/state_test.go:1818: err = settings.Update(nil, bson.D{{"$set",
bson.D{{"name", "foo"}}}})
What is this doing here? Is this because UpdateEnvironConfig won't
change the "name" attribute? If it's needed, you should probably be
using settings.UpdateId.

https://codereview.appspot.com/70190050/diff/100001/worker/provisioner/provisioner_test.go
File worker/provisioner/provisioner_test.go (left):

https://codereview.appspot.com/70190050/diff/100001/worker/provisioner/provisioner_test.go#oldcode595
worker/provisioner/provisioner_test.go:595: func (s *ProvisionerSuite)
TestProvisioningRecoversAfterInvalidEnvironmentPublished(c *gc.C) {
As before, reinstate until we can guarantee config validity.

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