Merge lp:~bcsaller/pyjuju/log-output into lp:pyjuju

Proposed by Benjamin Saller
Status: Merged
Merged at revision: 534
Proposed branch: lp:~bcsaller/pyjuju/log-output
Merge into: lp:pyjuju
Diff against target: 52 lines (+11/-4)
2 files modified
juju/hooks/commands.py (+4/-1)
juju/hooks/tests/test_invoker.py (+7/-3)
To merge this branch: bzr merge lp:~bcsaller/pyjuju/log-output
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+103762@code.launchpad.net

Description of the change

juju-log shouldn't log {} to stdout

The default output format for juju-log was including a marshalled response of an empty dict. This masks that output.

https://codereview.appspot.com/6114061/

To post a comment you must log in.
Revision history for this message
Benjamin Saller (bcsaller) wrote :

Reviewers: mp+103762_code.launchpad.net,

Message:
Please take a look.

Description:
juju-log shouldn't log {} to stdout

The default output format for juju-log was including a marshalled
response of an empty dict. This masks that output.

https://code.launchpad.net/~bcsaller/juju/log-output/+merge/103762

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M juju/hooks/cli.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/cli.py
=== modified file 'juju/hooks/cli.py'
--- juju/hooks/cli.py 2012-03-27 01:04:50 +0000
+++ juju/hooks/cli.py 2012-04-26 03:51:20 +0000
@@ -225,7 +225,7 @@
          print >>stream, json.dumps(result)

      def format_smart(self, result, stream):
- if result is not None:
+ if result is not None and not isinstance(result, dict):
              print >>stream, str(result)

Index: juju/hooks/tests/test_invoker.py
=== modified file 'juju/hooks/tests/test_invoker.py'
--- juju/hooks/tests/test_invoker.py 2012-04-25 00:20:26 +0000
+++ juju/hooks/tests/test_invoker.py 2012-04-26 03:51:20 +0000
@@ -827,9 +827,13 @@
          # of the MESSAGE variable
          result = yield exe(self.get_test_hook("echo-hook"))
          self.assertEqual(result, 0)
-
          yield exe.ended
          self.assertIn(message, self.log.getvalue())
+
+ # Logging used to log an empty response dict
+ # assure this doesn't happpen [b=915506]
+ self.assertNotIn("{}", self.log.getvalue())
+
          # The 'error' was sent via juju-log
          # to the UA. Our test UA has a fake log stream
          # which we can check now

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

i'd suggest an additional test for config-get against an empty config to
ensure this only affects the logging case.

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

https://codereview.appspot.com/6114061/diff/1/juju/hooks/cli.py#newcode228
juju/hooks/cli.py:228: if result is not None and not isinstance(result,
dict):
is this smart enough?.. what if i did relation-get/config-get against an
empty value, i'd normally get back an empty dictionary.

https://codereview.appspot.com/6114061/

lp:~bcsaller/pyjuju/log-output updated
534. By Benjamin Saller

wip for root issue around logging, rather than smarter output handling

535. By Benjamin Saller

merge trunk

536. By Benjamin Saller

the need for a return value remains, this fixes the command to not issue any output on success

Revision history for this message
Benjamin Saller (bcsaller) wrote :
Revision history for this message
Kapil Thangavelu (hazmat) wrote :
lp:~bcsaller/pyjuju/log-output updated
537. By Benjamin Saller

fix ws error

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'juju/hooks/commands.py'
2--- juju/hooks/commands.py 2012-04-24 21:47:40 +0000
3+++ juju/hooks/commands.py 2012-05-03 16:49:19 +0000
4@@ -137,7 +137,7 @@
5
6 def run(self):
7 return self.client.list_relations(self.options.client_id,
8- self.options.relation_id)
9+ self.options.relation_id)
10
11 def format_eval(self, result, stream):
12 """ eval `juju-list` """
13@@ -169,6 +169,9 @@
14 return self.client.log(self.options.l,
15 self.options.message)
16
17+ def render(self, result):
18+ return None
19+
20
21 def log():
22 """Entry point for juju-log."""
23
24=== modified file 'juju/hooks/tests/test_invoker.py'
25--- juju/hooks/tests/test_invoker.py 2012-04-25 00:20:26 +0000
26+++ juju/hooks/tests/test_invoker.py 2012-05-03 16:49:19 +0000
27@@ -33,8 +33,8 @@
28 self.client = client
29 self.socket_path = socket_path
30 self.charm_dir = charm_dir
31- self._clients = {} # client_id -> HookContext
32- self._invokers = {} # client_id -> Invoker
33+ self._clients = {} # client_id -> HookContext
34+ self._invokers = {} # client_id -> Invoker
35
36 self._agent_log = logging.getLogger("unit-agent")
37 self._agent_io = StringIO()
38@@ -827,9 +827,13 @@
39 # of the MESSAGE variable
40 result = yield exe(self.get_test_hook("echo-hook"))
41 self.assertEqual(result, 0)
42-
43 yield exe.ended
44 self.assertIn(message, self.log.getvalue())
45+
46+ # Logging used to log an empty response dict
47+ # assure this doesn't happpen [b=915506]
48+ self.assertNotIn("{}", self.log.getvalue())
49+
50 # The 'error' was sent via juju-log
51 # to the UA. Our test UA has a fake log stream
52 # which we can check now

Subscribers

People subscribed via source and target branches

to status/vote changes: