Merge lp:~bcsaller/pyjuju/relation-get-shell into lp:pyjuju

[Benjamin Saller]

Implements relation-get --format=shell
adds --shell-prefix option to relation-get

[Benjamin Saller (bcsaller)]

This version implicitly sets VAR_xxx for shell output as was mentioned around UDS

[Gustavo Niemeyer (niemeyer)]

[1]

+ print >>stream, 'VAR_%s="%s"' % (k.upper(), v)

What if there are quotes and special characters in the value, and
what if the name contains '-', '.', etc?

[2]

Would be good to verify the environment and blow up if there are any
variables starting with "VAR_". This way we'll know hooks will never
pick up variables from somewhere else and consider them as part of
the relation.

review: Needs Fixing

[Benjamin Saller (bcsaller)]

> [1]
>
> + print >>stream, 'VAR_%s="%s"' % (k.upper(), v)
>
> What if there are quotes and special characters in the value, and
> what if the name contains '-', '.', etc?

Bad chars are mapped to '_' now.

>
> [2]
>
> Would be good to verify the environment and blow up if there are any
> variables starting with "VAR_". This way we'll know hooks will never
> pick up variables from somewhere else and consider them as part of
> the relation.

It now un-sets such variables and logs this to stderr (which appears in the hook log).

[Gustavo Niemeyer (niemeyer)]

[1]

> Bad chars are mapped to '_' now.

What about values?

review: Needs Fixing

[Benjamin Saller (bcsaller)]

> [1]
>
> > Bad chars are mapped to '_' now.
>
> What about values?

Fair enough, this uses Python's pipe.quote to quote the shell values as well now

[Gustavo Niemeyer (niemeyer)]

[3]

43 + for k, v in sorted(result.items()):
44 + if k.startswith("VAR_"):

We just debated live. +1 as long as this is changed to nuke variables
from the _environment_, not from the relation.

review: Approve

[lp:~bcsaller/pyjuju/relation-get-shell]

234. By Benjamin Saller on 2011-08-09

review changes, filter env, not relation results

[William Reade (fwereade)]

Doesn't quite match the bug; it might be good to document on the bug that we have decided against adding --shell-prefix, and we just use VAR_. Otherwise LGTM, +1.

review: Approve

=== modified file 'ensemble/hooks/commands.py'
--- ensemble/hooks/commands.py	2011-06-02 19:29:20 +0000
+++ ensemble/hooks/commands.py	2011-08-09 18:46:23 +0000
@@ -1,6 +1,8 @@
+import logging
 import os
+import pipes
+import re
 import sys
-import logging
 
 from twisted.internet.defer import inlineCallbacks, returnValue
 
@@ -8,11 +10,15 @@
     CommandLineClient, parse_log_level, parse_port_protocol)
 
 
+BAD_CHARS = re.compile("[\-\./:()<>|?*]|(\\\)")
+
+
 class RelationGetCli(CommandLineClient):
     keyvalue_pairs = False
 
     def customize_parser(self):
         remote_unit = os.environ.get("ENSEMBLE_REMOTE_UNIT")
+
         self.parser.add_argument("settings_name", default="", nargs="?")
         self.parser.add_argument("unit_name", default=remote_unit, nargs="?")
 
@@ -26,6 +32,37 @@
                                                 self.options.settings_name)
         returnValue(result)
 
+    def format_shell(self, result, stream):
+        options = self.options
+        settings_name = options.settings_name
+
+        if settings_name and settings_name != "-":
+            # result should be a single value
+            result = {settings_name.upper(): result}
+
+        if result:
+            errs = []
+            for k, v in sorted(os.environ.items()):
+                if k.startswith("VAR_"):
+                    print >>stream, "%s=" % (k.upper(), )
+                    errs.append(k)
+
+            for k, v in sorted(result.items()):
+                k = BAD_CHARS.sub("_", k.upper())
+                v = pipes.quote(v)
+                print >>stream, 'VAR_%s=%s' % (k.upper(), v)
+
+            # order of output within streams is assured but we
+            # output on (commonly) two streams here and the
+            # ordering of those messages are significant to the user
+            stream.flush()
+
+            if errs:
+                print >>sys.stderr, "The following were omitted from " \
+                "the environment.  VAR_ prefixed variables indicate a " \
+                "usage error."
+                print >> sys.stderr, "".join(errs)
+
 
 def relation_get():
     """Entry point for relation-get"""

