Param usage incl. provider type, further tests.

jangocheng · Jan 31, 2019 · 457e682 · 457e682
1 parent a1760ea
commit 457e682
Show file tree

Hide file tree

Showing 13 changed files with 157 additions and 47 deletions.
diff --git a/api/storage/client.go b/api/storage/client.go
@@ -95,17 +95,21 @@ func (c *Client) DeletePool(pname string) error {
 	if c.BestAPIVersion() < 5 {
 		return errors.New("deleting storage pools is not supported by this version of Juju")
 	}
-	return c.facade.FacadeCall("DeletePool", pname, nil)
+	args := params.StoragePoolDelete{
+		Name: pname,
+	}
+	return c.facade.FacadeCall("DeletePool", args, nil)
 }
 
 // UpdatePool updates a  pool with specified parameters.
-func (c *Client) UpdatePool(pname string, attrs map[string]interface{}) error {
+func (c *Client) UpdatePool(pname, provider string, attrs map[string]interface{}) error {
 	if c.BestAPIVersion() < 5 {
 		return errors.New("updating storage pools is not supported by this version of Juju")
 	}
 	args := params.StoragePool{
-		Name:  pname,
-		Attrs: attrs,
+		Name:     pname,
+		Provider: provider,
+		Attrs:    attrs,
 	}
 	return c.facade.FacadeCall("UpdatePool", args, nil)
 }

diff --git a/api/storage/client_test.go b/api/storage/client_test.go
@@ -863,13 +863,13 @@ func (s *storageMockSuite) TestDeletePool(c *gc.C) {
 			c.Check(id, gc.Equals, "")
 			c.Check(request, gc.Equals, "DeletePool")
 
-			args, ok := a.(string)
+			args, ok := a.(params.StoragePoolDelete)
 			c.Assert(ok, jc.IsTrue)
-			c.Assert(args, gc.Equals, poolName)
+			c.Assert(args.Name, gc.Equals, poolName)
 
 			return nil
 		})
-	storageClient := storage.NewClient(apiCaller)
+	storageClient := storage.NewClient(basetesting.BestVersionCaller{BestVersion: 5, APICallerFunc: apiCaller})
 	err := storageClient.DeletePool(poolName)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(called, jc.IsTrue)
@@ -889,14 +889,15 @@ func (s *storageMockSuite) TestDeletePoolFacadeCallError(c *gc.C) {
 
 			return errors.New(msg)
 		})
-	storageClient := storage.NewClient(apiCaller)
+	storageClient := storage.NewClient(basetesting.BestVersionCaller{BestVersion: 5, APICallerFunc: apiCaller})
 	err := storageClient.DeletePool("")
 	c.Assert(errors.Cause(err), gc.ErrorMatches, msg)
 }
 
 func (s *storageMockSuite) TestUpdatePool(c *gc.C) {
 	var called bool
 	poolName := "poolName"
+	providerType := "loop"
 	poolConfig := map[string]interface{}{
 		"test": "one",
 		"pass": true,
@@ -916,13 +917,13 @@ func (s *storageMockSuite) TestUpdatePool(c *gc.C) {
 			args, ok := a.(params.StoragePool)
 			c.Assert(ok, jc.IsTrue)
 			c.Assert(args.Name, gc.Equals, poolName)
-			c.Assert(args.Provider, gc.Equals, "")
+			c.Assert(args.Provider, gc.Equals, providerType)
 			c.Assert(args.Attrs, gc.DeepEquals, poolConfig)
 
 			return nil
 		})
-	storageClient := storage.NewClient(apiCaller)
-	err := storageClient.UpdatePool(poolName, poolConfig)
+	storageClient := storage.NewClient(basetesting.BestVersionCaller{BestVersion: 5, APICallerFunc: apiCaller})
+	err := storageClient.UpdatePool(poolName, providerType, poolConfig)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(called, jc.IsTrue)
 }
