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 MustSpecifyRelationName()
          context = yield self.factory.get_context(client_id)
          ids = yield context.get_relation_idents(relation_name)
          defer.returnValue(dict(ids=" ".join(ids)))


Index: juju/hooks/tests/test_communications.py
=== 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-06 20:29:41 +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


Index: juju/hooks/tests/test_invoker.py
=== modified file 'juju/hooks/tests/test_invoker.py'
--- juju/hooks/tests/test_invoker.py	2012-04-05 01:11:43 +0000
+++ juju/hooks/tests/test_invoker.py	2012-04-06 20:29:41 +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(
+            ("Failure: juju.hooks.protocol.MustSpecifyRelationName: "
+             "Relation name must be specified"),
+            hook_log.getvalue())

      @defer.inlineCallbacks
      def test_relation_set_with_relation_id_option(self):