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

Proposed by Gary Poster
Status: Merged
Merged at revision: 48
Proposed branch: lp:~gary/charms/precise/juju-gui/update-for-jujucharms
Merge into: lp:~juju-gui/charms/precise/juju-gui/trunk
Diff against target: 313 lines (+105/-20)
10 files modified
.bzrignore (+1/-0)
config.yaml (+6/-0)
config/config.js.template (+1/-0)
hooks/backend.py (+2/-2)
hooks/install (+23/-5)
hooks/utils.py (+13/-4)
hooks/web-relation-joined (+23/-0)
revision (+1/-1)
tests/deploy.test (+4/-4)
tests/test_utils.py (+31/-4)
To merge this branch: bzr merge lp:~gary/charms/precise/juju-gui/update-for-jujucharms
Reviewer Review Type Date Requested Status
charmers Pending
Review via email: mp+161298@code.launchpad.net

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :

Please take a look.

Revision history for this message
Gary Poster (gary) wrote :

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

Revision history for this message
Richard Harding (rharding) wrote :

LGTM, thanks for working through it.

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

51. By Gary Poster

respond to review

Revision history for this message
Gary Poster (gary) wrote :
Download full text (4.3 KiB)

*** 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...

Read more...

Revision history for this message
Gary Poster (gary) wrote :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '.bzrignore'
2--- .bzrignore 2013-04-15 17:47:54 +0000
3+++ .bzrignore 2013-04-28 01:09:28 +0000
4@@ -1,2 +1,3 @@
5 .emacs*
6 tags
7+exec.d/*
8
9=== modified file 'config.yaml'
10--- config.yaml 2013-04-17 19:31:56 +0000
11+++ config.yaml 2013-04-28 01:09:28 +0000
12@@ -112,3 +112,9 @@
13 the source: hooks/install and hooks/backend.py in particular.
14 type: boolean
15 default: true
16+ charmworld-url:
17+ description: |
18+ The URL of the charm catalog site ("charmworld") from which charm catalog
19+ data will be drawn.
20+ type: string
21+ default: http://staging.jujucharms.com
22
23=== modified file 'config/config.js.template'
24--- config/config.js.template 2013-04-12 10:09:32 +0000
25+++ config/config.js.template 2013-04-28 01:09:28 +0000
26@@ -11,6 +11,7 @@
27 // SubApps.
28 consoleEnabled: {{console_enabled}},
29 charm_store_url: 'https://jujucharms.com/',
30+ charmworldURL: {{charmworld_url}},
31 // socket_url is only honored in older versions of the GUI.
32 socket_url: '{{raw_protocol}}://{{address}}/ws',
33 // socket_protocol is used instead by newer versions of the GUI to
34
35=== added directory 'exec.d'
36=== modified file 'hooks/backend.py'
37--- hooks/backend.py 2013-04-24 13:18:22 +0000
38+++ hooks/backend.py 2013-04-28 01:09:28 +0000
39@@ -107,8 +107,8 @@
40 start_gui(
41 config['juju-gui-console-enabled'], config['login-help'],
42 config['read-only'], config['staging'], config['ssl-cert-path'],
43- config['serve-tests'], secure=config['secure'],
44- sandbox=config['sandbox'])
45+ config['charmworld-url'], config['serve-tests'],
46+ secure=config['secure'], sandbox=config['sandbox'])
47 open_port(80)
48 open_port(443)
49
50
51=== modified file 'hooks/install'
52--- hooks/install 2013-04-24 10:46:20 +0000
53+++ hooks/install 2013-04-28 01:09:28 +0000
54@@ -1,8 +1,9 @@
55 #!/usr/bin/env python2
56 # -*- python -*-
57
58+import errno
59 import json
60-from subprocess import check_call
61+import os
62
63 # If the user's environment has "juju-origin: ppa" set, they will
64 # automatically have access to python-charmhelpers and python-shelltoolbox.
65@@ -27,18 +28,35 @@
66 'python-apt', 'python-charmhelpers', 'python-launchpadlib',
67 'python-shelltoolbox', 'python-tempita',
68 )
69-check_call(('apt-get', 'install', '-y') + PYTHON_DEPENDENCIES)
70-
71-
72+bootstrap_utils.run(*(('apt-get', 'install', '-y') + PYTHON_DEPENDENCIES))
73+
74+
75+from charmhelpers import log
76 from utils import (
77 config_json,
78 log_hook,
79 )
80-
81 from backend import Backend
82
83
84 def main():
85+ # Run pre-install tasks, if available. Please do not rely on the
86+ # exec.d interface without conferring with the Juju GUI team: it may
87+ # change after upcoming discussion with Canonical IS.
88+ if os.path.isdir('exec.d'):
89+ dirnames = os.listdir('exec.d')
90+ dirnames.sort()
91+ for module in dirnames:
92+ filename = os.path.join('exec.d', module, 'charm-pre-install')
93+ try:
94+ bootstrap_utils.run(filename)
95+ except OSError, e:
96+ # If the exec.d file does not exist or is not runnable,
97+ # assume we can recover. Log the problem and proceed.
98+ if e.errno in (errno.ENOENT, errno.EACCES):
99+ log('{}: {}'.format(e.strerror, filename))
100+ else:
101+ raise
102 config = get_config()
103 backend = Backend(config)
104 backend.install()
105
106=== modified file 'hooks/utils.py'
107--- hooks/utils.py 2013-04-25 16:46:07 +0000
108+++ hooks/utils.py 2013-04-28 01:09:28 +0000
109@@ -45,6 +45,7 @@
110 import shutil
111 from subprocess import CalledProcessError
112 import tempfile
113+from urlparse import urlparse
114
115 from launchpadlib.launchpad import Launchpad
116 from shelltoolbox import (
117@@ -191,7 +192,7 @@
118
119 @contextmanager
120 def log_hook():
121- """Log when an hook starts and stops its execution.
122+ """Log when a hook starts and stops its execution.
123
124 Also log to stdout possible CalledProcessError exceptions raised executing
125 the hook.
126@@ -217,10 +218,17 @@
127 - ('stable', '0.1.0'): stable release v0.1.0;
128 - ('trunk', None): latest trunk release;
129 - ('trunk', '0.1.0+build.1'): trunk release v0.1.0 bzr revision 1;
130- - ('branch', 'lp:juju-gui'): release is made from a branch.
131+ - ('branch', 'lp:juju-gui'): release is made from a branch;
132+ - ('url', 'http://example.com/gui'): release from a downloaded file.
133 """
134 if source.startswith('url:'):
135- return 'url', source[4:]
136+ source = source[4:]
137+ # Support file paths, including relative paths.
138+ if urlparse(source).scheme == '':
139+ if not source.startswith('/'):
140+ source = os.path.join(os.path.abspath(CURRENT_DIR), source)
141+ source = "file://%s" % source
142+ return 'url', source
143 if source in ('stable', 'trunk'):
144 return source, None
145 if source.startswith('lp:') or source.startswith('http://'):
146@@ -310,7 +318,7 @@
147
148 def start_gui(
149 console_enabled, login_help, readonly, in_staging, ssl_cert_path,
150- serve_tests, haproxy_path='/etc/haproxy/haproxy.cfg',
151+ charmworld_url, serve_tests, haproxy_path='/etc/haproxy/haproxy.cfg',
152 config_js_path=None, secure=True, sandbox=False):
153 """Set up and start the Juju GUI server."""
154 with su('root'):
155@@ -350,6 +358,7 @@
156 'user': json.dumps(user),
157 'protocol': json.dumps(protocol),
158 'sandbox': json.dumps(sandbox),
159+ 'charmworld_url': json.dumps(charmworld_url),
160 }
161 if config_js_path is None:
162 config_js_path = os.path.join(
163
164=== added file 'hooks/web-relation-joined'
165--- hooks/web-relation-joined 1970-01-01 00:00:00 +0000
166+++ hooks/web-relation-joined 2013-04-28 01:09:28 +0000
167@@ -0,0 +1,23 @@
168+#!/usr/bin/env python2
169+#-*- python -*-
170+
171+import socket
172+from charmhelpers import (
173+ get_config,
174+ relation_set,
175+)
176+from utils import log_hook
177+
178+
179+def main():
180+ hostname = socket.getfqdn()
181+ config = get_config()
182+ if config['secure']:
183+ port = 443
184+ else:
185+ port = 80
186+ relation_set(port=port, hostname=hostname)
187+
188+if __name__ == '__main__':
189+ with log_hook():
190+ main()
191
192=== modified file 'revision'
193--- revision 2013-04-23 07:32:46 +0000
194+++ revision 2013-04-28 01:09:28 +0000
195@@ -1,1 +1,1 @@
196-34
197+40
198
199=== modified file 'tests/deploy.test'
200--- tests/deploy.test 2013-03-13 21:24:31 +0000
201+++ tests/deploy.test 2013-04-28 01:09:28 +0000
202@@ -166,7 +166,7 @@
203 # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
204 self.addCleanup(
205 self.stop_services,
206- hostname, ['haproxy', 'nginx', 'juju-api-agent'])
207+ hostname, ['haproxy', 'apache2', 'juju-api-agent'])
208 self.navigate_to(hostname)
209 self.handle_browser_warning()
210 self.assertEnvironmentIsConnected()
211@@ -177,7 +177,7 @@
212 # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
213 self.addCleanup(
214 self.stop_services,
215- hostname, ['haproxy', 'nginx', 'juju-api-improv'])
216+ hostname, ['haproxy', 'apache2', 'juju-api-improv'])
217 self.navigate_to(hostname)
218 self.handle_browser_warning()
219 self.assertEnvironmentIsConnected()
220@@ -190,7 +190,7 @@
221 # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
222 self.addCleanup(
223 self.stop_services,
224- hostname, ['haproxy', 'nginx', 'juju-api-agent'])
225+ hostname, ['haproxy', 'apache2', 'juju-api-agent'])
226 self.navigate_to(hostname)
227 self.handle_browser_warning()
228 self.assertEnvironmentIsConnected()
229@@ -222,7 +222,7 @@
230 # XXX 2012-11-29 frankban bug=872264: see *stop_services* above.
231 self.addCleanup(
232 self.stop_services,
233- hostname, ['haproxy', 'nginx', 'juju-api-agent'])
234+ hostname, ['haproxy', 'apache2', 'juju-api-agent'])
235 self.navigate_to(hostname)
236 self.handle_browser_warning()
237 self.assertEnvironmentIsConnected()
238
239=== modified file 'tests/test_utils.py'
240--- tests/test_utils.py 2013-04-23 16:55:56 +0000
241+++ tests/test_utils.py 2013-04-28 01:09:28 +0000
242@@ -366,6 +366,15 @@
243
244 class ParseSourceTest(unittest.TestCase):
245
246+ def setUp(self):
247+ # Monkey patch utils.CURRENT_DIR.
248+ self.original_current_dir = utils.CURRENT_DIR
249+ utils.CURRENT_DIR = '/current/dir'
250+
251+ def tearDown(self):
252+ # Restore the original utils.CURRENT_DIR.
253+ utils.CURRENT_DIR = self.original_current_dir
254+
255 def test_latest_stable_release(self):
256 # Ensure the latest stable release is correctly parsed.
257 expected = ('stable', None)
258@@ -392,6 +401,19 @@
259 for source in sources:
260 self.assertTupleEqual(('branch', source), parse_source(source))
261
262+ def test_url(self):
263+ expected = ('url', 'http://example.com/gui')
264+ self.assertTupleEqual(
265+ expected, parse_source('url:http://example.com/gui'))
266+
267+ def test_file_url(self):
268+ expected = ('url', 'file:///foo/bar')
269+ self.assertTupleEqual(expected, parse_source('url:/foo/bar'))
270+
271+ def test_relative_file_url(self):
272+ expected = ('url', 'file:///current/dir/foo/bar')
273+ self.assertTupleEqual(expected, parse_source('url:foo/bar'))
274+
275
276 class RenderToFileTest(unittest.TestCase):
277
278@@ -539,9 +561,11 @@
279
280 def test_start_gui(self):
281 ssl_cert_path = '/tmp/certificates/'
282+ charmworld_url = 'http://charmworld.example'
283 start_gui(
284- False, 'This is login help.', True, True, ssl_cert_path, True,
285- haproxy_path='haproxy', config_js_path='config')
286+ False, 'This is login help.', True, True, ssl_cert_path,
287+ charmworld_url, True, haproxy_path='haproxy',
288+ config_js_path='config')
289 haproxy_conf = self.files['haproxy']
290 self.assertIn('ca-base {0}'.format(ssl_cert_path), haproxy_conf)
291 self.assertIn('crt-base {0}'.format(ssl_cert_path), haproxy_conf)
292@@ -558,6 +582,7 @@
293 self.assertIn('readOnly: true', js_conf)
294 self.assertIn("socket_url: 'wss://", js_conf)
295 self.assertIn('socket_protocol: "wss"', js_conf)
296+ self.assertIn('charmworldURL: "http://charmworld.example"', js_conf)
297 apache_conf = self.files['juju-gui']
298 self.assertIn('juju-gui/build-', apache_conf)
299 self.assertIn('VirtualHost *:{0}'.format(WEB_PORT), apache_conf)
300@@ -565,9 +590,11 @@
301
302 def test_start_gui_insecure(self):
303 ssl_cert_path = '/tmp/certificates/'
304+ charmworld_url = 'http://charmworld.example'
305 start_gui(
306- False, 'This is login help.', True, True, ssl_cert_path, True,
307- haproxy_path='haproxy', config_js_path='config', secure=False)
308+ False, 'This is login help.', True, True, ssl_cert_path,
309+ charmworld_url, True, haproxy_path='haproxy',
310+ config_js_path='config', secure=False)
311 js_conf = self.files['config']
312 self.assertIn("socket_url: 'ws://", js_conf)
313 self.assertIn('socket_protocol: "ws"', js_conf)

Subscribers

People subscribed via source and target branches

to all changes: