launchpad-reviewers team mailing list archive

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

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

Commit message:
Address fwereade's review comment #15: validate config transition in SetConfig().

Requested reviews:
  MAAS Maintainers (maas-maintainers)

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

I don't see the other providers doing this yet, but since we were already halfway done implementing thumper's global Validate(), it makes sense to validate not just the new config that's being set but the resulting changes as well.

This small branch took surprising amounts of debugging.


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

=== modified file 'environs/maas/environ.go'
--- environs/maas/environ.go	2013-04-11 11:59:26 +0000
+++ environs/maas/environ.go	2013-04-12 03:41:23 +0000
@@ -238,14 +238,25 @@
 
 // SetConfig is specified in the Environ interface.
 func (env *maasEnviron) SetConfig(cfg *config.Config) error {
+	env.ecfgMutex.Lock()
+	defer env.ecfgMutex.Unlock()
+
+	// The new config has already been validated by itself, but now we
+	// validate the transition from the old config to the new.
+	var oldCfg *config.Config
+	if env.ecfgUnlocked != nil {
+		oldCfg = env.ecfgUnlocked.Config
+	}
+	cfg, err := env.Provider().Validate(cfg, oldCfg)
+	if err != nil {
+		return err
+	}
+
 	ecfg, err := env.Provider().(*maasEnvironProvider).newConfig(cfg)
 	if err != nil {
 		return err
 	}
 
-	env.ecfgMutex.Lock()
-	defer env.ecfgMutex.Unlock()
-
 	env.name = cfg.Name()
 	env.ecfgUnlocked = ecfg
 

=== modified file 'environs/maas/environ_test.go'
--- environs/maas/environ_test.go	2013-04-11 12:05:50 +0000
+++ environs/maas/environ_test.go	2013-04-12 03:41:23 +0000
@@ -71,20 +71,41 @@
 	envtesting.PutFakeTools(c, storage)
 }
 
+func (EnvironSuite) TestSetConfigValidatesFirst(c *C) {
+	// SetConfig() validates the config change and disallows, for example,
+	// changes in the environment name.
+	server := "http://maas.example.com";
+	oauth := "a:b:c"
+	secret := "pssst"
+	oldCfg := getTestConfig("old-name", server, oauth, secret)
+	newCfg := getTestConfig("new-name", server, oauth, secret)
+	env, err := NewEnviron(oldCfg)
+	c.Assert(err, IsNil)
+
+	// SetConfig() fails, even though both the old and the new config are
+	// individually.
+	err = env.SetConfig(newCfg)
+	c.Assert(err, NotNil)
+	c.Check(err, ErrorMatches, ".*cannot change name.*")
+
+	// The old config is still in place.  The new config never took effect.
+	c.Check(env.Name(), Equals, "old-name")
+}
+
 func (EnvironSuite) TestSetConfigUpdatesConfig(c *C) {
-	cfg := getTestConfig("test env", "http://maas2.example.com";, "a:b:c", "secret")
+	name := "test env"
+	cfg := getTestConfig(name, "http://maas2.example.com";, "a:b:c", "secret")
 	env, err := NewEnviron(cfg)
 	c.Check(err, IsNil)
 	c.Check(env.name, Equals, "test env")
 
-	anotherName := "another name"
 	anotherServer := "http://maas.example.com";
 	anotherOauth := "c:d:e"
 	anotherSecret := "secret2"
-	cfg2 := getTestConfig(anotherName, anotherServer, anotherOauth, anotherSecret)
+	cfg2 := getTestConfig(name, anotherServer, anotherOauth, anotherSecret)
 	errSetConfig := env.SetConfig(cfg2)
 	c.Check(errSetConfig, IsNil)
-	c.Check(env.name, Equals, anotherName)
+	c.Check(env.name, Equals, name)
 	authClient, _ := gomaasapi.NewAuthenticatedClient(anotherServer, anotherOauth, apiVersion)
 	maas := gomaasapi.NewMAAS(*authClient)
 	MAASServer := env.maasClientUnlocked