Merge lp:~jimbaker/pyjuju/must-specify-relation-name into lp:pyjuju

[Jim Baker]

relation-ids should default to $JUJU_RELATION

If no relation name is specified for the relation-ids hook command, it should default to $JUJU_RELATION. If that's not available, it should raise an error stating "Relation name must be specified".

https://codereview.appspot.com/5991058/

[Jim Baker (jimbaker)]

Reviewers: mp+101156_code.launchpad.net,

Message:
Please take a look.

Description:
relation-ids should default to $JUJU_RELATION

If no relation name is specified for the relation-ids hook command, it
should default to $JUJU_RELATION. If that's not available, it should
raise an error stating "Relation name must be specified".

https://code.launchpad.net/~jimbaker/juju/must-specify-relation-name/+merge/101156

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M juju/hooks/commands.py
   M juju/hooks/protocol.py
   M juju/hooks/tests/test_communications.py
   M juju/hooks/tests/test_invoker.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/hooks/commands.py
=== modified file 'juju/hooks/commands.py'
--- juju/hooks/commands.py 2012-04-05 00:58:19 +0000
+++ juju/hooks/commands.py 2012-04-06 20:29:41 +0000
@@ -98,12 +98,15 @@
      keyvalue_pairs = False

      def customize_parser(self):
+ relation_name = os.environ.get("JUJU_RELATION", "")
+
          self.parser.add_argument(
              "relation_name",
              metavar="RELATION NAME",
              nargs="?",
- default="",
- help="Specify the relation name of the relation ids to list.")
+ default=relation_name,
+ help=("Specify the relation name of the relation ids to list. "
+ "Defaults to $JUJU_RELATION, if available."))

      def run(self):
          return self.client.relation_ids(

Index: juju/hooks/protocol.py
=== modified file 'juju/hooks/protocol.py'
--- juju/hooks/protocol.py 2012-04-05 16:38:15 +0000
+++ juju/hooks/protocol.py 2012-04-06 20:29:41 +0000
@@ -73,11 +73,19 @@
      """

+class MustSpecifyRelationName(JujuError):
+ """No relation name was specified."""
+
+ def __str__(self):
+ return "Relation name must be specified"
+
+
  class BaseCommand(amp.Command):
      errors = {NoSuchUnit: "NoSuchUnit",
                NoSuchKey: "NoSuchKey",
                NotRelationContext: "NotRelationContext",
- UnitRelationStateNotFound: "UnitRelationStateNotFound"}
+ UnitRelationStateNotFound: "UnitRelationStateNotFound",
+ MustSpecifyRelationName: "MustSpecifyRelationName"}

  # All the commands below this point should be documented in the
@@ -243,9 +251,15 @@
      def relation_ids(self, client_id, relation_name):
          """Set values into state.hook.RelationHookContext.

+ :client_id: hooks client id that is used to define a context
+ for a consistent view of state.
+
          :param relation_name: The relation name to query relation ids
- for this context.
+ for this context. If no such relation name is specified,
+ raises `MustSpecifyRelationName`.
          """
+ if not relation_name:
+ raise MustS...

[Kapil Thangavelu (hazmat)]

This is almost a trivial branch.. but.. its a bit odd that this bothers
to do a round trip to the server just to get an error that the client
could trivially check for before every doing the rpc, please add that
check.

https://codereview.appspot.com/5991058/

[Jim Baker (jimbaker)]

Please take a look.

https://codereview.appspot.com/5991058/

[lp:~jimbaker/pyjuju/must-specify-relation-name]

516. By Jim Baker on 2012-04-10

Client side check

[Jim Baker (jimbaker)]

I have added this check on the client side.

https://codereview.appspot.com/5991058/

=== modified file 'juju/hooks/commands.py'
--- juju/hooks/commands.py	2012-04-05 00:58:19 +0000
+++ juju/hooks/commands.py	2012-04-10 16:45:23 +0000
@@ -8,6 +8,7 @@
 
 from juju.hooks.cli import (
     CommandLineClient, parse_log_level, parse_port_protocol)
+from juju.hooks.protocol import MustSpecifyRelationName
 
 
 BAD_CHARS = re.compile("[\-\./:()<>|?*]|(\\\)")
@@ -98,16 +99,23 @@
     keyvalue_pairs = False
 
     def customize_parser(self):
+        relation_name = os.environ.get("JUJU_RELATION", "")
+
         self.parser.add_argument(
             "relation_name",
             metavar="RELATION NAME",
             nargs="?",
-            default="",
-            help="Specify the relation name of the relation ids to list.")
+            default=relation_name,
+            help=("Specify the relation name of the relation ids to list. "
+                  "Defaults to $JUJU_RELATION, if available."))
 
+    @inlineCallbacks
     def run(self):
-        return self.client.relation_ids(
+        if not self.options.relation_name:
+            raise MustSpecifyRelationName()
+        result = yield self.client.relation_ids(
             self.options.client_id, self.options.relation_name)
+        returnValue(result)
 
 
 def relation_ids():

=== modified file 'juju/hooks/protocol.py'
--- juju/hooks/protocol.py	2012-04-05 16:38:15 +0000
+++ juju/hooks/protocol.py	2012-04-10 16:45:23 +0000
@@ -73,11 +73,19 @@
     """
 
 
+class MustSpecifyRelationName(JujuError):
+    """No relation name was specified."""
+
+    def __str__(self):
+        return "Relation name must be specified"
+
+
 class BaseCommand(amp.Command):
     errors = {NoSuchUnit: "NoSuchUnit",
               NoSuchKey: "NoSuchKey",
               NotRelationContext: "NotRelationContext",
-              UnitRelationStateNotFound: "UnitRelationStateNotFound"}
+              UnitRelationStateNotFound: "UnitRelationStateNotFound",
+              MustSpecifyRelationName: "MustSpecifyRelationName"}
 
 
 # All the commands below this point should be documented in the
