|
|
|
Fix lp:1168744 validate config
Add new function to environs, ValidateConfig, which makes a new config by applying config args to oldConfig and validating via environ provider specific validator.
https://code.launchpad.net/~waigani/juju-core/validate-environ-config/+merge/205880
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : Fix lp:1168744 validate config #
Total comments: 8
Patch Set 3 : Fix lp:1168744 validate config #
Total messages: 6
|
waigani
Please take a look.
|
11 years, 11 months ago (2014年02月12日 21:09:58 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
Please take a look.
https://codereview.appspot.com/62400043/diff/10001/environs/config.go File environs/config.go (right): https://codereview.appspot.com/62400043/diff/10001/environs/config.go#newcode267 environs/config.go:267: func ValidateConfig(oldConfig *config.Config, args map[string]interface{}) (newProviderConfig *config.Config, err error) { This doesn't look quite right to me. I think it should just have the same signature as EnvironProvider.Validate. https://codereview.appspot.com/62400043/diff/10001/environs/config.go#newcode273 environs/config.go:273: env, err := New(oldConfig) This is way overkill for getting a provider. Instead, just get the provider type from the config and use environs.Provider(). https://codereview.appspot.com/62400043/diff/10001/state/apiserver/client/cli... File state/apiserver/client/client.go (right): https://codereview.appspot.com/62400043/diff/10001/state/apiserver/client/cli... state/apiserver/client/client.go:787: //Make new config by applying config args to oldConfig and validating via environ provider specific validator I don't think this comment is worthwhile. It's just restating what ValidateConfig says it does. https://codereview.appspot.com/62400043/diff/10001/state/apiserver/keymanager... File state/apiserver/keymanager/keymanager.go (right): https://codereview.appspot.com/62400043/diff/10001/state/apiserver/keymanager... state/apiserver/keymanager/keymanager.go:143: //Make new config by applying config args to currentConfig and validating via environ provider specific validator ditto
On 2014年02月13日 08:59:41, axw wrote: > https://codereview.appspot.com/62400043/diff/10001/environs/config.go > File environs/config.go (right): > > https://codereview.appspot.com/62400043/diff/10001/environs/config.go#newcode267 > environs/config.go:267: func ValidateConfig(oldConfig *config.Config, args > map[string]interface{}) (newProviderConfig *config.Config, err error) { > This doesn't look quite right to me. I think it should just have the same > signature as EnvironProvider.Validate. > > https://codereview.appspot.com/62400043/diff/10001/environs/config.go#newcode273 > environs/config.go:273: env, err := New(oldConfig) > This is way overkill for getting a provider. Instead, just get the provider type > from the config and use environs.Provider(). > > https://codereview.appspot.com/62400043/diff/10001/state/apiserver/client/cli... > File state/apiserver/client/client.go (right): > > https://codereview.appspot.com/62400043/diff/10001/state/apiserver/client/cli... > state/apiserver/client/client.go:787: //Make new config by applying config args > to oldConfig and validating via environ provider specific validator > I don't think this comment is worthwhile. It's just restating what > ValidateConfig says it does. > > https://codereview.appspot.com/62400043/diff/10001/state/apiserver/keymanager... > File state/apiserver/keymanager/keymanager.go (right): > > https://codereview.appspot.com/62400043/diff/10001/state/apiserver/keymanager... > state/apiserver/keymanager/keymanager.go:143: //Make new config by applying > config args to currentConfig and validating via environ provider specific > validator > ditto I don't think this is quite the right approach: it's fundamentally racy, and that can't be fixed unless we do it inside state (well... we *could* have a retry loop at the API level I guess? but I think that would suck a bit). Can we chat about this live? Axw is doing some relevant work that would mesh nicely, I think; I'll be online my sunday night/your monday morning to chat to Tim.
Please take a look. https://codereview.appspot.com/62400043/diff/10001/environs/config.go File environs/config.go (right): https://codereview.appspot.com/62400043/diff/10001/environs/config.go#newcode267 environs/config.go:267: func ValidateConfig(oldConfig *config.Config, args map[string]interface{}) (newProviderConfig *config.Config, err error) { On 2014年02月13日 08:59:42, axw wrote: > This doesn't look quite right to me. I think it should just have the same > signature as EnvironProvider.Validate. Changed func name after discussing with Ian. https://codereview.appspot.com/62400043/diff/10001/environs/config.go#newcode273 environs/config.go:273: env, err := New(oldConfig) On 2014年02月13日 08:59:42, axw wrote: > This is way overkill for getting a provider. Instead, just get the provider type > from the config and use environs.Provider(). Done.
https://codereview.appspot.com/62400043/diff/10001/state/apiserver/client/cli... File state/apiserver/client/client.go (right): https://codereview.appspot.com/62400043/diff/10001/state/apiserver/client/cli... state/apiserver/client/client.go:787: //Make new config by applying config args to oldConfig and validating via environ provider specific validator On 2014年02月13日 08:59:42, axw wrote: > I don't think this comment is worthwhile. It's just restating what > ValidateConfig says it does. Done. https://codereview.appspot.com/62400043/diff/10001/state/apiserver/keymanager... File state/apiserver/keymanager/keymanager.go (right): https://codereview.appspot.com/62400043/diff/10001/state/apiserver/keymanager... state/apiserver/keymanager/keymanager.go:143: //Make new config by applying config args to currentConfig and validating via environ provider specific validator On 2014年02月13日 08:59:42, axw wrote: > ditto Done.