Merge lp:~fwereade/pyjuju/warn-ignored-constraints into lp:~fwereade/pyjuju/shadow-trunk-1204

[William Reade]

ConstraintSet.parse now logs on ignored constraints

https://codereview.appspot.com/5956044/

[William Reade (fwereade)]

Reviewers: mp+99861_code.launchpad.net,

Message:
Please take a look.

Description:
ConstraintSet.parse now logs on ignored constraints

https://code.launchpad.net/~fwereade/juju/warn-ignored-constraints/+merge/99861

Requires:
https://code.launchpad.net/~fwereade/juju/block-legacy-constraints-usage/+merge/99856

(do not edit description out of merge proposal)

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

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

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: juju/machine/constraints.py
=== modified file 'juju/machine/constraints.py'
--- juju/machine/constraints.py 2012-03-28 18:11:37 +0000
+++ juju/machine/constraints.py 2012-03-29 03:35:45 +0000
@@ -1,8 +1,11 @@
+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
@@ -165,6 +168,8 @@
                  name, value = s.split("=", 1)
                  constraint = self.get(name)
                  if constraint is None:
+ log.warn(
+ "ignored irrelevant %r constraint", name)
                      continue
                  if value == "any":
                      value = None

Index: juju/machine/tests/test_constraints.py
=== modified file 'juju/machine/tests/test_constraints.py'
--- juju/machine/tests/test_constraints.py 2012-03-28 14:40:14 +0000
+++ juju/machine/tests/test_constraints.py 2012-03-29 03:35:45 +0000
@@ -243,12 +243,13 @@
          self.assertTrue(c2.can_satisfy(c1))

      def test_unregistered_name(self):
- self.allow_names("foo", "blob")
+ self.allow_names("foo", "bar", "baz")
          cs = ConstraintSet("provider")
- cs.register("foo")
- c1 = cs.parse(["foo=1", "blob=2"])
- self.assertEquals(c1["foo"], "1")
- self.assertFalse("blob" in c1)
+ cs.register("bar")
+ output = self.capture_logging()
+ cs.parse(["foo=1", "bar=2", "baz=3"])
+ self.assertIn("ignored irrelevant 'foo' constraint",
output.getvalue())
+ self.assertIn("ignored irrelevant 'baz' constraint",
output.getvalue())

      def test_register_invisible(self):
          self.allow_names("foo")

[lp:~fwereade/pyjuju/warn-ignored-constraints]

517. By William Reade on 2012-03-29

merge parent

518. By William Reade on 2012-03-29

trivial test improvement

[William Reade (fwereade)]

Please take a look.

https://codereview.appspot.com/5956044/

[lp:~fwereade/pyjuju/warn-ignored-constraints]

519. By William Reade on 2012-03-29

merge parent

[Kapil Thangavelu (hazmat)]

one minor on distinguishing warning context, else LGTM

https://codereview.appspot.com/5956044/diff/3001/juju/machine/constraints.py
File juju/machine/constraints.py (right):

https://codereview.appspot.com/5956044/diff/3001/juju/machine/constraints.py#newcode172
juju/machine/constraints.py:172: "ignored irrelevant %r constraint",
name)
The distinction between irrelevant (not applicable to the current
provider) and invalid (non existent) seems a bit muddled here. afaics
this is passed values straight from the cli, so fat fingering a
instance-typo would be better to err, as opposed to setting deploying a
service with local provider and instance-type.

https://codereview.appspot.com/5956044/

[William Reade (fwereade)]

https://codereview.appspot.com/5956044/diff/3001/juju/machine/constraints.py
File juju/machine/constraints.py (right):

https://codereview.appspot.com/5956044/diff/3001/juju/machine/constraints.py#newcode172
juju/machine/constraints.py:172: "ignored irrelevant %r constraint",
name)
On 2012/03/29 19:59:55, hazmat wrote:
> The distinction between irrelevant (not applicable to the current
provider) and
> invalid (non existent) seems a bit muddled here. afaics this is passed
values
> straight from the cli, so fat fingering a instance-typo would be
better to err,
> as opposed to setting deploying a service with local provider and
instance-type.

"instance-type=m1.small" will warn in a non-ec2 provider;
"instance-typo=m1.small" will error in every provider.

https://codereview.appspot.com/5956044/

[William Reade (fwereade)]

https://codereview.appspot.com/5956044/diff/3001/juju/machine/constraints.py
File juju/machine/constraints.py (right):

https://codereview.appspot.com/5956044/diff/3001/juju/machine/constraints.py#newcode172
juju/machine/constraints.py:172: "ignored irrelevant %r constraint",
name)
On 2012/03/30 08:33:05, fwereade wrote:
> On 2012/03/29 19:59:55, hazmat wrote:
> > The distinction between irrelevant (not applicable to the current
provider)
> and
> > invalid (non existent) seems a bit muddled here. afaics this is
passed values
> > straight from the cli, so fat fingering a instance-typo would be
better to
> err,
> > as opposed to setting deploying a service with local provider and
> instance-type.
> >

> "instance-type=m1.small" will warn in a non-ec2 provider;
> "instance-typo=m1.small" will error in every provider.

(I added an explanatory comment all the same, it was clearly unclear as
it stood.)

https://codereview.appspot.com/5956044/

[lp:~fwereade/pyjuju/warn-ignored-constraints]

520. By William Reade on 2012-03-30

merge parent

521. By William Reade on 2012-03-30

merge parent

[William Reade (fwereade)]

*** Submitted:

ConstraintSet.parse now logs on ignored constraints

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

https://codereview.appspot.com/5956044/

=== modified file 'juju/machine/constraints.py'
--- juju/machine/constraints.py	2012-03-28 18:11:37 +0000
+++ juju/machine/constraints.py	2012-03-30 09:37:21 +0000
@@ -1,8 +1,11 @@
+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
@@ -165,6 +168,11 @@
                 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
                 if value == "any":
                     value = None

=== modified file 'juju/machine/tests/test_constraints.py'
--- juju/machine/tests/test_constraints.py	2012-03-28 14:40:14 +0000
+++ juju/machine/tests/test_constraints.py	2012-03-30 09:37:21 +0000
@@ -243,12 +243,14 @@
         self.assertTrue(c2.can_satisfy(c1))
 
     def test_unregistered_name(self):
-        self.allow_names("foo", "blob")
+        self.allow_names("foo", "bar", "baz")
         cs = ConstraintSet("provider")
-        cs.register("foo")
-        c1 = cs.parse(["foo=1", "blob=2"])
-        self.assertEquals(c1["foo"], "1")
-        self.assertFalse("blob" in c1)
+        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")
 
     def test_register_invisible(self):
         self.allow_names("foo")