=== modified file 'ensemble/hooks/tests/test_invoker.py'
--- ensemble/hooks/tests/test_invoker.py	2011-07-29 18:14:21 +0000
+++ ensemble/hooks/tests/test_invoker.py	2011-08-09 18:46:23 +0000
@@ -289,6 +289,68 @@
         self.assertEqual(data["forgotten"], "lyrics")
 
     @defer.inlineCallbacks
+    def test_spawn_cli_get_format_shell(self):
+        """Validate the get hook can transmit values to the hook."""
+        yield self.build_default_relationships()
+
+        hook_log = self.capture_logging("hook")
+
+        # Populate and verify some data we will
+        # later extract with the hook
+        expected = {"name": "rabbit",
+                    "forgotten": "lyrics",}
+        yield self.wordpress_relation.set_data(expected)
+
+        exe = self.ua.get_invoker("db", "add", "mysql/0", self.mysql_relation,
+                                  client_id="client_id")
+        exe.environment["ENSEMBLE_REMOTE_UNIT"] = "wordpress/0"
+
+        # invoke relation-get and verify the result
+        result = yield exe(self.create_hook("relation-get", "--format=shell -"))
+        self.assertEqual(result, 0)
+        data = hook_log.getvalue()
+
+        self.assertEqual('VAR_FORGOTTEN=lyrics\nVAR_NAME=rabbit\n\n', data)
+
+        # and with a single value request
+        hook_log.truncate(0)
+        result = yield exe(self.create_hook("relation-get", "--format=shell name"))
+        self.assertEqual(result, 0)
+        data = hook_log.getvalue()
+        self.assertEqual('VAR_NAME=rabbit\n\n', data)
+
+    @defer.inlineCallbacks
+    def test_relation_get_format_shell_bad_vars(self):
+        """If illegal values are make somehow available warn."""
+        yield self.build_default_relationships()
+        hook_log = self.capture_logging("hook")
+
+        # Populate and verify some data we will
+        # later extract with the hook
+        expected = {"BAR": "none", "funny-chars*":  "should work"}
+        yield self.wordpress_relation.set_data(expected)
+
+        exe = self.ua.get_invoker("db", "add", "mysql/0", self.mysql_relation,
+                                  client_id="client_id")
+        exe.environment["ENSEMBLE_REMOTE_UNIT"] = "wordpress/0"
+        exe.environment["VAR_FOO"] = "jungle"
+
+        result = yield exe(self.create_hook("relation-get", "--format=shell -"))
+        self.assertEqual(result, 0)
+
+        data = hook_log.getvalue()
+        self.assertIn('VAR_BAR=none', data)
+        # Verify that illegal shell variable names get converted
+        # in an expected way
+        self.assertIn("VAR_FUNNY_CHARS_='should work'", data)
+
+        # Verify that it sets VAR_FOO to null because it shouldn't
+        # exist in the environment
+        self.assertIn("VAR_FOO=", data)
+        self.assertIn("The following were omitted from", data)
+        self.assertEqual(data.strip().split("\n")[-1], "VAR_FOO")
+
+    @defer.inlineCallbacks
     def test_hook_exec_in_formula_directory(self):
         """Hooks are executed in the formula directory."""
         yield self.build_default_relationships()
@@ -317,6 +379,7 @@
         self.assertEqual(hook_log.getvalue().strip(),
                          os.path.join(exe.unit_path, "formula"))
 
+
     @defer.inlineCallbacks
     def test_spawn_cli_config_get(self):
         """Validate that config-get returns expected values."""