Merge lp:~gary/charms/precise/juju-gui/update-for-jujucharms into lp:~juju-gui/charms/precise/juju-gui/trunk

[Gary Poster]

Fix charm for jujucharms.com use

- Merge work from mews to support exec.d directory and relative file paths. Add tests.
- Merge work from benji to support charmworld option needed by upcoming gui changes. Make a small fix.
- Fix deploy tests.

Tests are still fragile, but I got them all to pass with the current branch at one time, and verified manually that the various steps still work.

https://codereview.appspot.com/8999043/

[Gary Poster (gary)]

Please take a look.

[Gary Poster (gary)]

Reviewers: mp+161298_code.launchpad.net,

Message:
Please take a look.

Description:
Fix charm for jujucharms.com use

- Merge work from mews to support exec.d directory and relative file
paths. Add tests.
- Merge work from benji to support charmworld option needed by upcoming
gui changes. Make a small fix.
- Fix deploy tests.

Tests are still fragile, but I got them all to pass with the current
branch at one time, and verified manually that the various steps still
work.

https://code.launchpad.net/~gary/charms/precise/juju-gui/update-for-jujucharms/+merge/161298

(do not edit description out of merge proposal)

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

Affected files:
   M .bzrignore
   A [revision details]
   M config.yaml
   M config/config.js.template
   M hooks/backend.py
   M hooks/install
   M hooks/utils.py
   A hooks/web-relation-joined
   M revision
   M tests/deploy.test
   M tests/test_utils.py

[Richard Harding (rharding)]

LGTM, thanks for working through it.

https://codereview.appspot.com/8999043/

[lp:~gary/charms/precise/juju-gui/update-for-jujucharms]

51. By Gary Poster on 2013-04-28

respond to review

[Gary Poster (gary)]

*** Submitted:

Fix charm for jujucharms.com use

- Merge work from mews to support exec.d directory and relative file
paths. Add tests.
- Merge work from benji to support charmworld option needed by upcoming
gui changes. Make a small fix.
- Fix deploy tests.

Tests are still fragile, but I got them all to pass with the current
branch at one time, and verified manually that the various steps still
work.

R=rharding, benji
CC=
https://codereview.appspot.com/8999043

https://codereview.appspot.com/8999043/diff/1/hooks/install
File hooks/install (right):

https://codereview.appspot.com/8999043/diff/1/hooks/install#newcode46
hooks/install:46: for module in os.listdir('exec.d'):
On 2013/04/27 20:29:30, benji wrote:
> Usually ".d" files are to be run in lexicographic order, so throwing a
sort()
> around the listdir would be good.

Good idea in the abstract, and I changed it.

FWIW, this code is provided by IS for their use, and I don't like the
pattern and at least mew has agreed with me, so I'm not super excited
about improving it. Your ideas were good, though, so I grudgingly did
them anyway. :-)

https://codereview.appspot.com/8999043/diff/1/hooks/install#newcode49
hooks/install:49: except OSError:
On 2013/04/27 20:29:30, benji wrote:
> It would be safer if we knew which OSErrors in particular we should
ignore. It
> would also be nice to know why we are ignoring them (i.e., a comment).

OK, good point...I did it, because I agree with you, though I had to
guess at their intent. Hopefully I got it right. :-/

https://codereview.appspot.com/8999043/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/8999043/diff/1/hooks/utils.py#newcode195
hooks/utils.py:195: """Log when a hook starts and stops its execution.
On 2013/04/27 20:29:30, benji wrote:
> Yay! I hate the "an" before non-silent "h" thing.

> I know, I know; I'm weird.

heh

https://codereview.appspot.com/8999043/diff/1/hooks/utils.py#newcode228
hooks/utils.py:228: if source[0] != '/':
On 2013/04/27 20:29:30, benji wrote:
> I don't really prefer it, but just in case you do:

> if not source.startswith('/'):

Yeah, I do, thanks. Changed.

https://codereview.appspot.com/8999043/diff/1/hooks/web-relation-joined
File hooks/web-relation-joined (right):

https://codereview.appspot.com/8999043/diff/1/hooks/web-relation-joined#newcode9
hooks/web-relation-joined:9:
On 2013/04/27 20:29:30, benji wrote:
> Unnecessary newline.

I was doing as the Romans do, but eh, I don't think the Romans will
notice. Deleted.

https://codereview.appspot.com/8999043/diff/1/hooks/web-relation-joined#newcode11
hooks/web-relation-joined:11: log_hook,
On 2013/04/27 20:29:30, benji wrote:
> You can use a one-liner here.

Again, I was following the local patterns in the charm, but eh, I agree.
  Changed.

https://codereview.appspot.com/8999043/diff/1/hooks/web-relation-joined#newcode18
hooks/web-relation-joined:18: hostname = socket.gethostname()
On 2013/04/27 20:29:30, benji wrote:
> I'm pretty sure lines 16-18 are equivalent to

> hostname = socket.getfqdn()

I stared at this for a bit. I think you are probably right. What I had
here is what the haproxy or apache2 charms are...

[Gary Poster (gary)]

thank you both!

https://codereview.appspot.com/8999043/

=== modified file '.bzrignore'
--- .bzrignore	2013-04-15 17:47:54 +0000
+++ .bzrignore	2013-04-28 01:09:28 +0000
@@ -1,2 +1,3 @@
 .emacs*
 tags
+exec.d/*
 
=== modified file 'config.yaml'
--- config.yaml	2013-04-17 19:31:56 +0000
+++ config.yaml	2013-04-28 01:09:28 +0000
@@ -112,3 +112,9 @@
       the source: hooks/install and hooks/backend.py in particular.
     type: boolean
     default: true
+  charmworld-url:
+    description: |
+      The URL of the charm catalog site ("charmworld") from which charm catalog
+      data will be drawn.
+    type: string
+    default: http://staging.jujucharms.com
 
=== modified file 'config/config.js.template'
--- config/config.js.template	2013-04-12 10:09:32 +0000
+++ config/config.js.template	2013-04-28 01:09:28 +0000
@@ -11,6 +11,7 @@
   // SubApps.
   consoleEnabled: {{console_enabled}},
   charm_store_url: 'https://jujucharms.com/',
+  charmworldURL: {{charmworld_url}},
   // socket_url is only honored in older versions of the GUI.
   socket_url: '{{raw_protocol}}://{{address}}/ws',
   // socket_protocol is used instead by newer versions of the GUI to
 
=== added directory 'exec.d'
=== modified file 'hooks/backend.py'
--- hooks/backend.py	2013-04-24 13:18:22 +0000
+++ hooks/backend.py	2013-04-28 01:09:28 +0000
@@ -107,8 +107,8 @@
         start_gui(
             config['juju-gui-console-enabled'], config['login-help'],
             config['read-only'], config['staging'], config['ssl-cert-path'],
-            config['serve-tests'], secure=config['secure'],
-            sandbox=config['sandbox'])
+            config['charmworld-url'], config['serve-tests'],
+            secure=config['secure'], sandbox=config['sandbox'])
         open_port(80)
         open_port(443)
 
 
=== modified file 'hooks/install'
--- hooks/install	2013-04-24 10:46:20 +0000
+++ hooks/install	2013-04-28 01:09:28 +0000
@@ -1,8 +1,9 @@
 #!/usr/bin/env python2
 # -*- python -*-
 
+import errno
 import json
-from subprocess import check_call
+import os
 
 # If the user's environment has "juju-origin: ppa" set, they will
 # automatically have access to python-charmhelpers and python-shelltoolbox.
@@ -27,18 +28,35 @@
     'python-apt', 'python-charmhelpers', 'python-launchpadlib',
     'python-shelltoolbox', 'python-tempita',
 )
-check_call(('apt-get', 'install', '-y') + PYTHON_DEPENDENCIES)
-
-
+bootstrap_utils.run(*(('apt-get', 'install', '-y') + PYTHON_DEPENDENCIES))
+
+
+from charmhelpers import log
 from utils import (
     config_json,
     log_hook,
 )
-
 from backend import Backend
 
 
 def main():
+    # Run pre-install tasks, if available.  Please do not rely on the
+    # exec.d interface without conferring with the Juju GUI team: it may
+    # change after upcoming discussion with Canonical IS.
+    if os.path.isdir('exec.d'):
+        dirnames = os.listdir('exec.d')
+        dirnames.sort()
+        for module in dirnames:
+            filename = os.path.join('exec.d', module, 'charm-pre-install')
+            try:
+                bootstrap_utils.run(filename)
+            except OSError, e:
+                # If the exec.d file does not exist or is not runnable,
+                # assume we can recover.  Log the problem and proceed.
+                if e.errno in (errno.ENOENT, errno.EACCES):
+                    log('{}: {}'.format(e.strerror, filename))
+                else:
+                    raise
     config = get_config()
     backend = Backend(config)
     backend.install()
 
=== modified file 'hooks/utils.py'
--- hooks/utils.py	2013-04-25 16:46:07 +0000
+++ hooks/utils.py	2013-04-28 01:09:28 +0000
@@ -45,6 +45,7 @@
 import shutil
 from subprocess import CalledProcessError
 import tempfile
+from urlparse import urlparse
 
 from launchpadlib.launchpad import Launchpad
 from shelltoolbox import (
@@ -191,7 +192,7 @@
 
 @contextmanager
 def log_hook():
-    """Log when an hook starts and stops its execution.
+    """Log when a hook starts and stops its execution.
 
     Also log to stdout possible CalledProcessError exceptions raised executing
     the hook.
@@ -217,10 +218,17 @@
        - ('stable', '0.1.0'): stable release v0.1.0;
        - ('trunk', None): latest trunk release;
        - ('trunk', '0.1.0+build.1'): trunk release v0.1.0 bzr revision 1;
-       - ('branch', 'lp:juju-gui'): release is made from a branch.
+       - ('branch', 'lp:juju-gui'): release is made from a branch;
+       - ('url', 'http://example.com/gui'): release from a downloaded file.
     """
     if source.startswith('url:'):
