Storage constraints can be omitted.

bnhassin · Jun 1, 2015 · 395b018 · 395b018
1 parent b592700
commit 395b018
Show file tree

Hide file tree

Showing 6 changed files with 153 additions and 38 deletions.
diff --git a/cmd/juju/storage/add.go b/cmd/juju/storage/add.go
@@ -10,25 +10,54 @@ import (
 	"github.com/juju/cmd"
 	"github.com/juju/errors"
 	"github.com/juju/names"
-	"github.com/juju/utils/keyvalues"
 
 	"github.com/juju/juju/apiserver/params"
 	"github.com/juju/juju/storage"
 )
 
 const (
 	addCommandDoc = `
-Add storage instances to a unit dynamically.
+Add storage instances to a unit dynamically using provided storage directives.
 Specify a unit and a storage specification in the same format 
 as passed to juju deploy --storage=”...”.
 
+A storage directive consists of a storage name as per charm specification
+and storage constraints, for e.g. pool, count, size.
+
+The acceptable format for storage constraints is a comma separated
+sequence of: POOL, COUNT, and SIZE, where
+
+    POOL identifies the storage pool. POOL can be a string
+    starting with a letter, followed by zero or more digits
+    or letters optionally separated by hyphens.
+
+    COUNT is a positive integer indicating how many instances
+    of the storage to create. If unspecified, and SIZE is
+    specified, COUNT defaults to 1.
+
+    SIZE describes the minimum size of the storage instances to
+    create. SIZE is a floating point number and multiplier from
+    the set (M, G, T, P, E, Z, Y), which are all treated as
+    powers of 1024.
+
+Storage constraints can be optionally ommitted.
+Environment default values will be used for all ommitted constraint values.
+There is no need to comma-separate ommitted constraints. 
+
 Example:
     juju storage add u/0 data=ebs,1024,3 
+    juju storage add u/0 data=ebs,1024,3, 
     Add 3 ebs storage instances for "data" storage to unit u/0. 
+    
+    juju storage add u/0 data=1 
+    juju storage add u/0 data 
+    Add 1 storage instances for "data" storage to unit u/0 
+        using default environment provider pool. 
 `
 	addCommandAgs = `
 <unit name> <storage directive> ...
-    where storage directive is <charm storage name=<storage constraints>
+    where storage directive is <charm storage name>=<storage constraints> or
+        <charm storage name>
 `
 )
 
@@ -54,20 +83,8 @@ func (c *AddCommand) Init(args []string) (err error) {
 	}
 	c.unitTag = names.NewUnitTag(u).String()
 
-	options, err := keyvalues.Parse(args[1:], true)
-	if err != nil {
-		return err
-	}
-	c.storageCons = make(map[string]storage.Constraints, len(options))
-	for key, value := range options {
-		cons, err := storage.ParseConstraints(value)
-		if err != nil {
-			return errors.Annotatef(err, "cannot parse constraints for storage %q", key)
-		}
-		c.storageCons[key] = cons
-	}
-
-	return nil
+	c.storageCons, err = storage.ParseStorageConstraints(args[1:])
+	return
 }
 
 // Info implements Command.Info.

