Merge lp:~bcsaller/pyjuju/sane_output_test_option into lp:pyjuju

[Benjamin Saller]

Adds support for --test to cli utils

This branch bundles two related changes, support for --test on relation-get, unit-get and config-get.
This should be similar to the additions made in the golang version.

Also this changes to always use a formatter for output and the smart output formatter was changed to
YAML dump collection types, normal string values are still printed w/o serialization.

https://codereview.appspot.com/6216053/

[Benjamin Saller (bcsaller)]

Reviewers: mp+106067_code.launchpad.net,

Message:
Please take a look.

Description:
Adds support for --test and doesn't output Python reprs

This branch bundles two related changes, support for --test on
relation-get, unit-get and config-get.
This should be similar to the additions made in the golang version.

Also this changes to always use a formatter for output and the smart
output formatter was changed to
YAML dump collection types, normal string values are still printed w/o
serialization.

https://code.launchpad.net/~bcsaller/juju/sane_output_test_option/+merge/106067

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M juju/hooks/cli.py
   M juju/hooks/commands.py
   M juju/hooks/tests/test_invoker.py

[Gustavo Niemeyer (niemeyer)]

Some details here are not clear to me. I'd appreciate having a chat
about this feature with you before this goes in.

https://codereview.appspot.com/6216053/diff/1/juju/hooks/cli.py
File juju/hooks/cli.py (right):

https://codereview.appspot.com/6216053/diff/1/juju/hooks/cli.py#newcode228
juju/hooks/cli.py:228: # yaml dump container types
What was your thinking here? I believe it should dump yaml in all
cases. i.e. "true", not "True".

https://codereview.appspot.com/6216053/diff/1/juju/hooks/commands.py
File juju/hooks/commands.py (right):

https://codereview.appspot.com/6216053/diff/1/juju/hooks/commands.py#newcode80
juju/hooks/commands.py:80: v = json.dumps(v)
json?

https://codereview.appspot.com/6216053/

[Clint Byrum (clint-fewbar)]

I've been thinking about this, and this is a backward incompatible change, I'm afraid.

Up until now, charms w/ boolean configs have been able to count on "True" or "False" and have been written as such.

This will change the case, and that is, unfortunately, not acceptable in the planned "fixes only" release of Juju.

The --test is fantastic, and should be added. But I'm afraid if we do the other bit without bumping to a new "major release" we will have many many broken charms very suddenly.

[Kapil Thangavelu (hazmat)]

its backwards incompatible for charms, which we should fix and encourage extant users to update. the existing output is broken in that it depends on no standard except the implicit one by python's str/repr output. The whole 'smart' format stuff was decidedly not.

[Gustavo Niemeyer (niemeyer)]

Mail sent to canonical-juju. This bug will be fixed, we have no option. The decision of when to fix it is your hands, Clint.

[Kapil Thangavelu (hazmat)]

bug in base formatter.

https://codereview.appspot.com/6216053/diff/1/juju/hooks/cli.py
File juju/hooks/cli.py (right):

https://codereview.appspot.com/6216053/diff/1/juju/hooks/cli.py#newcode220
juju/hooks/cli.py:220: formatter(result, stream)
None is not a callable type, this tries a double output if formatter is
None. please default formatter to the correct default output.

https://codereview.appspot.com/6216053/

[Kapil Thangavelu (hazmat)]

it would be nice to rebase this on current trunk where the formatting work has been done in a compatible fashion, and slim this branch down to just the --test option which is still useful

[Benjamin Saller (bcsaller)]

Please take a look.

https://codereview.appspot.com/6216053/diff/1/juju/hooks/cli.py
File juju/hooks/cli.py (right):

https://codereview.appspot.com/6216053/diff/1/juju/hooks/cli.py#newcode228
juju/hooks/cli.py:228: # yaml dump container types
On 2012/05/17 02:42:44, niemeyer wrote:
> What was your thinking here? I believe it should dump yaml in all
cases. i.e.
> "true", not "True".

You're correct that non-string types should appear here.

https://codereview.appspot.com/6216053/

[Kapil Thangavelu (hazmat)]

I'm still a little unclear as to the desired behavior outcome and its
testing with this branch, as it currently tested it doesn't seem
particularly useful.

https://codereview.appspot.com/6216053/diff/6001/juju/hooks/commands.py
File juju/hooks/commands.py (right):

