Please take a look.


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
On 2014/03/17 12:15:56, axw wrote:
> No need to type assert, since EnvironProvider implements
state.ConfigValidator.
> Just "return provider, nil".

Done.

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#newcode44
environs/statepolicy.go:44: return nil,
errors.NewNotImplementedError("ConfigValidator")
On 2014/03/17 12:15:56, axw wrote:
> Lose the second return

Done.

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
On 2014/03/17 12:15:56, axw wrote:
> Please leave a space after //, preferably end sentence with a .

Done.

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)
On 2014/03/17 12:15:56, axw wrote:
> check error

Done.

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"])
On 2014/03/17 12:15:56, axw wrote:
> leftover logging

Done.

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
On 2014/03/17 12:15:56, axw wrote:
> 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).

Done.

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode316
state/state.go:316: //use validCfg to update settings
On 2014/03/17 12:15:56, axw wrote:
> Just drop this comment

Done.

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 {
On 2014/03/17 12:15:56, axw wrote:
> 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).

Done.

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"}}}})
On 2014/03/17 12:15:56, axw wrote:
> 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.

Done. Changed to UpdateId. As "name" has been removed, settings errors
out in UpdateEnvironConig. I'm not sure if this is the best way around
it, but it is a way.

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) {
On 2014/03/17 12:15:56, axw wrote:
> As before, reinstate until we can guarantee config validity.

Done.

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