@@ -243,9 +251,15 @@
     def relation_ids(self, client_id, relation_name):
         """Set values into state.hook.RelationHookContext.
 
+        :client_id: hooks client id that is used to define a context
+           for a consistent view of state.
+
         :param relation_name: The relation name to query relation ids
-           for this context.
+           for this context. If no such relation name is specified,
+           raises `MustSpecifyRelationName`.
         """
+        if not relation_name:
+            raise MustSpecifyRelationName()
         context = yield self.factory.get_context(client_id)
         ids = yield context.get_relation_idents(relation_name)
         defer.returnValue(dict(ids=" ".join(ids)))

=== modified file 'juju/hooks/tests/test_communications.py'
--- juju/hooks/tests/test_communications.py	2012-04-05 00:49:40 +0000
+++ juju/hooks/tests/test_communications.py	2012-04-10 16:45:23 +0000
@@ -3,10 +3,10 @@
 
 from twisted.internet import protocol, defer, error
 
+from juju.errors import JujuError
 from juju.hooks.protocol import (UnitAgentClient, UnitAgentServer,
                                  UnitSettingsFactory, NoSuchUnit,
-                                 NoSuchKey)
-
+                                 NoSuchKey, MustSpecifyRelationName)
 from juju.lib.mocker import ANY
 from juju.lib.testing import TestCase
 from juju.lib.twistutils import gather_results
@@ -305,6 +305,14 @@
         self.assertEquals(self.server.data["test_node"]["a"], "b")
         self.assertEquals(self.server.data["test_node"]["foo"], "bar")
 
+    def test_must_specify_relation_name(self):
+        """Verify `MustSpecifyRelationName` exception`"""
+        error = MustSpecifyRelationName()
+        self.assertTrue(isinstance(error, JujuError))
+        self.assertEquals(
+            str(error), 
+            "Relation name must be specified")
+
     @defer.inlineCallbacks
     def test_relation_ids(self):
         """Verify api support of relation_ids command"""
@@ -318,6 +326,12 @@
         relation_idents = yield self.client.relation_ids("client_id", "db")
         self.assertEqual(relation_idents, ["db:0", "db:1", "db:42"])
 
+        # A relation name must be specified.
+        e = yield self.assertFailure(
+            self.client.relation_ids("client_id", ""),
+            MustSpecifyRelationName)
+        self.assertEqual(str(e), "Relation name must be specified")
+
     @defer.inlineCallbacks
     def test_list_relations(self):
         # Allow our testing class to pass the usual guard

=== modified file 'juju/hooks/tests/test_invoker.py'
--- juju/hooks/tests/test_invoker.py	2012-04-06 11:19:31 +0000
+++ juju/hooks/tests/test_invoker.py	2012-04-10 16:45:23 +0000
@@ -1113,7 +1113,7 @@
             client_id="client_id")
 
         # Then verify the hook lists the relation ids corresponding to
-        # the relation name `db`; it will not return `db-admin`
+        # the relation name `db`
         hook = self.create_hook("relation-ids", "--format=json db")
         result = yield exe(hook)
         self.assertEqual(result, 0)
@@ -1128,13 +1128,18 @@
         yield self.add_a_blog("wordpress2")
         yield self.add_db_admin_tool("admin")
 
-        # Invoker will be in the context of the mysql/0 service unit
+        # Invoker will be in the context of the mysql/0 service
+        # unit. This test file's mock unit agent does not set the
+        # additional environment variables for relation hooks that are
+        # set by juju.unit.lifecycle.RelationInvoker; instead it has
+        # to be set by individual tests.
         exe = yield self.ua.get_invoker(
             "db:0", "add", "mysql/0", self.relation,
             client_id="client_id")
+        exe.environment["JUJU_RELATION"] = "db"
 
         # Then verify the hook lists the relation ids corresponding to
-        # any relation name
+        # to JUJU_RELATION (="db")
         hook_log = self.capture_logging("hook")
         hook = self.create_hook("relation-ids", "--format=json")
         result = yield exe(hook)
@@ -1142,7 +1147,27 @@
         yield exe.ended
         self.assertEqual(
             set(json.loads(hook_log.getvalue())),
-            set(["db:0", "db:1", "db-admin:2"]))
+            set(["db:0", "db:1"]))
+
+        # This time pretend this is a nonrelational hook
+        # context. Ignore the relation stuff in the get_invoker
+        # function below, really it is a nonrelational hook as far as
+        # the hook commands are concerned:
+        exe = yield self.ua.get_invoker(
+            "db:0", "add", "mysql/0", self.relation,
+            client_id="client_id")
+        hook_log = self.capture_logging("hook")
+        hook = self.create_hook("relation-ids", "--format=json")
+        result = yield exe(hook)
+
+        # Currently, exceptions of all hook commands are only logged;
+        # the exit code is always set to 0.
+        self.assertEqual(result, 0)
+        yield exe.ended
+        self.assertIn(
+            ("juju.hooks.protocol.MustSpecifyRelationName: "
+             "Relation name must be specified"),
+            hook_log.getvalue())
 
     @defer.inlineCallbacks
     def test_relation_set_with_relation_id_option(self):