Fix replace implementation, add needed tests.

jangocheng · Jan 30, 2019 · cf7be82 · cf7be82
1 parent 2029aa2
commit cf7be82
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 11 deletions.
diff --git a/state/settings.go b/state/settings.go
@@ -373,7 +373,7 @@ func removeSettingsOp(collection, key string) txn.Op {
 	}
 }
 
-// replaceSettings replaces the settings value for key.
+// replaceSettings replaces the settings values for key.
 func replaceSettings(db Database, collection, key string, values map[string]interface{}) error {
 	op, _, err := replaceSettingsOp(db, collection, key, values)
 	if err != nil {

diff --git a/storage/poolmanager/poolmanager.go b/storage/poolmanager/poolmanager.go
@@ -48,6 +48,21 @@ func (pm *poolManager) Create(name string, providerType storage.ProviderType, at
 		return nil, MissingTypeError
 	}
 
+	cfg, err := pm.validatedConfig(name, providerType, attrs)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+
+	poolAttrs := cfg.Attrs()
+	poolAttrs[Name] = name
+	poolAttrs[Type] = string(providerType)
+	if err := pm.settings.CreateSettings(globalKey(name), poolAttrs); err != nil {
+		return nil, errors.Annotatef(err, "creating pool %q", name)
+	}
+	return cfg, nil
+}
+
+func (pm *poolManager) validatedConfig(name string, providerType storage.ProviderType, attrs map[string]interface{}) (*storage.Config, error) {
 	cfg, err := storage.NewConfig(name, providerType, attrs)
 	if err != nil {
 		return nil, errors.Trace(err)
@@ -60,12 +75,6 @@ func (pm *poolManager) Create(name string, providerType storage.ProviderType, at
 		return nil, errors.Annotate(err, "validating storage provider config")
 	}
 
-	poolAttrs := cfg.Attrs()
-	poolAttrs[Name] = name
-	poolAttrs[Type] = string(providerType)
-	if err := pm.settings.CreateSettings(globalKey(name), poolAttrs); err != nil {
-		return nil, errors.Annotatef(err, "creating pool %q", name)
-	}
 	return cfg, nil
 }
 
@@ -83,6 +92,30 @@ func (pm *poolManager) Replace(name string, attrs map[string]interface{}) error
 	if name == "" {
 		return MissingNameError
 	}
+	var providerType storage.ProviderType
+	if newProviderType, ok := attrs[Type]; ok {
+		switch newProviderType.(type) {
+		case string:
+			providerType = storage.ProviderType(newProviderType.(string))
+		default:
+			return errors.New("provider type must be a string")
+		}
+	} else {
+		existingConfig, err := pm.Get(name)
+		if err != nil {
+			return errors.Trace(err)
+		}
+		providerType = existingConfig.Provider()
+	}
+	attrs[Type] = providerType
+	attrs[Name] = name
+	cfg, err := pm.validatedConfig(name, providerType, attrs)
+	if err != nil {
+		return errors.Trace(err)
+	}
+	validatedAttrs := cfg.Attrs()
+	validatedAttrs[Name] = name
+	validatedAttrs[Type] = string(providerType)
 	return pm.settings.ReplaceSettings(globalKey(name), attrs)
 }
 

diff --git a/storage/poolmanager/poolmanager_test.go b/storage/poolmanager/poolmanager_test.go
@@ -26,7 +26,7 @@ type poolSuite struct {
 var _ = gc.Suite(&poolSuite{})
 
 var poolAttrs = map[string]interface{}{
-	"name": "testpool", "type": "loop", "foo": "bar",
+	"name": "testpool", "type": "loop", "foo": "bar", "bleep": "bloop",
 }
 
 func (s *poolSuite) SetUpTest(c *gc.C) {
@@ -53,7 +53,7 @@ func (s *poolSuite) TestList(c *gc.C) {
 	pools, err := s.poolManager.List()
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(pools, gc.HasLen, 1)
-	c.Assert(pools[0].Attrs(), gc.DeepEquals, map[string]interface{}{"foo": "bar"})
+	c.Assert(pools[0].Attrs(), gc.DeepEquals, map[string]interface{}{"foo": "bar", "bleep": "bloop"})
 	c.Assert(pools[0].Name(), gc.Equals, "testpool")
 	c.Assert(pools[0].Provider(), gc.Equals, storage.ProviderType("loop"))
 }
@@ -72,7 +72,7 @@ func (s *poolSuite) TestListManyResults(c *gc.C) {
 		poolCfgs[p.Name()] = p.Attrs()
 	}
 	c.Assert(poolCfgs, jc.DeepEquals, map[string]map[string]interface{}{
-		"testpool":  {"foo": "bar"},
+		"testpool":  {"foo": "bar", "bleep": "bloop"},
 		"testpool2": {"foo2": "bar2"},
 	})
 }
@@ -87,7 +87,7 @@ func (s *poolSuite) TestPool(c *gc.C) {
 	s.createSettings(c)
 	p, err := s.poolManager.Get("testpool")
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(p.Attrs(), gc.DeepEquals, map[string]interface{}{"foo": "bar"})
+	c.Assert(p.Attrs(), gc.DeepEquals, map[string]interface{}{"foo": "bar", "bleep": "bloop"})
 	c.Assert(p.Name(), gc.Equals, "testpool")
 	c.Assert(p.Provider(), gc.Equals, storage.ProviderType("loop"))
 }
@@ -130,6 +130,57 @@ func (s *poolSuite) TestCreateInvalidConfig(c *gc.C) {
 	c.Assert(err, gc.ErrorMatches, "validating storage provider config: no good")
 }
 
+func (s *poolSuite) TestReplace(c *gc.C) {
+	s.createSettings(c)
+	err := s.poolManager.Replace("testpool", map[string]interface{}{"zip": "zap"})
+	c.Assert(err, jc.ErrorIsNil)
+	p, err := s.poolManager.Get("testpool")
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(p.Attrs(), gc.DeepEquals, map[string]interface{}{"zip": "zap"})
+	c.Assert(p.Name(), gc.Equals, "testpool")
+	c.Assert(p.Provider(), gc.Equals, storage.ProviderType("loop"))
+}
+
+func (s *poolSuite) TestReplaceMissingName(c *gc.C) {
+	err := s.poolManager.Replace("", map[string]interface{}{"foo": "bar"})
+	c.Assert(err, gc.ErrorMatches, "pool name is missing")
+}
+
+func (s *poolSuite) TestReplaceNewProvider(c *gc.C) {
+	s.registry.Providers["notebook"] = &dummystorage.StorageProvider{}
+	s.createSettings(c)
+	err := s.poolManager.Replace("testpool", map[string]interface{}{"handwritten": "true", "type": "notebook"})
+	c.Assert(err, jc.ErrorIsNil)
+	p, err := s.poolManager.Get("testpool")
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(p.Attrs(), gc.DeepEquals, map[string]interface{}{"handwritten": "true"})
+	c.Assert(p.Name(), gc.Equals, "testpool")
+	c.Assert(p.Provider(), gc.Equals, storage.ProviderType("notebook"))
+}
+
+func (s *poolSuite) TestReplaceNewProviderIncorrect(c *gc.C) {
+	s.registry.Providers["notebook"] = &dummystorage.StorageProvider{}
+	s.createSettings(c)
+	err := s.poolManager.Replace("testpool", map[string]interface{}{"handwritten": "true", "type": 1})
+	c.Assert(err, gc.ErrorMatches, "provider type must be a string")
+}
+
+func (s *poolSuite) TestReplaceInvalidConfig(c *gc.C) {
+	s.registry.Providers["invalid"] = &dummystorage.StorageProvider{
+		ValidateConfigFunc: func(*storage.Config) error {
+			return errors.New("no good")
+		},
+	}
+	s.createSettings(c)
+	err := s.poolManager.Replace("testpool", map[string]interface{}{"zip": "zap", "type": "invalid"})
+	c.Assert(err, gc.ErrorMatches, "validating storage provider config: no good")
+}
+
+func (s *poolSuite) TestReplaceNotFound(c *gc.C) {
+	err := s.poolManager.Replace("deadpool", map[string]interface{}{"zip": "zap"})
+	c.Assert(err, gc.ErrorMatches, "pool \"deadpool\" not found")
+}
+
 func (s *poolSuite) TestDelete(c *gc.C) {
 	s.createSettings(c)
 	err := s.poolManager.Delete("testpool")