Merge lp:~bac/charms/precise/juju-gui/1086790 into lp:~juju-gui/charms/precise/juju-gui/trunk

[Brad Crittenden]

Add command-level logging in the hooks. Also for the run('make') command, stdout and stderr are redirected to a separate file so that progress can be monitored as it occurs. None of the other commands are long running to require that level of detail.

Sorry for not using Rietveld but I could not get lbox to work with this target.

[Benji York (benji)]

The branch looks good. I only had small suggestions or questions.

The "#-*- python -*-" on line 28 surprised me. I am not against that
style of file type markers, but I just wanted to be sure it was
intentional. Oh, and it would be nice if there were a space after the
"#" character. :)

The import on line 130 could be a single-line import.

Line 164: hmm, prefixing the string with a newline will mean that the
log file starts with a blank line. That's not the end of the world, but
it is a bit odd. Maybe a comment to explain why would be in order.

Line 171 of the diff, the contents of the revision file: I /think/ I
saw a recent branch that bumped the revision higher than 13. You might
want to double-check what number the trunk is on.

Line 206: "The monkey patch is undone at the end of the test." Should
that read "The monkey patch is undone in the tearDown method."?

review: Approve (code)

[lp:~bac/charms/precise/juju-gui/1086790]

9. By Brad Crittenden on 2012-12-07

Changes from review: formatting, adding comments, clarifications.

[Gary Poster (gary)]

Nice, Brad, thank you. Thank you for the test, also.

Your hooks/stop change added a tab (line 114 of current diff). I'm pretty sure you/we want it to be spaces.

I worry that the new logging function in hooks/utils.py will make fixing bug 1086507 harder still. Thinking it through, though, I think we will want to make that a separate task even for this change: to partially address it now will mean adding a config-changed hook, and that would be a bigger job than I want in this branch.

That's all I have from a visual review. I'll actually try it out now, and then follow up here.

review: Approve

[lp:~bac/charms/precise/juju-gui/1086790]

10. By Brad Crittenden on 2012-12-10

Detabify

=== modified file 'config.yaml'
--- config.yaml	2012-12-03 10:02:45 +0000
+++ config.yaml	2012-12-10 14:55:21 +0000
@@ -29,6 +29,13 @@
     default: sample
   juju-gui-console-enabled:
     description: |
-      Whether or not the browser's console should be enabled.
+      Whether or not the console should be enabled for the browser.
     type: boolean
     default: false
+  command-log-file:
+    description: |
+      The log file where stdout and stderr should be sent for all
+      commands that are run by charm hooks.
+    type: string
+    default: /var/log/juju/juju-gui.log
+

=== modified file 'hooks/install'
--- hooks/install	2012-11-30 17:00:51 +0000
+++ hooks/install	2012-12-10 14:55:21 +0000
@@ -1,7 +1,12 @@
 #!/usr/bin/env python2
-
-from subprocess import check_call
-
+# -*- python -*-
+
+import os.path
+from subprocess import (
+    CalledProcessError,
+    check_call,
+    )
+import tempfile
 
 # python-shelltoolbox is installed as a dependency of python-charmhelpers.
 check_call(['apt-get', 'install', '-y', 'python-charmhelpers'])
@@ -23,34 +28,45 @@
     run,
 )
 
+from utils import cmd_log
+
 
 def fetch(juju_gui_branch, juju_api_branch):
     """Install required dependencies and retrieve Juju/Juju GUI branches."""
     log('Installing dependencies.')
-    install_extra_repositories('ppa:chris-lea/node.js')
-    apt_get_install('bzr', 'imagemagick', 'make', 'nodejs', 'npm', 'zookeeper')
+    cmd_log(install_extra_repositories('ppa:chris-lea/node.js'))
+    cmd_log(
+        apt_get_install('bzr', 'imagemagick', 'make',
+                        'nodejs', 'npm', 'zookeeper'))
     log('Retrieving source checkouts.')
     bzr_checkout = command('bzr', 'co', '--lightweight')
-    bzr_checkout(juju_gui_branch, 'juju-gui')
-    bzr_checkout(juju_api_branch, 'juju')
-
-
-def build():
+    cmd_log(bzr_checkout(juju_gui_branch, 'juju-gui'))
+    cmd_log(bzr_checkout(juju_api_branch, 'juju'))
+
+
+def build(logpath):
     """Set up Juju GUI."""
     log('Building Juju GUI.')
     with cd('juju-gui'):
