Merge lp:~fwereade/pyjuju/constraints-compatibility into lp:pyjuju

[William Reade]

Forward-compatibility fixes for Constraints

* Any constraint name can now be registered; no more global registry
* Unrecognised constraints are now ignored but persisted when introduced via
  ConstraintSet.load
* Unrecognised traits are now always errors when introduced via
  ConstraintSet.parse

https://codereview.appspot.com/6001050/

[William Reade (fwereade)]

Reviewers: mp+101560_code.launchpad.net,

Message:
Please take a look.

Description:
Forward-compatibility fixes for Constraints

* Any constraint name can now be registered; no more global registry
* Unrecognised constraints are now ignored but persisted when introduced
via
   ConstraintSet.load
* Unrecognised traits are now always errors when introduced via
   ConstraintSet.parse

https://code.launchpad.net/~fwereade/juju/constraints-compatibility/+merge/101560

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6001050/

Affected files:
   A [revision details]
   M juju/machine/constraints.py
   M juju/machine/tests/test_constraints.py
   M juju/providers/common/tests/test_launch.py

[Kapil Thangavelu (hazmat)]

lgtm, thanks

https://codereview.appspot.com/6001050/

[William Reade (fwereade)]

*** Submitted:

Forward-compatibility fixes for Constraints

* Any constraint name can now be registered; no more global registry
* Unrecognised constraints are now ignored but persisted when introduced
via
   ConstraintSet.load
* Unrecognised traits are now always errors when introduced via
   ConstraintSet.parse

R=hazmat
CC=
https://codereview.appspot.com/6001050

https://codereview.appspot.com/6001050/

=== modified file 'juju/machine/constraints.py'
--- juju/machine/constraints.py	2012-03-30 08:41:02 +0000
+++ juju/machine/constraints.py	2012-04-11 14:08:26 +0000
@@ -1,25 +1,8 @@
-import logging
 import operator
 from UserDict import DictMixin
 
 from juju.errors import ConstraintError, UnknownConstraintError
 
-log = logging.getLogger("juju.machine.constraints")
-
-# To allow providers to construct ConstraintSets which silently ignore known-
-# but-inapplicable constraints (ec2-zone on orchestra, for example), we keep
-# a hardcoded global registry here. It will not be hard to remember to update
-# this when working with providers, because _ConstraintTypes with unknown
-# names cannot be created and will cause test failures so long as the added
-# constraint registration code is exercised in the provider tests.
-ALL_NAMES = (
-    "ubuntu-series", "provider-type",
-    "arch", "cpu", "mem", "instance-type",
-    "ec2-zone",
-    "maas-name",
-    "orchestra-classes",
-)
-
 
 def _dont_convert(s):
     return s
@@ -42,7 +25,6 @@
     """
 
     def __init__(self, name, default, converter, comparer, visible):
-        assert name in ALL_NAMES, "please update ALL_NAMES"
         self.name = name
         self.default = default
         self._converter = converter
@@ -150,15 +132,9 @@
     def get(self, name):
         """Get the (internal) _ConstraintType object corresponding to `name`.
 
-        If `name` is known, but not registered in this ConstraintSet, None will
-        be returned; if the constraint name is entirely unknown, an
-        UnknownConstraintError will be raised.
+        Returns None if no _ConstraintType has been registered under that name.
         """
-        try:
-            return self._registry[name]
-        except KeyError:
-            if name not in ALL_NAMES:
-                raise UnknownConstraintError(name)
+        return self._registry.get(name)
 
     def parse(self, strs):
         """Create a Constraints from strings (as used on the command line)"""
@@ -168,12 +144,7 @@
                 name, value = s.split("=", 1)
                 constraint = self.get(name)
                 if constraint is None:
-                    # A constraint called name does exist for some provider (if
-                    # it didn't exist anywhere, .get would have raised) but is
-                    # not relevant for this provider.
-                    log.warn(
-                        "ignored irrelevant %r constraint", name)
-                    continue
+                    raise UnknownConstraintError(name)
                 if value == "any":
                     value = None
                 if value == "":
@@ -204,7 +175,10 @@
     def load(self, data):
         """Convert a data dict to a Constraints"""
         for k, v in data.items():
-            self.get(k).convert(v)
+            constraint = self.get(k)
+            if constraint is not None:
+                # Include all of data; validate those parts we know how to.
+                constraint.convert(v)
         return Constraints(self, data)
 
 

=== modified file 'juju/machine/tests/test_constraints.py'
--- juju/machine/tests/test_constraints.py	2012-03-29 10:27:30 +0000
+++ juju/machine/tests/test_constraints.py	2012-04-11 14:08:26 +0000
@@ -27,6 +27,12 @@
 all_providers = ["dummy", "ec2", "orchestra"]
 
 
+def _raiser(exc_type):
+    def raise_(s):
+        raise exc_type(s)
+    return raise_
+
+
 class ConstraintsTestCase(TestCase):
 
     def assert_error(self, message, *raises_args):
@@ -219,48 +225,19 @@
 
 class ConstraintSetTest(TestCase):
 
-    def allow_names(self, *names):
-        all_names = ["ubuntu-series", "provider-type"]
-        all_names.extend(names)
-        from juju.machine import constraints
-        self.patch(constraints, "ALL_NAMES", all_names)
-
-    def test_register_unknown_name(self):
-        e = self.assertRaises(
-            AssertionError, ConstraintSet("provider").register, "nonsense")
-        self.assertEquals(str(e), "please update ALL_NAMES")
-
-    def test_register_known_names(self):
-        self.allow_names("foo", "blob")
-        cs = ConstraintSet("provider")
-        cs.register("foo")
-        cs.register("blob")
-        c1 = cs.parse(["foo=bar"]).with_series("series")
-        self.assertEquals(c1["foo"], "bar")
-        self.assertEquals(c1["blob"], None)
-        c2 = cs.parse(["foo=bar"]).with_series("series")
-        self.assertTrue(c1.can_satisfy(c2))
-        self.assertTrue(c2.can_satisfy(c1))
-
     def test_unregistered_name(self):
-        self.allow_names("foo", "bar", "baz")
         cs = ConstraintSet("provider")
         cs.register("bar")
-        output = self.capture_logging()
-        constraints = cs.parse(["foo=1", "bar=2", "baz=3"])
-        self.assertIn("ignored irrelevant 'foo' constraint", output.getvalue())
-        self.assertIn("ignored irrelevant 'baz' constraint", output.getvalue())
-        self.assertEquals(constraints["bar"], "2")
+        e = self.assertRaises(ConstraintError, cs.parse, ["bar=2", "baz=3"])
+        self.assertEquals(str(e), "Unknown constraint: 'baz'")
 
     def test_register_invisible(self):
-        self.allow_names("foo")
         cs = ConstraintSet("provider")
         cs.register("foo", visible=False)
         e = self.assertRaises(ConstraintError, cs.parse, ["foo=bar"])
         self.assertEquals(str(e), "Cannot set computed constraint: 'foo'")
 
     def test_register_comparer(self):
-        self.allow_names("foo")
         cs = ConstraintSet("provider")
         cs.register("foo", comparer=operator.ne)
         c1 = cs.parse(["foo=bar"]).with_series("series")
@@ -272,7 +249,6 @@
         self.assertTrue(c3.can_satisfy(c1))
 
     def test_register_default_and_converter(self):
-        self.allow_names("foo")
         cs = ConstraintSet("provider")
         cs.register("foo", default="star", converter=lambda s: "death-" + s)
         c1 = cs.parse([])
@@ -281,18 +257,13 @@
         self.assertEquals(c1["foo"], "death-clock")
 
     def test_convert_wraps_ValueError(self):
-        self.allow_names("foo", "bar")
-
-        def raiser(e):
-            raise e
         cs = ConstraintSet("provider")
-        cs.register("foo", converter=lambda s: raiser(ValueError(s)))
-        cs.register("bar", converter=lambda s: raiser(KeyError(s)))
+        cs.register("foo", converter=_raiser(ValueError))
+        cs.register("bar", converter=_raiser(KeyError))
         self.assertRaises(ConstraintError, cs.parse, ["foo=1"])
         self.assertRaises(KeyError, cs.parse, ["bar=1"])
 
     def test_register_conflicts(self):
-        self.allow_names("foo", "bar", "baz", "qux")
         cs = ConstraintSet("provider")
         cs.register("foo")
         cs.register("bar")
@@ -394,13 +365,14 @@
         self.assertEquals(c8["mem"], 512.0)
         self.assertEquals(c8["instance-type"], None)
 
-    def test_load_validates(self):
-        self.allow_names("foo")
+    def test_load_validates_known(self):
         cs = ConstraintSet("provider")
-
-        def blam(s):
-            raise ValueError(s)
-
-        cs.register("foo", converter=blam)
+        cs.register("foo", converter=_raiser(ValueError))
         e = self.assertRaises(ConstraintError, cs.load, {"foo": "bar"})
         self.assertEquals(str(e), "Bad 'foo' constraint 'bar': bar")
+
+    def test_load_preserves_unknown(self):
+        cs = ConstraintSet("provider")
+        constraints = cs.load({"foo": "bar"})
+        self.assertNotIn("foo", constraints)
+        self.assertEquals(constraints.data, {"foo": "bar"})

=== modified file 'juju/providers/common/tests/test_launch.py'
--- juju/providers/common/tests/test_launch.py	2012-03-27 22:46:44 +0000
+++ juju/providers/common/tests/test_launch.py	2012-04-11 14:08:26 +0000
@@ -4,6 +4,7 @@
 
 from juju.errors import EnvironmentNotFound
 from juju.lib.testing import TestCase
+from juju.machine.tests.test_constraints import dummy_cs
 from juju.providers.common.base import MachineProviderBase
 from juju.providers.common.launch import LaunchMachine
 from juju.providers.dummy import DummyMachine, FileStorage
@@ -22,6 +23,9 @@
         def get_file_storage(self):
             return self._file_storage
 
+        def get_constraint_set(self):
+            return succeed(dummy_cs)
+
         def get_zookeeper_machines(self):
             if zookeeper:
                 return succeed([zookeeper])