State policy based config validation.

State policy based config validation.

First pass at using state policy to preform environ specific config validations.

https://code.launchpad.net/~waigani/juju-core/trunk/+merge/207586

(do not edit description out of merge proposal)

[waigani, 2014-02-21 03:42:06]

Please take a look.

[axw, 2014-02-21 04:07:35]

Preliminary comments.

I think it'd be a good idea to tone back the changes to SetEnvironConfig for
this CL, and just add the validation check after checkEnvironConfig (leaving the
TODO and everything else as it is). Up to you, though.

Please also update the API server code (stateserver/client/client.go,
EnvironSet) to remove the Validate call which will become redundant.

https://codereview.appspot.com/63790045/diff/1/environs/statepolicy.go
File environs/statepolicy.go (right):

https://codereview.appspot.com/63790045/diff/1/environs/statepolicy.go#newcode38
environs/statepolicy.go:38: env, err := New(cfg)
We don't need an environ here, we should be using the provider. You can get that
with: environs.Provider(cfg.Type())

https://codereview.appspot.com/63790045/diff/1/environs/statepolicy.go#newcode43
environs/statepolicy.go:43: if c, ok := env.(state.ConfigValidator); ok {
And here you'd just return provider: state.ConfigValidator should have a method
with the same signature as EnvironProvider.Validate.

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

https://codereview.appspot.com/63790045/diff/1/state/policy.go#newcode27
state/policy.go:27: // ConfigValidator takes a *config.Config and returns
Space before the comment please, to separate the fields: they're logical
independent things, so grouping isn't meaningful.

https://codereview.appspot.com/63790045/diff/1/state/policy.go#newcode49
state/policy.go:49: ValidateEnvironsConfig(newConfig *config.Config, oldConfig
*config.Config) (*config.Config, error)
If you just call the method Validate, then environs.EnvironProvider already
implements this.

https://codereview.appspot.com/63790045/diff/1/state/policy.go#newcode75
state/policy.go:75: // a Validator, and calls PrecheckInstance if a non-nil
Prechecker is returned.
Surely it doesn't call PrecheckInstance? :)

https://codereview.appspot.com/63790045/diff/1/state/policy.go#newcode80
state/policy.go:80: cfg, err := st.EnvironConfig()
No need to get config from state here, we've already got one.

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

https://codereview.appspot.com/63790045/diff/1/state/state.go#newcode266
state/state.go:266: newValidConfig, err := st.validateEnvironConfig(cfg, old)
Not quite right, I think.

What I think we should be doing here is computing the delta based on cfg-old. We
then validate this against the config in state and replace what's in state. This
must all be done atomically.

Otherwise, you might have two independently valid changes that are invalid when
combined.

We may also need a change to EnvironSet in the API server: it gets a config from
state and passes it into SetEnvironConfig. That should be part of the
transaction too (and all the validation bits in there can be removed, because
they'll be redundant).

[waigani, 2014-02-21 09:37:19]

Please take a look.

[axw, 2014-02-21 09:44:47]

On 2014/02/21 09:37:19, waigani wrote:
> Please take a look.

Implementations looks good, but please add some tests.

[waigani, 2014-02-23 05:04:45]

Please take a look.

[fwereade, 2014-02-27 13:36:33]

LGTM assuming there's a followup in which my comments are, or will be,
addressed. Progress not perfection :).

https://codereview.appspot.com/63790045/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/63790045/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:791: }
Shouldn't all this code above here move into state? If we're using oldConfig as
a base we need to assert that the base really is oldConfig, and handle
state-changed in here, and have a retry loop in here, etc.

https://codereview.appspot.com/63790045/diff/40001/state/policy.go
File state/policy.go (right):

https://codereview.appspot.com/63790045/diff/40001/state/policy.go#newcode81
state/policy.go:81: configValidator, err := st.policy.ConfigValidator(cfg)
if this just depends on type, can we make it explicit and pass in the type? I'm
just a bit twitchy about seeing cfg passed in twice.

https://codereview.appspot.com/63790045/diff/40001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/63790045/diff/40001/state/state.go#newcode261
state/state.go:261: func (st *State) SetEnvironConfig(cfg, old *config.Config)
error {
yeah, passing in old here really makes no sense. If we're applying a delta, it
should be a delta all the way; the old/new stuff is purely to validate that
delta.

If there's a followup addressing this then feel free to address this stuff
there, though, it's not like this implementation is any worse ;).

https://codereview.appspot.com/63790045/diff/40001/state/state.go#newcode283
state/state.go:283: settings.Delete(k)
I tried to resist the urge to say this again, but, gaah, this is all messed up.
This is not a comment on your branch.