Explicitly errors on understood storage constraint

- Previously bad storage constraint resulted in debug output but successful deploy calls that look valid but fail in unexpected ways. This change explicitly fails on bad storage constraint arguments now. Fixes bug https://bugs.launchpad.net/juju/+bug/1855181
bnhassin · Dec 11, 2019 · da04aef · da04aef
1 parent 48b056d
commit da04aef
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 6 deletions.
diff --git a/storage/constraints.go b/storage/constraints.go
@@ -62,7 +62,7 @@ func ParseConstraints(s string) (Constraints, error) {
 		}
 		if IsValidPoolName(field) {
 			if cons.Pool != "" {
-				logger.Debugf("pool name is already set to %q, ignoring %q", cons.Pool, field)
+				return cons, errors.NotValidf("pool name is already set to %q, new value %q", cons.Pool, field)
 			} else {
 				cons.Pool = field
 			}
@@ -82,7 +82,7 @@ func ParseConstraints(s string) (Constraints, error) {
 			cons.Size = size
 			continue
 		}
-		logger.Debugf("ignoring unknown storage constraint %q", field)
+		return cons, errors.NotValidf("unrecognized storage constraint %q", field)
 	}
 	if cons.Count == 0 && cons.Size == 0 && cons.Pool == "" {
 		return Constraints{}, errors.New("storage constraints require at least one field to be specified")

diff --git a/storage/constraints_test.go b/storage/constraints_test.go
@@ -57,10 +57,6 @@ func (s *ConstraintsSuite) TestParseConstraintsOptions(c *gc.C) {
 		Count: 1,
 		Size:  1,
 	})
-	s.testParse(c, "p,anyoldjunk", storage.Constraints{
-		Pool:  "p",
-		Count: 1,
-	})
 }
 
 func (s *ConstraintsSuite) TestParseConstraintsCountRange(c *gc.C) {
@@ -75,6 +71,18 @@ func (s *ConstraintsSuite) TestParseConstraintsSizeRange(c *gc.C) {
 	s.testParseError(c, "p,-100M", `cannot parse size: expected a non-negative number, got "-100M"`)
 }
 
+func (s *ConstraintsSuite) TestParseMultiplePoolNames(c *gc.C) {
+	s.testParseError(c, "pool1,anyoldjunk", `pool name is already set to "pool1", new value "anyoldjunk" not valid`)
+	s.testParseError(c, "pool1,pool2", `pool name is already set to "pool1", new value "pool2" not valid`)
+	s.testParseError(c, "pool1,pool2,pool3", `pool name is already set to "pool1", new value "pool2" not valid`)
+}
+
+func (s *ConstraintsSuite) TestParseConstraintsUnknown(c *gc.C) {
+	// Regression test for #1855181
+	s.testParseError(c, "p,100M database-b", `unrecognized storage constraint "100M database-b" not valid`)
+	s.testParseError(c, "p,$1234", `unrecognized storage constraint "\$1234" not valid`)
+}
+
 func (*ConstraintsSuite) testParse(c *gc.C, s string, expect storage.Constraints) {
 	cons, err := storage.ParseConstraints(s)
 	c.Check(err, jc.ErrorIsNil)