-        run('make')
+        logdir = os.path.dirname(logpath)
+        fd, name = tempfile.mkstemp(prefix='make-', dir=logdir)
+        log('Output from "make" sent to', name)
+        run('make', stdout=fd, stderr=fd)
 
 
 def main():
     config = get_config()
     fetch(config['juju-gui-branch'], config['juju-api-branch'])
-    build()
+    build(config['command-log-file'])
 
 
 if __name__ == '__main__':
     log_entry()
     try:
         main()
+    except CalledProcessError as e:
+        log('Exception caught:')
+        log(e.output)
+        raise
     finally:
         log_exit()

=== modified file 'hooks/start'
--- hooks/start	2012-12-03 10:02:45 +0000
+++ hooks/start	2012-12-10 14:55:21 +0000
@@ -1,4 +1,5 @@
 #!/usr/bin/env python2
+#-*- python -*-
 
 import json
 import os

=== modified file 'hooks/stop'
--- hooks/stop	2012-12-03 10:02:45 +0000
+++ hooks/stop	2012-12-10 14:55:21 +0000
@@ -1,4 +1,5 @@
 #!/usr/bin/env python2
+#-*- python -*-
 
 from charmhelpers import (
     get_config,

=== modified file 'hooks/utils.py'
--- hooks/utils.py	2012-11-29 14:49:41 +0000
+++ hooks/utils.py	2012-12-10 14:55:21 +0000
@@ -1,8 +1,10 @@
 """Juju GUI charm utilities."""
 
 import os
+import logging
 
 from shelltoolbox import search_file
+from charmhelpers import get_config
 
 
 def get_zookeeper_address(agent_file_path):
@@ -30,3 +32,28 @@
     contents = open(template_path).read()
     with open(destination, 'w') as stream:
         stream.write(contents % context)
+
+
+results_log = None
+
+def _setupLogging():
+    global results_log
+    if results_log is not None:
+        return
+    config = get_config()
+    logging.basicConfig(
+        filename=config['command-log-file'],
+        level=logging.INFO,
+        format="%(asctime)s: %(name)s@%(levelname)s %(message)s")
+    results_log = logging.getLogger('juju-gui')
+
+
+def cmd_log(results):
+    global results_log
+    if not results:
+        return
+    if results_log is None:
+        _setupLogging()
+    # Since 'results' may be multi-line output, start it on a separate line
+    # from the logger timestamp, etc.
+    results_log.info('\n' + results)

=== modified file 'revision'
--- revision	2012-11-30 17:36:15 +0000
+++ revision	2012-12-10 14:55:21 +0000
@@ -1,1 +1,1 @@
-12
+13

=== modified file 'tests/deploy.test'
--- tests/deploy.test	2012-11-30 17:00:51 +0000
+++ tests/deploy.test	2012-12-10 14:55:21 +0000
@@ -1,4 +1,5 @@
 #!/usr/bin/env python2
+#-*- python -*-
 
 import time
 import unittest

=== modified file 'tests/test_utils.py'
--- tests/test_utils.py	2012-11-29 14:49:41 +0000
+++ tests/test_utils.py	2012-12-10 14:55:21 +0000
@@ -3,8 +3,11 @@
 import os
 import tempfile
 import unittest
+import charmhelpers
+from simplejson import dumps
 
 from utils import (
+    cmd_log,
     get_zookeeper_address,
     render_to_file,
 )
@@ -44,6 +47,25 @@
         expected = self.template_contents % context
         self.assertEqual(expected, self.destination_file.read())
 
+class CmdLogTest(unittest.TestCase):
+    def setUp(self):
+        # Patch the charmhelpers 'command', which powers get_config.  The
+        # result of this is the mock_config dictionary will be returned.
+        # The monkey patch is undone in the tearDown.
+        self.command = charmhelpers.command
+        fd, self.log_file_name = tempfile.mkstemp()
+        os.close(fd)
+        mock_config = {'command-log-file': self.log_file_name}
+        charmhelpers.command = lambda *args: lambda: dumps(mock_config)
+
+    def tearDown(self):
+        charmhelpers.command = self.command
+
+    def test_contents_logged(self):
+        cmd_log('foo')
+        line = open(self.log_file_name, 'r').read()
+        self.assertTrue(line.endswith(': juju-gui@INFO \nfoo\n'))
+
 
 if __name__ == '__main__':
     unittest.main(verbosity=2)

=== modified file 'tests/unit.test'
--- tests/unit.test	2012-11-21 14:51:05 +0000
+++ tests/unit.test	2012-12-10 14:55:21 +0000
@@ -1,4 +1,5 @@
 #!/usr/bin/env python2
+# -*- python -*-
 
 import os
 import sys