-        return 'url', source[4:]
+        source = source[4:]
+        # Support file paths, including relative paths.
+        if urlparse(source).scheme == '':
+            if not source.startswith('/'):
+                source = os.path.join(os.path.abspath(CURRENT_DIR), source)
+            source = "file://%s" % source
+        return 'url', source
     if source in ('stable', 'trunk'):
         return source, None
     if source.startswith('lp:') or source.startswith('http://'):
@@ -310,7 +318,7 @@
 
 def start_gui(
         console_enabled, login_help, readonly, in_staging, ssl_cert_path,
-        serve_tests, haproxy_path='/etc/haproxy/haproxy.cfg',
+        charmworld_url, serve_tests, haproxy_path='/etc/haproxy/haproxy.cfg',
         config_js_path=None, secure=True, sandbox=False):
     """Set up and start the Juju GUI server."""
     with su('root'):
@@ -350,6 +358,7 @@
         'user': json.dumps(user),
         'protocol': json.dumps(protocol),
         'sandbox': json.dumps(sandbox),
+        'charmworld_url': json.dumps(charmworld_url),
     }
     if config_js_path is None:
         config_js_path = os.path.join(
 
=== added file 'hooks/web-relation-joined'
--- hooks/web-relation-joined	1970-01-01 00:00:00 +0000
+++ hooks/web-relation-joined	2013-04-28 01:09:28 +0000
@@ -0,0 +1,23 @@
+#!/usr/bin/env python2
+#-*- python -*-
+
+import socket
+from charmhelpers import (
+    get_config,
+    relation_set,
+)
+from utils import log_hook
+
+
+def main():
+    hostname = socket.getfqdn()
+    config = get_config()
+    if config['secure']:
+    	port = 443
+    else:
+    	port = 80
+    relation_set(port=port, hostname=hostname)
+
+if __name__ == '__main__':
+    with log_hook():
+        main()
 
=== modified file 'revision'
--- revision	2013-04-23 07:32:46 +0000
+++ revision	2013-04-28 01:09:28 +0000
@@ -1,1 +1,1 @@
-34
+40
 
=== modified file 'tests/deploy.test'
--- tests/deploy.test	2013-03-13 21:24:31 +0000
+++ tests/deploy.test	2013-04-28 01:09:28 +0000
@@ -166,7 +166,7 @@
         # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
         self.addCleanup(
             self.stop_services,
-            hostname, ['haproxy', 'nginx', 'juju-api-agent'])
+            hostname, ['haproxy', 'apache2', 'juju-api-agent'])
         self.navigate_to(hostname)
         self.handle_browser_warning()
         self.assertEnvironmentIsConnected()
