Merge lp://staging/~waigani/juju-core/validate-environ-config into lp://staging/~go-bot/juju-core/trunk

Reviewers: mp+205880_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/62400043/

Affected files (+40, -28 lines):
   A [revision details]
   M cmd/juju/environment.go
   M cmd/juju/environment_test.go
   M environs/config.go
   M state/apiserver/client/client.go
   M state/apiserver/keymanager/keymanager.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140211064343-d5yn0b3om93nuecj
+New revision: <email address hidden>

Index: environs/config.go
=== modified file 'environs/config.go'
--- environs/config.go 2014-02-10 03:29:45 +0000
+++ environs/config.go 2014-02-12 21:01:39 +0000
@@ -262,3 +262,23 @@
   }
   return cfg, nil
  }
+
+//Make new config by applying config args to oldConfig and validating via
environ provider specific validator
+func ValidateConfig(oldConfig *config.Config, args map[string]interface{})
(newProviderConfig *config.Config, err error) {
+ // Apply the attributes specified for the command to the state config.
+ newConfig, err := oldConfig.Apply(args)
+ if err != nil {
+ return nil, err
+ }
+ env, err := New(oldConfig)
+ if err != nil {
+ return nil, err
+ }
+ // Now validate this new config against the existing config via the
provider.
+ provider := env.Provider()
+ newProviderConfig, err = provider.Validate(newConfig, oldConfig)
+ if err != nil {
+ return nil, err
+ }
+ return newProviderConfig, err
+}

Index: cmd/juju/environment.go
=== modified file 'cmd/juju/environment.go'
--- cmd/juju/environment.go 2013-12-17 18:21:26 +0000
+++ cmd/juju/environment.go 2014-02-12 01:16:10 +0000
@@ -10,6 +10,7 @@
   "launchpad.net/gnuflag"

   "launchpad.net/juju-core/cmd"
+ "launchpad.net/juju-core/environs"
   "launchpad.net/juju-core/juju"
   "launchpad.net/juju-core/state/api/params"
  )
@@ -169,17 +170,14 @@
   if err != nil {
    return err
   }
- // Apply the attributes specified for the command to the state config.
- newConfig, err := oldConfig.Apply(c.values)
- if err != nil {
- return err
- }
- // Now validate this new config against the existing config via the
provider.
- provider := conn.Environ.Provider()
- newProviderConfig, err := provider.Validate(newConfig, oldConfig)
- if err != nil {
- return err
- }
+
+ // Apply the attributes specified for the command to the state config and
+ // validate this new config against the existing config via the provider.
+ newProviderConfig, err := environs.ValidateConfig(oldConfig, c.values)
+ if err != nil {
+ return err
+ }
+
   // Now try to apply the new validated config.
   return conn.State.SetEnvironConfig(newProviderConfig, oldConfig)
  }

Index: cmd/juju/environment_test.go
=== ...

Please take a look.

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

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/

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/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

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.

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

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/

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
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/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
On 2014/02/13 08:59:42, axw wrote:
> ditto

Done.

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

WIPping this so we can extend axw's state Policy mechanism to get real environ-specific validation inside state.