launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/juju-core/mpv-fwreade-12 into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

Commit message:
Address fwreade's review comment #12: Validate() should up-call to thumper's new global Validate().

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/juju-core/mpv-fwreade-12/+merge/158318

This addresses William's review comment for our feature branch:

«12) validCfg, err := prov.Validate(cfg, nil)

«thumper's just landing some changes around this area: there's another method you need to call in Validate, IIRC, that checks old/new configs match sanely (no changing env names, for example).»

That branch had not landed yet in the juju-code version we were working against, but it's a good thing this made me merge it: the provider API had changed again, requiring us to change StartInstance() and an internal helper.

The new function (not method as it turns out) that thumper created is called environs.config.Validate().


Jeroen
-- 
https://code.launchpad.net/~jtv/juju-core/mpv-fwreade-12/+merge/158318
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/juju-core/mpv-fwreade-12 into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

=== modified file 'environs/maas/config.go'
--- environs/maas/config.go	2013-02-06 11:31:27 +0000
+++ environs/maas/config.go	2013-04-11 10:15:39 +0000
@@ -46,6 +46,12 @@
 var errMalformedMaasOAuth = errors.New("malformed maas-oauth (3 items separated by colons)")
 
 func (prov maasEnvironProvider) Validate(cfg, oldCfg *config.Config) (*config.Config, error) {
+	// Validate base configuration change before validating MAAS specifics.
+	err := config.Validate(cfg, oldCfg)
+	if err != nil {
+		return nil, err
+	}
+
 	v, err := maasConfigChecker.Coerce(cfg.UnknownAttrs(), nil)
 	if err != nil {
 		return nil, err

=== modified file 'environs/maas/config_test.go'
--- environs/maas/config_test.go	2013-04-11 05:45:26 +0000
+++ environs/maas/config_test.go	2013-04-11 10:15:39 +0000
@@ -72,3 +72,23 @@
 	c.Assert(err, NotNil)
 	c.Check(err, ErrorMatches, ".*malformed maas-oauth.*")
 }
+
+func (ConfigSuite) TestValidateUpcallsEnvironsConfigValidate(c *C) {
+	// The base Validate() function will not allow an environment to
+	// change its name.  Trigger that error so as to prove that the
+	// environment provider's Validate() calls the base Validate().
+	baseAttrs := map[string]interface{}{
+		"maas-server": "http://maas.example.com/maas/";,
+		"maas-oauth":  "consumer-key:resource-token:resource-secret",
+	}
+	oldCfg, err := newConfig(baseAttrs)
+	c.Assert(err, IsNil)
+	newName := oldCfg.Name() + "-but-different"
+	newCfg, err := oldCfg.Apply(map[string]interface{}{"name": newName})
+	c.Assert(err, IsNil)
+
+	_, err = maasEnvironProvider{}.Validate(newCfg, oldCfg.Config)
+
+	c.Assert(err, NotNil)
+	c.Check(err, ErrorMatches, ".*cannot change name.*")
+}