@@ -941,7 +942,7 @@ func (s *storageMockSuite) TestUpdatePoolFacadeCallError(c *gc.C) {
 
 			return errors.New(msg)
 		})
-	storageClient := storage.NewClient(apiCaller)
-	err := storageClient.UpdatePool("", nil)
+	storageClient := storage.NewClient(basetesting.BestVersionCaller{BestVersion: 5, APICallerFunc: apiCaller})
+	err := storageClient.UpdatePool("", "", nil)
 	c.Assert(errors.Cause(err), gc.ErrorMatches, msg)
 }
diff --git a/apiserver/facades/client/storage/pooldelete_test.go b/apiserver/facades/client/storage/pooldelete_test.go
@@ -9,6 +9,7 @@ import (
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
+	"github.com/juju/juju/apiserver/params"
 	"github.com/juju/juju/storage"
 	"github.com/juju/juju/storage/provider"
 )
@@ -33,7 +34,9 @@ func (s *poolDeleteSuite) TestDeletePool(c *gc.C) {
 	s.createPools(c, 1)
 	poolName := fmt.Sprintf("%v%v", tstName, 0)
 
-	err := s.api.DeletePool(poolName)
+	err := s.api.DeletePool(params.StoragePoolDelete{
+		Name: poolName,
+	})
 	c.Assert(err, jc.ErrorIsNil)
 
 	pools, err := s.poolManager.List()
@@ -44,7 +47,9 @@ func (s *poolDeleteSuite) TestDeletePool(c *gc.C) {
 func (s *poolDeleteSuite) TestDeleteNotExists(c *gc.C) {
 	poolName := fmt.Sprintf("%v%v", tstName, 0)
 
-	err := s.api.DeletePool(poolName)
+	err := s.api.DeletePool(params.StoragePoolDelete{
+		Name: poolName,
+	})
 	c.Assert(err, jc.ErrorIsNil)
 
 	pools, err := s.poolManager.List()

diff --git a/apiserver/facades/client/storage/storage.go b/apiserver/facades/client/storage/storage.go
@@ -458,6 +458,8 @@ func (a *StorageAPI) validateProviderCriteria(providers []string) error {
 
 // CreatePool creates a new pool with specified parameters.
 func (a *StorageAPI) CreatePool(p params.StoragePool) error {
+	// (veebers) TODO: This should be a bulk call. Observed during addition
+	// of update/delete, put off for a follow up fix.
 	_, err := a.poolManager.Create(
 		p.Name,
 		storage.ProviderType(p.Provider),
@@ -1123,11 +1125,11 @@ func (a *StorageAPI) importFilesystem(
 }
 
 // DeletePool deletes the named pool
-func (a *StorageAPI) DeletePool(poolName string) error {
+func (a *StorageAPI) DeletePool(p params.StoragePoolDelete) error {
 	if err := a.checkCanWrite(); err != nil {
 		return errors.Trace(err)
 	}
-	err := a.poolManager.Delete(poolName)
+	err := a.poolManager.Delete(p.Name)
 	return err
 }
 
@@ -1136,7 +1138,7 @@ func (a *StorageAPI) UpdatePool(p params.StoragePool) error {
 	if err := a.checkCanWrite(); err != nil {
 		return errors.Trace(err)
 	}
-	err := a.poolManager.Replace(p.Name, p.Attrs)
+	err := a.poolManager.Replace(p.Name, p.Provider, p.Attrs)
 	return err
 }
 

diff --git a/apiserver/params/storage.go b/apiserver/params/storage.go
@@ -585,6 +585,11 @@ type StoragePool struct {
 	Attrs map[string]interface{} `json:"attrs"`
 }
 
+// StoragePoolDelete holds data for a pool instance to be deleted
+type StoragePoolDelete struct {
+	Name string `json:"name"`
+}
+
 // StoragePoolFilter holds a filter for matching storage pools.
 type StoragePoolFilter struct {
 	// Names are pool's names to filter on.

diff --git a/cmd/juju/commands/main.go b/cmd/juju/commands/main.go
@@ -414,6 +414,8 @@ func registerCommands(r commandRegistry, ctx *cmd.Context) {
 	r.Register(storage.NewListCommand())
 	r.Register(storage.NewPoolCreateCommand())
 	r.Register(storage.NewPoolListCommand())
+	r.Register(storage.NewPoolDeleteCommand())
+	r.Register(storage.NewPoolUpdateCommand())
 	r.Register(storage.NewShowCommand())
 	r.Register(storage.NewRemoveStorageCommandWithAPI())
 	r.Register(storage.NewDetachStorageCommandWithAPI())

diff --git a/cmd/juju/commands/main_test.go b/cmd/juju/commands/main_test.go
@@ -454,6 +454,7 @@ var commandNames = []string{
 	"destroy-controller",
 	"destroy-model",
 	"detach-storage",
+	"delete-storage-pool",
 	"diff-bundle",
 	"disable-command",
 	"disable-user",
@@ -589,6 +590,7 @@ var commandNames = []string{
 	"unregister",
 	"update-clouds",
 	"update-credential",
+	"update-storage-pool",
 	"upgrade-charm",
 	"upgrade-gui",
 	"upgrade-juju",

diff --git a/cmd/juju/storage/poolupdate.go b/cmd/juju/storage/poolupdate.go
@@ -12,10 +12,14 @@ import (
 	"github.com/juju/juju/cmd/modelcmd"
 )
 
+const (
+	Type = "type"
+)
+
 // PoolUpdateAPI defines the API methods that the storage commands use.
 type PoolUpdateAPI interface {
 	Close() error
-	UpdatePool(name string, attr map[string]interface{}) error
+	UpdatePool(name, provider string, attr map[string]interface{}) error
 	BestAPIVersion() int
 }
 
@@ -51,6 +55,7 @@ type poolUpdateCommand struct {
 	newAPIFunc  func() (PoolUpdateAPI, error)
 	poolName    string
 	configAttrs map[string]interface{}
+	provider    string
 }
 
 // Init implements Command.Init.
@@ -65,10 +70,14 @@ func (c *poolUpdateCommand) Init(args []string) (err error) {
 	if err != nil {
 		return err
 	}
-	if len(config) == 0 {
-		return errors.New("pool update requires configuration attributes")
-	}
+	// if len(config) == 0 {
+	// 	return errors.New("pool update requires configuration attributes")
+	// }
 
+	if providerType, ok := config[Type]; ok {
+		delete(config, Type)
+		c.provider = providerType
+	}
 	c.configAttrs = make(map[string]interface{})
 	for key, value := range config {
 		c.configAttrs[key] = value
@@ -94,9 +103,9 @@ func (c *poolUpdateCommand) Run(ctx *cmd.Context) (err error) {
 	}
 	defer api.Close()
 	if api.BestAPIVersion() < 5 {
-		return errors.New("updating storage pools is not supported by this API server")
+		return errors.New("updating storage pools is not supported by this version of Juju")
 	}
-	err = api.UpdatePool(c.poolName, c.configAttrs)
+	err = api.UpdatePool(c.poolName, c.provider, c.configAttrs)
 	if err != nil {
 		return err
 	}

diff --git a/cmd/juju/storage/poolupdate_test.go b/cmd/juju/storage/poolupdate_test.go
@@ -33,59 +33,88 @@ func (s *PoolUpdateSuite) runPoolUpdate(c *gc.C, args []string) (*cmd.Context, e
 func (s *PoolUpdateSuite) TestPoolUpdateOneArg(c *gc.C) {
 	_, err := s.runPoolUpdate(c, []string{"sunshine"})
 	c.Check(err, gc.ErrorMatches, "pool update requires name and configuration attributes")
+	c.Assert(len(s.mockAPI.Updates), gc.Equals, 0)
 }
 
 func (s *PoolUpdateSuite) TestPoolUpdateNoArgs(c *gc.C) {
 	_, err := s.runPoolUpdate(c, []string{""})
 	c.Check(err, gc.ErrorMatches, "pool update requires name and configuration attributes")
+	c.Assert(len(s.mockAPI.Updates), gc.Equals, 0)
 }
 
 func (s *PoolUpdateSuite) TestPoolUpdateWithAttrArgs(c *gc.C) {
 	_, err := s.runPoolUpdate(c, []string{"sunshine", "lollypop=true"})
 	c.Check(err, jc.ErrorIsNil)
+	c.Assert(len(s.mockAPI.Updates), gc.Equals, 1)
+	updatedConfigs := s.mockAPI.Updates[0]
+	c.Assert(updatedConfigs.Name, gc.Equals, "sunshine")
+	c.Assert(updatedConfigs.Config, gc.DeepEquals, map[string]interface{}{"lollypop": "true"})
 }
 
 func (s *PoolUpdateSuite) TestPoolUpdateAttrMissingKey(c *gc.C) {
 	_, err := s.runPoolUpdate(c, []string{"sunshine", "=too"})
 	c.Check(err, gc.ErrorMatches, `expected "key=value", got "=too"`)
+	c.Assert(len(s.mockAPI.Updates), gc.Equals, 0)
 }
 
 func (s *PoolUpdateSuite) TestPoolUpdateAttrMissingValue(c *gc.C) {
 	_, err := s.runPoolUpdate(c, []string{"sunshine", "something="})
 	c.Check(err, gc.ErrorMatches, `expected "key=value", got "something="`)
+	c.Assert(len(s.mockAPI.Updates), gc.Equals, 0)
 }
 
 func (s *PoolUpdateSuite) TestPoolUpdateAttrEmptyValue(c *gc.C) {
 	_, err := s.runPoolUpdate(c, []string{"sunshine", `something=""`})
 	c.Check(err, jc.ErrorIsNil)
+	c.Assert(len(s.mockAPI.Updates), gc.Equals, 1)
+	updatedConfigs := s.mockAPI.Updates[0]
+	c.Assert(updatedConfigs.Name, gc.Equals, "sunshine")
+	c.Assert(updatedConfigs.Config, gc.DeepEquals, map[string]interface{}{"something": "\"\""})
 }
 
 func (s *PoolUpdateSuite) TestPoolUpdateOneAttr(c *gc.C) {
 	_, err := s.runPoolUpdate(c, []string{"sunshine", "something=too"})
 	c.Check(err, jc.ErrorIsNil)
+	c.Assert(len(s.mockAPI.Updates), gc.Equals, 1)
+	updatedConfigs := s.mockAPI.Updates[0]
+	c.Assert(updatedConfigs.Name, gc.Equals, "sunshine")
+	c.Assert(updatedConfigs.Config, gc.DeepEquals, map[string]interface{}{"something": "too"})
 }
 
 func (s *PoolUpdateSuite) TestPoolUpdateEmptyAttr(c *gc.C) {
 	_, err := s.runPoolUpdate(c, []string{"sunshine", ""})
 	c.Check(err, gc.ErrorMatches, `expected "key=value", got ""`)
+	c.Assert(len(s.mockAPI.Updates), gc.Equals, 0)
 }
 
 func (s *PoolUpdateSuite) TestPoolUpdateManyAttrs(c *gc.C) {
 	_, err := s.runPoolUpdate(c, []string{"sunshine", "something=too", "another=one"})
 	c.Check(err, jc.ErrorIsNil)
+	c.Assert(len(s.mockAPI.Updates), gc.Equals, 1)
+	updatedConfigs := s.mockAPI.Updates[0]
+	c.Assert(updatedConfigs.Name, gc.Equals, "sunshine")
+	c.Assert(updatedConfigs.Config, gc.DeepEquals, map[string]interface{}{"something": "too", "another": "one"})
 }
 
 func (s *PoolUpdateSuite) TestPoolUpdateUnsupportedAPIVersion(c *gc.C) {
 	s.mockAPI.APIVersion = 3
 	_, err := s.runPoolUpdate(c, []string{"sunshine", "something=too", "another=one"})
-	c.Check(err, gc.ErrorMatches, "updating storage pools is not supported by this API server")
+	c.Check(err, gc.ErrorMatches, "updating storage pools is not supported by this version of Juju")
+	c.Assert(len(s.mockAPI.Updates), gc.Equals, 0)
+}
+
+type mockUpdateData struct {
+	Name   string
+	Config map[string]interface{}
 }
 
 type mockPoolUpdateAPI struct {
 	APIVersion int
+	Updates    []mockUpdateData
 }
 
-func (s mockPoolUpdateAPI) UpdatePool(pname string, pconfig map[string]interface{}) error {
+func (s *mockPoolUpdateAPI) UpdatePool(pname string, pconfig map[string]interface{}) error {
+	s.Updates = append(s.Updates, mockUpdateData{Name: pname, Config: pconfig})
 	return nil
 }
 

diff --git a/featuretests/storage_test.go b/featuretests/storage_test.go
@@ -697,3 +697,65 @@ storage-block/1  data/1      block                       pending
 
 `[1:])
 }
+
+func runPoolUpdate(c *gc.C, args ...string) (string, string, error) {
+	cmdArgs := append([]string{"update-storage-pool"}, args...)
+	ctx, err := runJujuCommand(c, cmdArgs...)
+	stdout, stderr := "", ""
+	if ctx != nil {
+		stdout = cmdtesting.Stdout(ctx)
+		stderr = cmdtesting.Stderr(ctx)
+	}
+	return stdout, stderr, err
+
+}
+
+func (s *cmdStorageSuite) TestUpdate(c *gc.C) {
+	stdout, _, err := runPoolUpdate(c, testPool, "smth=one")
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(stdout, gc.Equals, "")
+	assertPoolExists(c, s.State, testPool, "loop", "smth=one")
+}
+
+func (s *cmdStorageSuite) TestUpdateNoMatch(c *gc.C) {
+	_, stderr, err := runPoolUpdate(c, "nope", "smth=one")
+	c.Assert(err, gc.NotNil)
+	c.Assert(stderr, gc.Equals, "ERROR pool \"nope\" not found (not found)\n")
+	assertPoolExists(c, s.State, testPool, "loop", "it=works")
+}
+
+func runPoolDelete(c *gc.C, args ...string) (string, string, error) {
+	cmdArgs := append([]string{"delete-storage-pool"}, args...)
+	ctx, err := runJujuCommand(c, cmdArgs...)
+	stdout, stderr := "", ""
+	if ctx != nil {
+		stdout = cmdtesting.Stdout(ctx)
+		stderr = cmdtesting.Stderr(ctx)
+	}
+	return stdout, stderr, err
+
+}
+
+func (s *cmdStorageSuite) TestDelete(c *gc.C) {
+	assertPoolExists(c, s.State, testPool, "loop", "it=works")
+	stdout, _, err := runPoolDelete(c, testPool)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(stdout, gc.Equals, "")
+
+	stsetts := state.NewStateSettings(s.State)
+	poolManager := poolmanager.New(stsetts, storage.ChainedProviderRegistry{
+		dummy.StorageProviders(),
+		provider.CommonStorageProviders(),
+	})
+
+	found, err := poolManager.List()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(len(found), gc.Equals, 0)
+}
+
+func (s *cmdStorageSuite) TestDeleteNoMatch(c *gc.C) {
+	_, stderr, err := runPoolUpdate(c, "nope", "smth=one")
+	c.Assert(err, gc.NotNil)
+	c.Assert(stderr, gc.Equals, "ERROR pool \"nope\" not found (not found)\n")
+	assertPoolExists(c, s.State, testPool, "loop", "it=works")
+}