diff --git a/cmd/juju/storage/add_test.go b/cmd/juju/storage/add_test.go
@@ -46,9 +46,8 @@ type tstData struct {
 var errorTsts = []tstData{
 	{nil, ".*storage add requires a unit and a storage directive.*"},
 	{[]string{"tst/123"}, ".*storage add requires a unit and a storage directive.*"},
-	{[]string{"tst/123", "data"}, `.*expected "key=value", got "data".*`},
-	{[]string{"tst/123", "data="}, `.*storage constraints require at least one field to be specified`},
 	{[]string{"tst/123", "data=-676"}, `.*count must be greater than zero, got "-676".*`},
+	{[]string{"tst/123", "data=676", "data=676"}, `.*storage "data" specified more than once.*`},
 }
 
 func (s *addSuite) TestAddArgs(c *gc.C) {
@@ -73,9 +72,18 @@ func (s *addSuite) TestAddInvalidUnit(c *gc.C) {
 	s.assertAddErrorOutput(c, `.*unit name "tst-123" not valid.*`)
 }
 
+var successTsts = []tstData{
+	{[]string{"tst/123", "data=676"}, ""},
+	{[]string{"tst/123", "data"}, ``},
+	{[]string{"tst/123", "data="}, ``},
+}
+
 func (s *addSuite) TestAddSuccess(c *gc.C) {
-	s.args = []string{"tst/123", "data=676"}
-	s.assertAddOutput(c, "", "")
+	for i, t := range successTsts {
+		c.Logf("test %d for %q", i, t.args)
+		s.args = t.args
+		s.assertAddOutput(c, "", "")
+	}
 }
 
 func (s *addSuite) TestAddOperationAborted(c *gc.C) {

diff --git a/storage/constraints.go b/storage/constraints.go
@@ -98,6 +98,46 @@ func IsValidPoolName(s string) bool {
 	return poolRE.MatchString(s)
 }
 
+// ParseStorageConstraints parses string representation of
+// storage constraints into a map keyed on storage names
+// with constraints as values.
+//
+// Storage constraints may be specified as
+//     <name>=<constraints>
+// or as
+//     <name>
+// where latter is equivalent to <name>=1.
+//
+// Duplicate storage names cause an error to be returned.
+func ParseStorageConstraints(args []string) (map[string]Constraints, error) {
+	results := make(map[string]Constraints, len(args))
+	for _, kv := range args {
+		parts := strings.SplitN(kv, "=", -1)
+		if len(parts) > 2 || len(parts[0]) == 0 {
+			return nil, errors.Errorf(`expected "name=constraints" or "name", got %q`, kv)
+		}
+
+		if _, exists := results[parts[0]]; exists {
+			return nil, errors.Errorf("storage %q specified more than once", parts[0])
+		}
+		if len(parts) == 1 {
+			// This will happen if only <name> is supplied.
+			parts = append(parts, "")
+		}
+		if len(parts[1]) == 0 {
+			//<name> is equivalent to <name>=1
+			parts[1] = "1"
+		}
+		cons, err := ParseConstraints(parts[1])
+		if err != nil {
+			return nil, errors.Annotatef(err, "cannot parse constraints for storage %q", parts[0])
+		}
+
+		results[parts[0]] = cons
+	}
+	return results, nil
+}
+
 func parseCount(s string) (uint64, bool, error) {
 	if !countRE.MatchString(s) {
 		return 0, false, nil

diff --git a/storage/constraints_test.go b/storage/constraints_test.go
@@ -103,3 +103,62 @@ func (s *ConstraintsSuite) TestInvalidPoolName(c *gc.C) {
 	c.Assert(storage.IsValidPoolName("-00l"), jc.IsFalse)
 	c.Assert(storage.IsValidPoolName("*00l"), jc.IsFalse)
 }
+
+func (s *ConstraintsSuite) TestParseStorageConstraints(c *gc.C) {
+	s.testParseStorageConstraints(c,
+		[]string{"data=p,1M,"},
+		map[string]storage.Constraints{"data": storage.Constraints{
+			Pool:  "p",
+			Count: 1,
+			Size:  1,
+		}})
+	s.testParseStorageConstraints(c,
+		[]string{"data"},
+		map[string]storage.Constraints{"data": storage.Constraints{
+			Count: 1,
+		}})
+	s.testParseStorageConstraints(c,
+		[]string{"data="},
+		map[string]storage.Constraints{"data": storage.Constraints{
+			Count: 1,
+		}})
+	s.testParseStorageConstraints(c,
+		[]string{"data=", "cache"},
+		map[string]storage.Constraints{
+			"data": storage.Constraints{
+				Count: 1,
+			},
+			"cache": storage.Constraints{
+				Count: 1,
+			},
+		})
+}
+
+func (s *ConstraintsSuite) TestParseStorageConstraintsErrors(c *gc.C) {
+	s.testStorageConstraintsError(c,
+		[]string{"data=p,=1M,"},
+		`.*expected "name=constraints" or "name", got .*`)
+	s.testStorageConstraintsError(c,
+		[]string{"data", "data"},
+		`storage "data" specified more than once`)
+	s.testStorageConstraintsError(c,
+		[]string{"data=-1"},
+		`.*cannot parse constraints for storage "data".*`)
+}
+
+func (*ConstraintsSuite) testParseStorageConstraints(c *gc.C,
+	s []string,
+	expect map[string]storage.Constraints,
+) {
+	cons, err := storage.ParseStorageConstraints(s)
+	c.Check(err, jc.ErrorIsNil)
+	c.Assert(len(cons), gc.Equals, len(expect))
+	for k, v := range expect {
+		c.Check(cons[k], gc.DeepEquals, v)
+	}
+}
+
+func (*ConstraintsSuite) testStorageConstraintsError(c *gc.C, s []string, e string) {
+	_, err := storage.ParseStorageConstraints(s)
+	c.Check(err, gc.ErrorMatches, e)
+}
diff --git a/worker/uniter/runner/jujuc/storage-add.go b/worker/uniter/runner/jujuc/storage-add.go
@@ -6,7 +6,6 @@ package jujuc
 import (
 	"github.com/juju/cmd"
 	"github.com/juju/errors"
-	"github.com/juju/utils/keyvalues"
 
 	"github.com/juju/juju/apiserver/params"
 	"github.com/juju/juju/storage"
@@ -45,11 +44,11 @@ sequence of: POOL, COUNT, and SIZE, where
     the set (M, G, T, P, E, Z, Y), which are all treated as
     powers of 1024.
 
-Storage constraints can be optionally ommitted as long as at least one 
-value is provided. 
+Storage constraints can be optionally ommitted.
 Environment default values will be used for all ommitted constraint values.
-There is no need to comma-separate ommitted constraints, 
-e.g. data=ebs,2, can be written as  data=ebs,2
+There is no need to comma-separate ommitted constraints, e.g. 
+    data=ebs,2,    <equivalent to>   data=ebs,2
+    data=1         <equivalent to>   data
 `
 
 func (s *StorageAddCommand) Info() *cmd.Info {
@@ -66,22 +65,14 @@ func (s *StorageAddCommand) Init(args []string) error {
 		return errors.New("storage add requires a storage directive")
 	}
 
-	// TODO (anastasiamac 2015-5-27) Bug 1459060:
-	//     For store names that already have constraints,
-	//     add support for "storage-add <name>".
-	//     This is equivalent to "storage-add <name>=1".
-	cons, err := keyvalues.Parse(args, true)
+	cons, err := storage.ParseStorageConstraints(args)
 	if err != nil {
 		return errors.Trace(err)
 	}
 
 	s.all = make(map[string]params.StorageConstraints, len(cons))
 	for k, v := range cons {
-		c, err := storage.ParseConstraints(v)
-		if err != nil {
-			return errors.Annotatef(err, "cannot parse constraints for %q", k)
-		}
-		s.all[k] = params.StorageConstraints{c.Pool, &c.Size, &c.Count}
+		s.all[k] = params.StorageConstraints{v.Pool, &v.Size, &v.Count}
 	}
 	return nil
 }

diff --git a/worker/uniter/runner/jujuc/storage-add_test.go b/worker/uniter/runner/jujuc/storage-add_test.go
@@ -56,8 +56,6 @@ type tstData struct {
 func (s *storageAddSuite) TestStorageAddInit(c *gc.C) {
 	tests := []tstData{
 		{[]string{}, 1, "storage add requires a storage directive"},
-		{[]string{"data"}, 1, `expected "key=value", got "data"`},
-		{[]string{"data="}, 1, ".*storage constraints require at least one.*"},
 		{[]string{"data=-676"}, 1, `.*cannot parse count: count must be gre.*`},
 	}
 	for i, t := range tests {
@@ -70,6 +68,8 @@ func (s *storageAddSuite) TestStorageAddInit(c *gc.C) {
 func (s *storageAddSuite) TestAddUnitStorage(c *gc.C) {
 	tests := []tstData{
 		{[]string{"data=676"}, 0, ""},
+		{[]string{"data"}, 0, ``},
+		{[]string{"data="}, 0, ""},
 	}
 
 	for i, t := range tests {