|
|
|
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)
Patch Set 1 #
Total comments: 7
Patch Set 2 : State policy based config validation. #Patch Set 3 : State policy based config validation. #
Total comments: 4
Total messages: 6
|
waigani
Please take a look.
|
11 years, 10 months ago (2014年02月21日 03:42:06 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
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).
Please take a look.
On 2014年02月21日 09:37:19, waigani wrote: > Please take a look. Implementations looks good, but please add some tests.
Please take a look.
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.