https://codereview.appspot.com/6216053/diff/6001/juju/hooks/commands.py#newcode18
juju/hooks/commands.py:18: class TestOptionMixin(object):
missing doc string describing purpose/usage.

https://codereview.appspot.com/6216053/diff/6001/juju/hooks/commands.py#newcode34
juju/hooks/commands.py:34: self.handle_test(result)
there's a lot of unesc. method delegation here. just inline the
test_true/handle_test behavior.

The use of the mixin, also feels like its adding complexity here, around
subclass ordering and proper lineage to customize_parser vs. render
(which have inverse call lineages). Incorporating into the base class
might be cleaner via class flag switch.

https://codereview.appspot.com/6216053/diff/6001/juju/hooks/commands.py#newcode114
juju/hooks/commands.py:114: def render(self, result):
Why is this returning None, why is it implemented at all? ditto for the
two other implementations of the same here.

https://codereview.appspot.com/6216053/diff/6001/juju/hooks/tests/test_invoker.py
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/6216053/diff/6001/juju/hooks/tests/test_invoker.py#newcode356
juju/hooks/tests/test_invoker.py:356:
this is testing something very different then what the cli usage would
commonly be, namely the cli usage would be testing for values unset.

For values set, a null value seems valid, has the go team documented the
behavior of this cli param?

https://codereview.appspot.com/6216053/

Unmerged revisions

556. By Benjamin Saller on 2012-07-03

test option support in cli hooks

=== modified file 'juju/hooks/commands.py'
--- juju/hooks/commands.py	2012-05-03 16:46:40 +0000
+++ juju/hooks/commands.py	2012-07-03 18:49:20 +0000
@@ -1,3 +1,4 @@
+import json
 import logging
 import os
 import pipes
@@ -14,7 +15,28 @@
 BAD_CHARS = re.compile("[\-\./:()<>|?*]|(\\\)")
 
 
-class RelationGetCli(CommandLineClient):
+class TestOptionMixin(object):
+    def customize_parser(self):
+        self.parser.add_argument("--test",
+                                 action="store_true",
+                                 default=False,
+                                 help="Exit code indicates if the value "
+                                 "is present and not empty")
+
+    def tests_true(self, result):
+        return 0 if bool(result) else 1
+
+    def handle_test(self, result):
+        self.exit_code = self.tests_true(result)
+
+    def render(self, result):
+        if self.options.test is True:
+            self.handle_test(result)
+        else:
+            super(TestOptionMixin, self).render(result)
+
+
+class RelationGetCli(TestOptionMixin, CommandLineClient):
     keyvalue_pairs = False
 
     def customize_parser(self):
@@ -24,6 +46,7 @@
             "-r", dest="relation_id", default="", metavar="RELATION ID")
         self.parser.add_argument("settings_name", default="", nargs="?")
         self.parser.add_argument("unit_name", default=remote_unit, nargs="?")
+        super(RelationGetCli, self).customize_parser()
 
     @inlineCallbacks
     def run(self):
@@ -88,6 +111,9 @@
                                         self.options.relation_id,
                                         self.options.keyvalue_pairs)
 
+    def render(self, result):
+        return None
+
 
 def relation_set():
     """Entry point for relation-set."""
@@ -179,11 +205,12 @@
     sys.exit(client())
 
 
-class ConfigGetCli(CommandLineClient):
+class ConfigGetCli(TestOptionMixin, CommandLineClient):
     keyvalue_pairs = False
 
     def customize_parser(self):
         self.parser.add_argument("option_name", default="", nargs="?")
+        super(ConfigGetCli, self).customize_parser()
 
     @inlineCallbacks
     def run(self):
@@ -213,6 +240,9 @@
         return self.client.open_port(
             self.options.client_id, *self.options.port_protocol)
 
+    def render(self, result):
+        return None
+
 
 def open_port():
     """Entry point for open-port."""
@@ -234,6 +264,9 @@
         return self.client.close_port(
             self.options.client_id, *self.options.port_protocol)
 
+    def render(self, result):
+        return None
+
 
 def close_port():
     """Entry point for close-port."""
@@ -241,11 +274,12 @@
     sys.exit(client())
 
 
-class UnitGetCli(CommandLineClient):
+class UnitGetCli(TestOptionMixin, CommandLineClient):
     keyvalue_pairs = False
 
     def customize_parser(self):
         self.parser.add_argument("setting_name")
+        super(UnitGetCli, self).customize_parser()
 
     @inlineCallbacks
     def run(self):

=== modified file 'juju/hooks/tests/test_invoker.py'
--- juju/hooks/tests/test_invoker.py	2012-07-03 07:30:53 +0000
+++ juju/hooks/tests/test_invoker.py	2012-07-03 18:49:20 +0000
@@ -340,6 +340,49 @@
         self.assertEqual('VAR_NAME=rabbit\n\n', data)
 
     @defer.inlineCallbacks
+    def test_relation_get_test_option(self):
+        yield self.build_default_relationships()
+
+        hook_log = self.capture_logging("hook")
+
+        # Populate and verify some data we will
+        # later extract with the hook
+        values = {"string": "rabbit hole",
+                  "emptylist": [],
+                  "emptystring": "",
+                  "zero": 0,
+                  "number": 1}
+        yield self.wordpress_relation.set_data(values)
+
+        exe = yield self.ua.get_invoker(
+            "db:42", "add", "mysql/0", self.mysql_relation,
+            client_id="client_id")
+        exe.environment["JUJU_REMOTE_UNIT"] = "wordpress/0"
+
+        @defer.inlineCallbacks
+        def test_exit(variable, expected):
+            # --test can return a bad exit code from the POV of
+            # the invoker, this is fine, its just the way I'm using
+            # it to get at the value thats otherwise fine in the
+            # hook itself
+            try:
+                result = yield exe(self.create_hook("relation-get",
+                                                    "--test %s" % variable))
+                self.assertEqual(result, expected)
+            except errors.CharmInvocationError:
+                if expected == 1:
+                    pass
+            data = hook_log.getvalue()
+            self.assertEqual(data, "")
+
+        yield test_exit("string", 0)
+        yield test_exit("emptylist", 1)
+        yield test_exit("emptystring", 1)
+        yield test_exit("zero", 1)
+        yield test_exit("number", 0)
+
+
+    @defer.inlineCallbacks
     def test_relation_get_format_shell_bad_vars(self):
         """If illegal values are make somehow available warn."""
         yield self.build_default_relationships()
@@ -433,6 +476,49 @@
         self.assertEqual(data["name"], "rabbit")
         self.assertEqual(data["forgotten"], "lyrics")
 
+    @defer.inlineCallbacks
+    def test_config_get_test_option(self):
+        """Validate that config-get returns expected values."""
+        yield self.build_default_relationships()
+
+        hook_log = self.capture_logging("hook")
+
+        # Populate and verify some data we will
+        # later extract with the hook
+        expected = {"string": "rabbit",
+                    "empty": "",
+                    "emptylist": []}
+
+        exe = yield self.ua.get_invoker(
+            "db:42", "add", "mysql/0", self.mysql_relation,
+            client_id="client_id")
+
+        context = yield self.ua.get_context("client_id")
+        config = yield context.get_config()
+        config.update(expected)
+        yield config.write()
+
+        # invoke relation-get and verify the result
+        @defer.inlineCallbacks
+        def test_exit(variable, expected):
+            # --test can return a bad exit code from the POV of
+            # the invoker, this is fine, its just the way I'm using
+            # it to get at the value thats otherwise fine in the
+            # hook itself
+            try:
+                result = yield exe(self.create_hook("config-get",
+                                                    "--test %s" % variable))
+                self.assertEqual(result, expected)
+            except errors.CharmInvocationError:
+                if expected == 1:
+                    pass
+            data = hook_log.getvalue()
+            self.assertEqual(data, "")
+
+        yield test_exit("string", 0)
+        yield test_exit("emptylist", 1)
+        yield test_exit("emptystring", 1)
+
 
 class RelationInvokerTestBase(InvokerTestBase, RelationTestBase):
 
@@ -684,6 +770,18 @@
         self.assertEqual("mysql-0.example.com", data.strip())
 
     @defer.inlineCallbacks
+    def test_unit_get_test_option(self):
+        """Private addresses can be tested for."""
+        hook_log = self.capture_logging("hook")
+        exe = yield self.ua.get_invoker(
+            "database:42", "add", "mysql/0", self.relation,
+            client_id="client_id")
+        result = yield exe(self.create_hook("unit-get", "--test private-address"))
+        self.assertEqual(result, 0)
+        data = hook_log.getvalue()
+        self.assertEqual("", data.strip())
+
+    @defer.inlineCallbacks
     def test_spawn_cli_get_unit_unknown_public_address(self):
         """If for some hysterical raison, the public address hasn't been set.