Code review comment for lp://staging/~waigani/juju-core/validate-environ-config

Revision history for this message
Andrew Wilkins (axwalk) 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/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/62400043/diff/10001/state/apiserver/client/client.go#newcode787
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/keymanager.go
File state/apiserver/keymanager/keymanager.go (right):

https://codereview.appspot.com/62400043/diff/10001/state/apiserver/keymanager/keymanager.go#newcode143
state/apiserver/keymanager/keymanager.go:143: //Make new config by
applying config args to currentConfig and validating via environ
provider specific validator
ditto

https://codereview.appspot.com/62400043/

« Back to merge proposal