@@ -177,7 +177,7 @@
         # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
         self.addCleanup(
             self.stop_services,
-            hostname, ['haproxy', 'nginx', 'juju-api-improv'])
+            hostname, ['haproxy', 'apache2', 'juju-api-improv'])
         self.navigate_to(hostname)
         self.handle_browser_warning()
         self.assertEnvironmentIsConnected()
@@ -190,7 +190,7 @@
         # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
         self.addCleanup(
             self.stop_services,
-            hostname, ['haproxy', 'nginx', 'juju-api-agent'])
+            hostname, ['haproxy', 'apache2', 'juju-api-agent'])
         self.navigate_to(hostname)
         self.handle_browser_warning()
         self.assertEnvironmentIsConnected()
@@ -222,7 +222,7 @@
         # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
         self.addCleanup(
             self.stop_services,
-            hostname, ['haproxy', 'nginx', 'juju-api-agent'])
+            hostname, ['haproxy', 'apache2', 'juju-api-agent'])
         self.navigate_to(hostname)
         self.handle_browser_warning()
         self.assertEnvironmentIsConnected()
 
=== modified file 'tests/test_utils.py'
--- tests/test_utils.py	2013-04-23 16:55:56 +0000
+++ tests/test_utils.py	2013-04-28 01:09:28 +0000
@@ -366,6 +366,15 @@
 
 class ParseSourceTest(unittest.TestCase):
 
+    def setUp(self):
+        # Monkey patch utils.CURRENT_DIR.
+        self.original_current_dir = utils.CURRENT_DIR
+        utils.CURRENT_DIR = '/current/dir'
+
+    def tearDown(self):
+        # Restore the original utils.CURRENT_DIR.
+        utils.CURRENT_DIR = self.original_current_dir
+
     def test_latest_stable_release(self):
         # Ensure the latest stable release is correctly parsed.
         expected = ('stable', None)
@@ -392,6 +401,19 @@
         for source in sources:
             self.assertTupleEqual(('branch', source), parse_source(source))
 
+    def test_url(self):
+        expected = ('url', 'http://example.com/gui')
+        self.assertTupleEqual(
+            expected, parse_source('url:http://example.com/gui'))
+
+    def test_file_url(self):
+        expected = ('url', 'file:///foo/bar')
+        self.assertTupleEqual(expected, parse_source('url:/foo/bar'))
+
+    def test_relative_file_url(self):
+        expected = ('url', 'file:///current/dir/foo/bar')
+        self.assertTupleEqual(expected, parse_source('url:foo/bar'))
+
 
 class RenderToFileTest(unittest.TestCase):
 
@@ -539,9 +561,11 @@
 
     def test_start_gui(self):
         ssl_cert_path = '/tmp/certificates/'
+        charmworld_url = 'http://charmworld.example'
         start_gui(
-            False, 'This is login help.', True, True, ssl_cert_path, True,
-            haproxy_path='haproxy', config_js_path='config')
+            False, 'This is login help.', True, True, ssl_cert_path,
+            charmworld_url, True, haproxy_path='haproxy',
+            config_js_path='config')
         haproxy_conf = self.files['haproxy']
         self.assertIn('ca-base {0}'.format(ssl_cert_path), haproxy_conf)
         self.assertIn('crt-base {0}'.format(ssl_cert_path), haproxy_conf)
@@ -558,6 +582,7 @@
         self.assertIn('readOnly: true', js_conf)
         self.assertIn("socket_url: 'wss://", js_conf)
         self.assertIn('socket_protocol: "wss"', js_conf)
+        self.assertIn('charmworldURL: "http://charmworld.example"', js_conf)
         apache_conf = self.files['juju-gui']
         self.assertIn('juju-gui/build-', apache_conf)
         self.assertIn('VirtualHost *:{0}'.format(WEB_PORT), apache_conf)
@@ -565,9 +590,11 @@
 
     def test_start_gui_insecure(self):
         ssl_cert_path = '/tmp/certificates/'
+        charmworld_url = 'http://charmworld.example'
         start_gui(
-            False, 'This is login help.', True, True, ssl_cert_path, True,
-            haproxy_path='haproxy', config_js_path='config', secure=False)
+            False, 'This is login help.', True, True, ssl_cert_path, 
+            charmworld_url, True, haproxy_path='haproxy',
+            config_js_path='config', secure=False)
         js_conf = self.files['config']
         self.assertIn("socket_url: 'ws://", js_conf)
         self.assertIn('socket_protocol: "ws"', js_conf)