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

Proposed by Brad Crittenden on 2013-11-14
Status: Merged
Merged at revision: 132
Proposed branch: lp:~bac/charms/precise/juju-gui/increment-deployments
Merge into: lp:~juju-gui/charms/precise/juju-gui/trunk
Diff against target: 794 lines (+315/-43)
15 files modified
config/guiserver.conf.template (+1/-0)
hooks/backend.py (+1/-1)
hooks/utils.py (+5/-3)
server/guiserver/apps.py (+2/-1)
server/guiserver/bundles/__init__.py (+8/-2)
server/guiserver/bundles/base.py (+19/-7)
server/guiserver/bundles/utils.py (+43/-0)
server/guiserver/bundles/views.py (+15/-7)
server/guiserver/manage.py (+4/-0)
server/guiserver/tests/bundles/test_base.py (+95/-14)
server/guiserver/tests/bundles/test_utils.py (+79/-0)
server/guiserver/tests/bundles/test_views.py (+28/-1)
server/runserver.py (+1/-0)
tests/test_backends.py (+5/-3)
tests/test_utils.py (+9/-4)
To merge this branch: bzr merge lp:~bac/charms/precise/juju-gui/increment-deployments
Reviewer Review Type Date Requested Status
charmers 2013-11-14 Pending
Review via email: mp+195317@code.launchpad.net

Description of the change

After deploying a bundle increment counter.

The guiserver will make a GET request to the deployment counter incrementer
URL for the bundle. This required accepting the bundle ID in the
Deployer/Import path.

Note neither the GUI nor quickstart have been updated to pass this value yet.

The charmworldurl is also now passed to the server via the command line.

QA: just deploy your favorite bundle and see that it works as it should. You
can observe the bundle page in charmworld and see the counts remain at 0.

https://codereview.appspot.com/26740043/

To post a comment you must log in.
Brad Crittenden (bac) wrote :

Reviewers: mp+195317_code.launchpad.net,

Message:
Please take a look.

Description:
After deploying a bundle increment counter.

The guiserver will make a GET request to the deployment counter
incrementer
URL for the bundle. This required accepting the bundle ID in the
Deployer/Import path.

Note neither the GUI nor quickstart have been updated to pass this value
yet.

The charmworldurl is also now passed to the server via the command line.

QA: just deploy your favorite bundle and see that it works as it should.
  You
can observe the bundle page in charmworld and see the counts remain at
0.

https://code.launchpad.net/~bac/charms/precise/juju-gui/increment-deployments/+merge/195317

(do not edit description out of merge proposal)

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

Affected files (+217, -41 lines):
   A [revision details]
   M config/guiserver.conf.template
   M hooks/backend.py
   M hooks/utils.py
   M server/guiserver/apps.py
   M server/guiserver/bundles/__init__.py
   M server/guiserver/bundles/base.py
   M server/guiserver/bundles/utils.py
   M server/guiserver/bundles/views.py
   M server/guiserver/manage.py
   M server/guiserver/tests/bundles/test_base.py
   M server/guiserver/tests/bundles/test_utils.py
   M server/guiserver/tests/bundles/test_views.py
   M server/runserver.py
   M tests/test_backends.py
   M tests/test_utils.py

Francesco Banconi (frankban) wrote :
Download full text (6.5 KiB)

This branch looks very nice Brad.
LGTM with changes. I added a lot of comments below, but they are mostly
minors/nice to have, except for some documentation requests, code
removal requests, and a possible string encoding problem. I will QA this
code when the changes are pushed later. Thank you!

https://codereview.appspot.com/26740043/diff/1/server/guiserver/apps.py
File server/guiserver/apps.py (right):

https://codereview.appspot.com/26740043/diff/1/server/guiserver/apps.py#newcode43
server/guiserver/apps.py:43: options.charmworldurl)
It would be nice to expose the charmworld URL also in the
/gui-server-info InfoHandler. That's just a nice to have for another
card I guess.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/__init__.py
File server/guiserver/bundles/__init__.py (right):

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/__init__.py#newcode99
server/guiserver/bundles/__init__.py:99: },
In the other parts of this docstring, we aligned the closing brace of
the value with the key. <shrug>

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/__init__.py#newcode105
server/guiserver/bundles/__init__.py:105: parameter is optional in the
case YAML includes only one bundle.
Please document that the BundleID is completely optional, what that
value means, what it is used for and how it looks like (e.g. I guess it
must not start with "bundle:" because that's a bundle URL, but better
not to guess).

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base.py
File server/guiserver/bundles/base.py (right):

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base.py#newcode99
server/guiserver/bundles/base.py:99: self.bundle_ids = {}
Is bundle_ids really required? ISTM it's not really used.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base.py#newcode125
server/guiserver/bundles/base.py:125: def import_bundle(self, user,
name, bundle, bundle_id, test_callback=None):
It would be nice to make bundle_id explicitly optional (i.e.
bundle_id=None here).

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base.py#newcode159
server/guiserver/bundles/base.py:159: self.bundle_ids[deployment_id] =
bundle_id
AFAICT these two lines can be removed as well.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base.py#newcode190
server/guiserver/bundles/base.py:190: # Increment the Charmworld
deployment count upon successful
The count is increased also if the deployment failed. It seems ok to me,
but we should fix the comment.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base.py#newcode192
server/guiserver/bundles/base.py:192: if bundle_id is not None:
Nice and simple.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils.py
File server/guiserver/bundles/utils.py (right):

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils.py#newcode25
server/guiserver/bundles/utils.py:25: from tornado.httpclient import
AsyncHTTPClient
Very very minor: could you please move this import after the "from
tornado import..." one? O...

Read more...

135. By Brad Crittenden on 2013-11-15

Clean up tests and code from review

Brad Crittenden (bac) wrote :
Download full text (5.7 KiB)

Please take a look.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/apps.py
File server/guiserver/apps.py (right):

https://codereview.appspot.com/26740043/diff/1/server/guiserver/apps.py#newcode43
server/guiserver/apps.py:43: options.charmworldurl)
Good idea but I'd rather not do it in this branch.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/__init__.py
File server/guiserver/bundles/__init__.py (right):

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/__init__.py#newcode99
server/guiserver/bundles/__init__.py:99: },
On 2013/11/15 08:48:20, frankban wrote:
> In the other parts of this docstring, we aligned the closing brace of
the value
> with the key. <shrug>

Done.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/__init__.py#newcode105
server/guiserver/bundles/__init__.py:105: parameter is optional in the
case YAML includes only one bundle.
On 2013/11/15 08:48:20, frankban wrote:
> Please document that the BundleID is completely optional, what that
value means,
> what it is used for and how it looks like (e.g. I guess it must not
start with
> "bundle:" because that's a bundle URL, but better not to guess).

Done.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base.py
File server/guiserver/bundles/base.py (right):

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base.py#newcode99
server/guiserver/bundles/base.py:99: self.bundle_ids = {}
Good catch. I stopped using it when I began passing it directly to
add_future.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base.py#newcode125
server/guiserver/bundles/base.py:125: def import_bundle(self, user,
name, bundle, bundle_id, test_callback=None):
I don't see the reason. It is optional in the params payload and will
get passed as None if there. import_bundle is not called through any
other path.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base.py#newcode159
server/guiserver/bundles/base.py:159: self.bundle_ids[deployment_id] =
bundle_id
On 2013/11/15 08:48:20, frankban wrote:
> AFAICT these two lines can be removed as well.

Done.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base.py#newcode190
server/guiserver/bundles/base.py:190: # Increment the Charmworld
deployment count upon successful
I'd really like to only increment on success. I'll add a check for
error is None.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils.py
File server/guiserver/bundles/utils.py (right):

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils.py#newcode25
server/guiserver/bundles/utils.py:25: from tornado.httpclient import
AsyncHTTPClient
On 2013/11/15 08:48:20, frankban wrote:
> Very very minor: could you please move this import after the "from
tornado
> import..." one? Or just add "httpclient" to the list below?
> <shrug>

Done.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils.py#newcode229
server/guiserver/bundles/utils.py:229: url =
'{}api/3/bundle/{}/{}'.format(charmworld_url, bundle_id, path)
On 2013/11...

Read more...

Francesco Banconi (frankban) wrote :

Just one more comment, I'll QA asap.

https://codereview.appspot.com/26740043/diff/20001/server/guiserver/bundles/base.py
File server/guiserver/bundles/base.py (right):

https://codereview.appspot.com/26740043/diff/20001/server/guiserver/bundles/base.py#newcode188
server/guiserver/bundles/base.py:188: if bundle_id is not None and error
is None:
I the future has been cancelled then error is not defined here.

https://codereview.appspot.com/26740043/

136. By Brad Crittenden on 2013-11-15

Merge from trunk

137. By Brad Crittenden on 2013-11-15

Fix success criteria for incrementing deployment counter.

138. By Brad Crittenden on 2013-11-15

lint

139. By Brad Crittenden on 2013-11-15

Fix _import_callback error and test the method.

140. By Brad Crittenden on 2013-11-15

lint

Brad Crittenden (bac) wrote :

*** Submitted:

After deploying a bundle increment counter.

The guiserver will make a GET request to the deployment counter
incrementer
URL for the bundle. This required accepting the bundle ID in the
Deployer/Import path.

Note neither the GUI nor quickstart have been updated to pass this value
yet.

The charmworldurl is also now passed to the server via the command line.

QA: just deploy your favorite bundle and see that it works as it should.
  You
can observe the bundle page in charmworld and see the counts remain at
0.

R=frankban
CC=
https://codereview.appspot.com/26740043

https://codereview.appspot.com/26740043/diff/20001/server/guiserver/bundles/base.py
File server/guiserver/bundles/base.py (right):

https://codereview.appspot.com/26740043/diff/20001/server/guiserver/bundles/base.py#newcode188
server/guiserver/bundles/base.py:188: if bundle_id is not None and error
is None:
On 2013/11/15 14:42:03, frankban wrote:
> I the future has been cancelled then error is not defined here.

Done.

https://codereview.appspot.com/26740043/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'config/guiserver.conf.template'
2--- config/guiserver.conf.template 2013-08-26 12:18:19 +0000
3+++ config/guiserver.conf.template 2013-11-15 18:31:21 +0000
4@@ -8,6 +8,7 @@
5 --logging="{{builtin_server_logging}}" \
6 --guiroot="{{gui_root}}" \
7 --sslpath="{{ssl_cert_path}}" \
8+ --charmworldurl="{{charmworld_url}}" \
9 {{if sandbox}}
10 --sandbox \
11 {{else}}
12
13=== modified file 'hooks/backend.py'
14--- hooks/backend.py 2013-11-13 18:01:03 +0000
15+++ hooks/backend.py 2013-11-15 18:31:21 +0000
16@@ -216,7 +216,7 @@
17 utils.start_builtin_server(
18 build_dir, config['ssl-cert-path'], config['serve-tests'],
19 config['sandbox'], config['builtin-server-logging'],
20- not config['secure'])
21+ not config['secure'], config['charmworld-url'])
22
23 def stop(self, backend):
24 utils.stop_builtin_server()
25
26=== modified file 'hooks/utils.py'
27--- hooks/utils.py 2013-11-14 19:10:28 +0000
28+++ hooks/utils.py 2013-11-15 18:31:21 +0000
29@@ -557,7 +557,7 @@
30
31 def write_builtin_server_startup(
32 gui_root, ssl_cert_path, serve_tests=False, sandbox=False,
33- builtin_server_logging='info', insecure=False):
34+ builtin_server_logging='info', insecure=False, charmworld_url=''):
35 """Generate the builtin server Upstart file."""
36 log('Generating the builtin server Upstart file.')
37 context = {
38@@ -567,6 +567,7 @@
39 'sandbox': sandbox,
40 'serve_tests': serve_tests,
41 'ssl_cert_path': ssl_cert_path,
42+ 'charmworld_url': charmworld_url,
43 }
44 if not sandbox:
45 is_legacy_juju = legacy_juju()
46@@ -586,11 +587,12 @@
47
48 def start_builtin_server(
49 build_dir, ssl_cert_path, serve_tests, sandbox, builtin_server_logging,
50- insecure):
51+ insecure, charmworld_url):
52 """Start the builtin server."""
53 write_builtin_server_startup(
54 build_dir, ssl_cert_path, serve_tests=serve_tests, sandbox=sandbox,
55- builtin_server_logging=builtin_server_logging, insecure=insecure)
56+ builtin_server_logging=builtin_server_logging, insecure=insecure,
57+ charmworld_url=charmworld_url)
58 log('Starting the builtin server.')
59 with su('root'):
60 service_control(BUILTIN_SERVER, RESTART)
61
62=== modified file 'server/guiserver/apps.py'
63--- server/guiserver/apps.py 2013-08-26 13:10:57 +0000
64+++ server/guiserver/apps.py 2013-11-15 18:31:21 +0000
65@@ -39,7 +39,8 @@
66 guiroot = options.guiroot
67 static_path = os.path.join(guiroot, 'juju-ui')
68 # Set up the bundle deployer.
69- deployer = Deployer(options.apiurl, options.apiversion)
70+ deployer = Deployer(options.apiurl, options.apiversion,
71+ options.charmworldurl)
72 # Set up handlers.
73 server_handlers = []
74 if not options.sandbox:
75
76=== modified file 'server/guiserver/bundles/__init__.py'
77--- server/guiserver/bundles/__init__.py 2013-09-23 14:45:58 +0000
78+++ server/guiserver/bundles/__init__.py 2013-11-15 18:31:21 +0000
79@@ -93,13 +93,19 @@
80 'RequestId': 1,
81 'Type': 'Deployer',
82 'Request': 'Import',
83- 'Params': {'Name': 'bundle-name', 'YAML': 'bundles'},
84+ 'Params': {
85+ 'Name': 'bundle-name',
86+ 'YAML': 'bundles',
87+ 'BundleID': 'id'
88+ },
89 }
90
91 In the request parameters above, the YAML field stores the YAML encoded
92 contents representing one or more bundles, and the Name field is the name of
93 the specific bundle (included in YAML) that must be deployed. The Name
94-parameter is optional in the case YAML includes only one bundle.
95+parameter is optional in the case YAML includes only one bundle. The BundleID
96+is optional and is used for incrementing the deployment counter in
97+Charmworld.
98
99 After receiving a deployment request, the DeployMiddleware sends a response
100 indicating whether or not the request has been accepted. This response is sent
101
102=== modified file 'server/guiserver/bundles/base.py'
103--- server/guiserver/bundles/base.py 2013-11-07 11:17:54 +0000
104+++ server/guiserver/bundles/base.py 2013-11-15 18:31:21 +0000
105@@ -69,7 +69,7 @@
106 singleton by all WebSocket requests.
107 """
108
109- def __init__(self, apiurl, apiversion, io_loop=None):
110+ def __init__(self, apiurl, apiversion, charmworldurl=None, io_loop=None):
111 """Initialize the deployer.
112
113 The apiurl argument is the URL of the juju-core WebSocket server.
114@@ -77,6 +77,9 @@
115 """
116 self._apiurl = apiurl
117 self._apiversion = apiversion
118+ if charmworldurl is not None and not charmworldurl.endswith('/'):
119+ charmworldurl = charmworldurl + '/'
120+ self._charmworldurl = charmworldurl
121 if io_loop is None:
122 io_loop = IOLoop.current()
123 self._io_loop = io_loop
124@@ -117,15 +120,16 @@
125 except Exception as err:
126 raise gen.Return(str(err))
127
128- def import_bundle(self, user, name, bundle, test_callback=None):
129+ def import_bundle(self, user, name, bundle, bundle_id, test_callback=None):
130 """Schedule a deployment bundle import process.
131
132 The deployment is executed in a separate process.
133
134- Three arguments are required:
135+ The following arguments are required:
136 - user: the current authenticated user;
137- - name: then name of the bundle to be imported;
138+ - name: the name of the bundle to be imported;
139 - bundle: a YAML decoded object representing the bundle contents.
140+ - bundle_id: the ID of the bundle. May be None.
141
142 It is possible to also provide an optional test_callback that will be
143 called when the deployment is completed. Note that this functionality
144@@ -146,7 +150,8 @@
145 future = self._run_executor.submit(
146 blocking.import_bundle,
147 self._apiurl, user.password, name, bundle, IMPORTER_OPTIONS)
148- add_future(self._io_loop, future, self._import_callback, deployment_id)
149+ add_future(self._io_loop, future, self._import_callback,
150+ deployment_id, bundle_id)
151 self._futures[deployment_id] = future
152 # If a customized callback is provided, schedule it as well.
153 if test_callback is not None:
154@@ -157,7 +162,7 @@
155 self._run_executor.submit(time.sleep, 1)
156 return deployment_id
157
158- def _import_callback(self, deployment_id, future):
159+ def _import_callback(self, deployment_id, bundle_id, future):
160 """Callback called when a deployment process is completed.
161
162 This callback, scheduled in self.import_bundle(), receives the
163@@ -167,17 +172,24 @@
164 if future.cancelled():
165 # Notify a deployment has been cancelled.
166 self._observer.notify_cancelled(deployment_id)
167+ success = False
168 else:
169 exception = future.exception()
170 error = None if exception is None else str(exception)
171 # Notify a deployment completed.
172 self._observer.notify_completed(deployment_id, error=error)
173+ success = (error is None)
174 # Remove the completed deployment job from the queue.
175 self._queue.remove(deployment_id)
176 del self._futures[deployment_id]
177 # Notify the new position of all remaining deployments in the queue.
178 for position, deploy_id in enumerate(self._queue):
179 self._observer.notify_position(deploy_id, position)
180+ # Increment the Charmworld deployment count upon successful
181+ # deployment.
182+ if success and bundle_id is not None:
183+ utils.increment_deployment_counter(
184+ bundle_id, self._charmworldurl)
185
186 def watch(self, deployment_id):
187 """Start watching a deployment and return a watcher identifier.
188@@ -232,7 +244,7 @@
189
190 This class handles the process of parsing requests from the GUI, checking
191 if any incoming message is a deployment request, ensuring that the request
192- is well formed and, if so, forwarding the requests to the bundle views.
193+ is well-formed and, if so, forwarding the requests to the bundle views.
194
195 Assuming that:
196 - user is a guiserver.auth.User instance (used by this middleware in
197
198=== modified file 'server/guiserver/bundles/utils.py'
199--- server/guiserver/bundles/utils.py 2013-11-07 13:55:56 +0000
200+++ server/guiserver/bundles/utils.py 2013-11-15 18:31:21 +0000
201@@ -21,11 +21,13 @@
202 import itertools
203 import logging
204 import time
205+import urllib
206
207 from tornado import (
208 gen,
209 escape,
210 )
211+from tornado.httpclient import AsyncHTTPClient
212
213 from guiserver.watchers import AsyncWatcher
214
215@@ -204,3 +206,44 @@
216 logging.error('deployer: {}'.format(escape.utf8(error)))
217 data['Error'] = error
218 return gen.Return(data)
219+
220+
221+@gen.coroutine
222+def increment_deployment_counter(bundle_id, charmworld_url):
223+ """Increment the deployment count in Charmworld.
224+
225+ If the call to Charmworld fails we log the error but don't report it.
226+ This counter is a 'best effort' attempt but it will not impede our
227+ deployment of the bundle.
228+
229+ Arguments are:
230+ - bundle_id: the ID for the bundle in Charmworld.
231+ - charmworld_url: the URL for charmworld, including the protocol.
232+ If None, do nothing.
233+
234+ Returns True if the counter is successfully incremented else False.
235+ """
236+ if charmworld_url is None:
237+ raise gen.Return(False)
238+
239+ if not all((isinstance(bundle_id, basestring),
240+ isinstance(charmworld_url, basestring))):
241+ raise gen.Return(False)
242+
243+ path = 'metric/deployments/increment'
244+ url = u'{}api/3/bundle/{}/{}'.format(
245+ charmworld_url,
246+ urllib.quote(bundle_id), path)
247+ logging.info('Incrementing bundle deployment count using\n{}.'.format(
248+ url.encode('utf-8')))
249+ client = AsyncHTTPClient()
250+ # We use a GET instead of a POST since there is not request body.
251+ try:
252+ resp = yield client.fetch(url, callback=None)
253+ except Exception as exc:
254+ logging.error('Attempt to increment deployment counter failed.')
255+ logging.error('URL: {}'.format(url))
256+ logging.exception(exc)
257+ raise gen.Return(False)
258+ success = bool(resp.code == 200)
259+ raise gen.Return(success)
260
261=== modified file 'server/guiserver/bundles/views.py'
262--- server/guiserver/bundles/views.py 2013-11-07 11:17:54 +0000
263+++ server/guiserver/bundles/views.py 2013-11-15 18:31:21 +0000
264@@ -71,11 +71,15 @@
265
266
267 def _validate_import_params(params):
268- """Parse the request data and return a (name, bundle) tuple.
269+ """Parse the request data and return a (name, bundle, bundle_id) tuple.
270
271 In the tuple:
272 - name is the name of the bundle to be imported;
273 - bundle is the YAML decoded bundle object.
274+ - bundle_id is the permanent id of the bundle of the form
275+ ~user/basketname/version/bundlename, e.g.
276+ ~jorge/mediawiki/3/mediawiki-simple. The bundle_id is optional and
277+ will be None if not given.
278
279 Raise a ValueError if data represents an invalid request.
280 """
281@@ -88,14 +92,17 @@
282 raise ValueError('invalid YAML contents: {}'.format(err))
283 name = params.get('Name')
284 if name is None:
285- # The Name is optional if the YAML contents contain only one bunlde.
286+ # The Name is optional if the YAML contents contain only one bundle.
287 if len(bundles) == 1:
288- return bundles.items()[0]
289- raise ValueError('invalid data parameters: no bundle name provided')
290+ name = bundles.keys()[0]
291+ else:
292+ raise ValueError(
293+ 'invalid data parameters: no bundle name provided')
294 bundle = bundles.get(name)
295 if bundle is None:
296 raise ValueError('bundle {} not found'.format(name))
297- return name, bundle
298+ bundle_id = params.get('BundleID')
299+ return name, bundle, bundle_id
300
301
302 @gen.coroutine
303@@ -111,7 +118,7 @@
304 """
305 # Validate the request parameters.
306 try:
307- name, bundle = _validate_import_params(request.params)
308+ name, bundle, bundle_id = _validate_import_params(request.params)
309 except ValueError as err:
310 raise response(error='invalid request: {}'.format(err))
311 # Validate and prepare the bundle.
312@@ -125,7 +132,8 @@
313 if err is not None:
314 raise response(error='invalid request: {}'.format(err))
315 # Add the bundle deployment to the Deployer queue.
316- deployment_id = deployer.import_bundle(request.user, name, bundle)
317+ deployment_id = deployer.import_bundle(
318+ request.user, name, bundle, bundle_id)
319 raise response({'DeploymentId': deployment_id})
320
321
322
323=== modified file 'server/guiserver/manage.py'
324--- server/guiserver/manage.py 2013-08-26 11:27:32 +0000
325+++ server/guiserver/manage.py 2013-11-15 18:31:21 +0000
326@@ -116,6 +116,10 @@
327 'an in-memory backend. When this is set to True, the GUI server '
328 'does not listen to incoming WebSocket connections, and '
329 'therefore the --apiurl and --apiversion options are ignored.')
330+ define(
331+ 'charmworldurl', type=str,
332+ help='The URL to use for Charmworld.')
333+
334 # In Tornado, parsing the options also sets up the default logger.
335 parse_command_line()
336 _validate_required('guiroot')
337
338=== modified file 'server/guiserver/tests/bundles/test_base.py'
339--- server/guiserver/tests/bundles/test_base.py 2013-11-07 17:27:20 +0000
340+++ server/guiserver/tests/bundles/test_base.py 2013-11-15 18:31:21 +0000
341@@ -42,6 +42,18 @@
342 raise jujuclient.EnvError({'Error': 'bad wolf'})
343
344
345+class FakeFuture(object):
346+ def __init__(self, cancelled=False, exception=None):
347+ self._cancelled = cancelled
348+ self._exception = exception
349+
350+ def cancelled(self):
351+ return self._cancelled
352+
353+ def exception(self):
354+ return self._exception
355+
356+
357 @mock.patch('time.time', mock.Mock(return_value=42))
358 class TestDeployer(helpers.BundlesTestMixin, AsyncTestCase):
359
360@@ -108,7 +120,8 @@
361 deployer = self.make_deployer()
362 with self.patch_import_bundle():
363 deployment_id = deployer.import_bundle(
364- self.user, 'bundle', self.bundle, test_callback=self.stop)
365+ self.user, 'bundle', self.bundle, bundle_id=None,
366+ test_callback=self.stop)
367 self.assertIsInstance(deployment_id, int)
368 # Wait for the deployment to be completed.
369 self.wait()
370@@ -118,7 +131,8 @@
371 deployer = self.make_deployer()
372 with self.patch_import_bundle() as mock_import_bundle:
373 deployer.import_bundle(
374- self.user, 'bundle', self.bundle, test_callback=self.stop)
375+ self.user, 'bundle', self.bundle, bundle_id=None,
376+ test_callback=self.stop)
377 # Wait for the deployment to be completed.
378 self.wait()
379 mock_import_bundle.assert_called_once_with(
380@@ -140,7 +154,8 @@
381 deployer = self.make_deployer()
382 with self.patch_import_bundle():
383 deployment_id = deployer.import_bundle(
384- self.user, 'bundle', self.bundle, test_callback=self.stop)
385+ self.user, 'bundle', self.bundle, bundle_id=None,
386+ test_callback=self.stop)
387 watcher_id = deployer.watch(deployment_id)
388 self.assertIsInstance(watcher_id, int)
389 # Wait for the deployment to be completed.
390@@ -157,7 +172,8 @@
391 deployer = self.make_deployer()
392 with self.patch_import_bundle():
393 deployment_id = deployer.import_bundle(
394- self.user, 'bundle', self.bundle, test_callback=self.stop)
395+ self.user, 'bundle', self.bundle, bundle_id=None,
396+ test_callback=self.stop)
397 watcher_id = deployer.watch(deployment_id)
398 # A first change is received notifying that the deployment is started.
399 changes = yield deployer.next(watcher_id)
400@@ -178,9 +194,10 @@
401 deployer = self.make_deployer()
402 with self.patch_import_bundle():
403 deployment1 = deployer.import_bundle(
404- self.user, 'bundle', self.bundle)
405+ self.user, 'bundle', self.bundle, bundle_id=None)
406 deployment2 = deployer.import_bundle(
407- self.user, 'bundle', self.bundle, test_callback=self.stop)
408+ self.user, 'bundle', self.bundle, bundle_id=None,
409+ test_callback=self.stop)
410 watcher1 = deployer.watch(deployment1)
411 watcher2 = deployer.watch(deployment2)
412 # The first deployment is started.
413@@ -205,7 +222,8 @@
414 deployer = self.make_deployer()
415 with self.patch_import_bundle(side_effect=RuntimeError('bad wolf')):
416 deployment_id = deployer.import_bundle(
417- self.user, 'bundle', self.bundle, test_callback=self.stop)
418+ self.user, 'bundle', self.bundle, bundle_id=None,
419+ test_callback=self.stop)
420 watcher_id = deployer.watch(deployment_id)
421 # We expect two changes: the second one should include the error.
422 yield deployer.next(watcher_id)
423@@ -222,7 +240,8 @@
424 import_bundle_path = 'guiserver.bundles.base.blocking.import_bundle'
425 with mock.patch(import_bundle_path, import_bundle_mock):
426 deployer.import_bundle(
427- self.user, 'bundle', self.bundle, test_callback=self.stop)
428+ self.user, 'bundle', self.bundle, bundle_id=None,
429+ test_callback=self.stop)
430 # Wait for the deployment to be completed.
431 self.wait()
432 status = deployer.status()
433@@ -249,9 +268,10 @@
434 # The test callback is passed to the first deployment because we
435 # expect the second one to be immediately cancelled.
436 deployer.import_bundle(
437- self.user, 'bundle', self.bundle, test_callback=self.stop)
438+ self.user, 'bundle', self.bundle, bundle_id=None,
439+ test_callback=self.stop)
440 deployment_id = deployer.import_bundle(
441- self.user, 'bundle', self.bundle)
442+ self.user, 'bundle', self.bundle, bundle_id=None)
443 watcher_id = deployer.watch(deployment_id)
444 self.assertIsNone(deployer.cancel(deployment_id))
445 # We expect two changes: the second one should notify the deployment
446@@ -274,7 +294,8 @@
447 deployer = self.make_deployer()
448 with self.patch_import_bundle():
449 deployment_id = deployer.import_bundle(
450- self.user, 'bundle', self.bundle, test_callback=self.stop)
451+ self.user, 'bundle', self.bundle, bundle_id=None,
452+ test_callback=self.stop)
453 watcher_id = deployer.watch(deployment_id)
454 # Assume the deployment is completed after two changes.
455 yield deployer.next(watcher_id)
456@@ -291,7 +312,8 @@
457 deployer = self.make_deployer()
458 with self.patch_import_bundle() as mock_import_bundle:
459 deployment_id = deployer.import_bundle(
460- self.user, 'bundle', self.bundle, test_callback=self.stop)
461+ self.user, 'bundle', self.bundle, bundle_id=None,
462+ test_callback=self.stop)
463 watcher_id = deployer.watch(deployment_id)
464 # Wait until the deployment is started.
465 yield deployer.next(watcher_id)
466@@ -314,9 +336,10 @@
467 deployer = self.make_deployer()
468 with self.patch_import_bundle():
469 deployment1 = deployer.import_bundle(
470- self.user, 'bundle', self.bundle)
471+ self.user, 'bundle', self.bundle, bundle_id=None)
472 deployment2 = deployer.import_bundle(
473- self.user, 'bundle', self.bundle, test_callback=self.stop)
474+ self.user, 'bundle', self.bundle, bundle_id=None,
475+ test_callback=self.stop)
476 # Wait for the deployment to be completed.
477 self.wait()
478 # At this point we expect two completed deployments.
479@@ -326,6 +349,64 @@
480 self.assertEqual(deployment1, change1['DeploymentId'])
481 self.assertEqual(deployment2, change2['DeploymentId'])
482
483+ def test_import_callback_cancelled(self):
484+ deployer = self.make_deployer()
485+ deployer_id = 123
486+ deployer._queue.append(deployer_id)
487+ deployer._futures[deployer_id] = None
488+ mock_path = 'guiserver.bundles.utils.increment_deployment_counter'
489+ future = FakeFuture(True)
490+ with mock.patch.object(
491+ deployer._observer, 'notify_cancelled') as mock_notify:
492+ with mock.patch(mock_path) as mock_incrementer:
493+ deployer._import_callback(deployer_id, None, future)
494+ mock_notify.assert_called_with(deployer_id)
495+ self.assertFalse(mock_incrementer.called)
496+
497+ def test_import_callback_error(self):
498+ deployer = self.make_deployer()
499+ deployer_id = 123
500+ deployer._queue.append(deployer_id)
501+ deployer._futures[deployer_id] = None
502+ mock_path = 'guiserver.bundles.utils.increment_deployment_counter'
503+ future = FakeFuture(exception='aiiee')
504+ with mock.patch.object(
505+ deployer._observer, 'notify_completed') as mock_notify:
506+ with mock.patch(mock_path) as mock_incrementer:
507+ deployer._import_callback(deployer_id, None, future)
508+ mock_notify.assert_called_with(deployer_id, error='aiiee')
509+ self.assertFalse(mock_incrementer.called)
510+
511+ def test_import_callback_no_bundleid(self):
512+ deployer = self.make_deployer()
513+ deployer_id = 123
514+ deployer._queue.append(deployer_id)
515+ deployer._futures[deployer_id] = None
516+ mock_path = 'guiserver.bundles.utils.increment_deployment_counter'
517+ future = FakeFuture()
518+ with mock.patch.object(
519+ deployer._observer, 'notify_completed') as mock_notify:
520+ with mock.patch(mock_path) as mock_incrementer:
521+ deployer._import_callback(deployer_id, None, future)
522+ mock_notify.assert_called_with(deployer_id, error=None)
523+ self.assertFalse(mock_incrementer.called)
524+
525+ def test_import_callback_success(self):
526+ deployer = self.make_deployer()
527+ deployer_id = 123
528+ bundle_id = '~jorge/basket/bundle'
529+ deployer._charmworldurl = 'http://cw.example.com'
530+ deployer._queue.append(deployer_id)
531+ deployer._futures[deployer_id] = None
532+ mock_path = 'guiserver.bundles.utils.increment_deployment_counter'
533+ future = FakeFuture()
534+ with mock.patch.object(
535+ deployer._observer, 'notify_completed') as mock_notify:
536+ with mock.patch(mock_path) as mock_incrementer:
537+ deployer._import_callback(deployer_id, bundle_id, future)
538+ mock_notify.assert_called_with(deployer_id, error=None)
539+ mock_incrementer.assert_called_with(bundle_id, deployer._charmworldurl)
540+
541
542 class TestDeployMiddleware(helpers.BundlesTestMixin, AsyncTestCase):
543
544
545=== modified file 'server/guiserver/tests/bundles/test_utils.py'
546--- server/guiserver/tests/bundles/test_utils.py 2013-11-07 13:55:56 +0000
547+++ server/guiserver/tests/bundles/test_utils.py 2013-11-15 18:31:21 +0000
548@@ -18,6 +18,7 @@
549
550 import unittest
551
552+from concurrent.futures import Future
553 import mock
554 from tornado import gen
555 from tornado.testing import(
556@@ -26,6 +27,7 @@
557 gen_test,
558 LogTrapTestCase,
559 )
560+import urllib
561
562 from guiserver import watchers
563 from guiserver.bundles import utils
564@@ -422,3 +424,80 @@
565 # An error log is written when a failure response is generated.
566 with ExpectLog('', 'deployer: an error occurred', required=True):
567 utils.response(error='an error occurred')
568+
569+
570+def mock_fetch_factory(response_code, called_with=None):
571+ def fetch(*args, **kwargs):
572+ if called_with is not None:
573+ called_with.append((args[1:], kwargs))
574+
575+ class FakeResponse(object):
576+ pass
577+
578+ resp = FakeResponse()
579+ resp.code = response_code
580+ future = Future()
581+ future.set_result(resp)
582+ return future
583+ return fetch
584+
585+
586+class TestIncrementDeploymentCounter(LogTrapTestCase, AsyncTestCase):
587+
588+ @gen_test
589+ def test_no_cw_url_returns_true(self):
590+ bundle_id = '~bac/muletrain/wiki'
591+ mock_path = 'tornado.httpclient.AsyncHTTPClient.fetch'
592+ with mock.patch(mock_path) as mock_fetch:
593+ ok = yield utils.increment_deployment_counter(bundle_id, None)
594+ self.assertFalse(ok)
595+ self.assertFalse(mock_fetch.called)
596+
597+ @gen_test
598+ def test_increment_nonstring_bundle_id(self):
599+ bundle_id = 4
600+ cw_url = 'http://my.charmworld.example.com/'
601+ mock_path = 'tornado.httpclient.AsyncHTTPClient.fetch'
602+ with mock.patch(mock_path) as mock_fetch:
603+ ok = yield utils.increment_deployment_counter(bundle_id, cw_url)
604+ self.assertFalse(ok)
605+ self.assertFalse(mock_fetch.called)
606+
607+ @gen_test
608+ def test_increment_nonstring_cwurl(self):
609+ bundle_id = u'~bac/muletrain/wiki'
610+ cw_url = 7
611+ mock_path = 'tornado.httpclient.AsyncHTTPClient.fetch'
612+ with mock.patch(mock_path) as mock_fetch:
613+ ok = yield utils.increment_deployment_counter(bundle_id, cw_url)
614+ self.assertFalse(ok)
615+ self.assertFalse(mock_fetch.called)
616+
617+ @gen_test
618+ def test_increment_url_logged(self):
619+ bundle_id = '~bac/muletrain/wiki'
620+ cw_url = 'http://my.charmworld.example.com/'
621+ url = u'{}api/3/bundle/{}/metric/deployments/increment'.format(
622+ cw_url, bundle_id)
623+ expected = 'Incrementing bundle.+'
624+ called_with = []
625+ mock_fetch = mock_fetch_factory(200, called_with)
626+ with ExpectLog('', expected, required=True):
627+ mock_path = 'tornado.httpclient.AsyncHTTPClient.fetch'
628+ with mock.patch(mock_path, mock_fetch):
629+ ok = yield utils.increment_deployment_counter(
630+ bundle_id, cw_url)
631+ self.assertTrue(ok)
632+ called_args, called_kwargs = called_with[0]
633+ self.assertEqual(url, urllib.unquote(called_args[0]))
634+ self.assertEqual(dict(callback=None), called_kwargs)
635+
636+ @gen_test
637+ def test_increment_errors(self):
638+ bundle_id = '~bac/muletrain/wiki'
639+ cw_url = 'http://my.charmworld.example.com/'
640+ mock_path = 'tornado.httpclient.AsyncHTTPClient.fetch'
641+ mock_fetch = mock_fetch_factory(404)
642+ with mock.patch(mock_path, mock_fetch):
643+ ok = yield utils.increment_deployment_counter(bundle_id, cw_url)
644+ self.assertFalse(ok)
645
646=== modified file 'server/guiserver/tests/bundles/test_views.py'
647--- server/guiserver/tests/bundles/test_views.py 2013-11-07 11:17:54 +0000
648+++ server/guiserver/tests/bundles/test_views.py 2013-11-15 18:31:21 +0000
649@@ -206,13 +206,38 @@
650 # Ensure the Deployer methods have been correctly called.
651 args = (request.user, 'mybundle', {'services': {}})
652 self.deployer.validate.assert_called_once_with(*args)
653+ args = (request.user, 'mybundle', {'services': {}}, None)
654 self.deployer.import_bundle.assert_called_once_with(*args)
655
656- @gen_test
657+ # The following tests exercise views._validate_import_params directly.
658 def test_no_name_success(self):
659 # The process succeeds if the bundle name is not provided but the
660 # YAML contents include just one bundle.
661 params = {'YAML': 'mybundle: {services: {}}'}
662+ results = views._validate_import_params(params)
663+ expected = ('mybundle', {'services': {}}, None)
664+ self.assertEqual(expected, results)
665+
666+ def test_id_provided(self):
667+ params = {'YAML': 'mybundle: {services: {}}',
668+ 'BundleID': '~jorge/wiki/3/smallwiki'}
669+ results = views._validate_import_params(params)
670+ expected = ('mybundle', {'services': {}}, '~jorge/wiki/3/smallwiki')
671+ self.assertEqual(expected, results)
672+
673+ def test_id_and_name_provided(self):
674+ params = {'YAML': 'mybundle: {services: {}}',
675+ 'Name': 'mybundle',
676+ 'BundleID': '~jorge/wiki/3/smallwiki'}
677+ results = views._validate_import_params(params)
678+ expected = ('mybundle', {'services': {}}, '~jorge/wiki/3/smallwiki')
679+ self.assertEqual(expected, results)
680+
681+ @gen_test
682+ def test_id_passed_to_deployer(self):
683+ params = {'YAML': 'mybundle: {services: {}}',
684+ 'Name': 'mybundle',
685+ 'BundleID': '~jorge/wiki/3/smallwiki'}
686 request = self.make_view_request(params=params)
687 # Set up the Deployer mock.
688 self.deployer.validate.return_value = self.make_future(None)
689@@ -222,6 +247,8 @@
690 # Ensure the Deployer methods have been correctly called.
691 args = (request.user, 'mybundle', {'services': {}})
692 self.deployer.validate.assert_called_once_with(*args)
693+ args = (request.user, 'mybundle', {'services': {}},
694+ '~jorge/wiki/3/smallwiki')
695 self.deployer.import_bundle.assert_called_once_with(*args)
696
697
698
699=== modified file 'server/runserver.py'
700--- server/runserver.py 2013-08-26 11:27:32 +0000
701+++ server/runserver.py 2013-11-15 18:31:21 +0000
702@@ -25,6 +25,7 @@
703 --insecure
704 --sandbox
705 --logging=debug|info|warning|error
706+ --charmworldurl="https://manage.jujucharms.com/"
707
708 The --sslpath option is ignored if --insecure is set.
709 The --apiurl and --apiversion options are ignored if --sandbox is set.
710
711=== modified file 'tests/test_backends.py'
712--- tests/test_backends.py 2013-11-13 18:31:14 +0000
713+++ tests/test_backends.py 2013-11-15 18:31:21 +0000
714@@ -202,7 +202,7 @@
715 config = {
716 'builtin-server': True,
717 'builtin-server-logging': 'info',
718- 'charmworld-url': 'http://charmworld.example.com',
719+ 'charmworld-url': 'http://charmworld.example.com/',
720 'command-log-file': self.command_log_file,
721 'default-viewmode': 'sidebar',
722 'ga-key': 'my-key',
723@@ -437,7 +437,8 @@
724 mocks.start_builtin_server.assert_called_once_with(
725 mocks.compute_build_dir(), self.ssl_cert_path,
726 config['serve-tests'], config['sandbox'],
727- config['builtin-server-logging'], not config['secure'])
728+ config['builtin-server-logging'], not config['secure'],
729+ config['charmworld-url'])
730 self.assertFalse(mocks.start_haproxy_apache.called)
731
732 def test_start_go_builtin(self):
733@@ -455,7 +456,8 @@
734 mocks.start_builtin_server.assert_called_once_with(
735 mocks.compute_build_dir(), self.ssl_cert_path,
736 config['serve-tests'], config['sandbox'],
737- config['builtin-server-logging'], not config['secure'])
738+ config['builtin-server-logging'], not config['secure'],
739+ config['charmworld-url'])
740 self.assertFalse(mocks.start_haproxy_apache.called)
741
742 def test_stop_python_legacy(self):
743
744=== modified file 'tests/test_utils.py'
745--- tests/test_utils.py 2013-11-14 19:10:28 +0000
746+++ tests/test_utils.py 2013-11-15 18:31:21 +0000
747@@ -833,7 +833,7 @@
748 self.run_call_count = 0
749 self.fake_zk_address = '192.168.5.26'
750 self.build_dir = 'juju-gui/build-'
751- self.charmworld_url = 'http://charmworld.example'
752+ self.charmworld_url = 'http://charmworld.example.com/'
753 self.ssl_cert_path = 'ssl/cert/path'
754
755 # Monkey patches.
756@@ -983,7 +983,8 @@
757
758 def test_write_builtin_server_startup(self):
759 write_builtin_server_startup(
760- JUJU_GUI_DIR, self.ssl_cert_path, serve_tests=True, insecure=True)
761+ JUJU_GUI_DIR, self.ssl_cert_path, serve_tests=True, insecure=True,
762+ charmworld_url=self.charmworld_url)
763 guiserver_conf = self.files['guiserver.conf']
764 self.assertIn('description "GUIServer"', guiserver_conf)
765 self.assertIn('--logging="info"', guiserver_conf)
766@@ -993,6 +994,8 @@
767 '--testsroot="{}/test/"'.format(JUJU_GUI_DIR), guiserver_conf)
768 self.assertIn('--insecure', guiserver_conf)
769 self.assertNotIn('--sandbox', guiserver_conf)
770+ self.assertIn('--charmworldurl="http://charmworld.example.com/"',
771+ guiserver_conf)
772
773 def test_write_builtin_server_startup_sandbox_and_logging(self):
774 # The upstart configuration file for the GUI server is correctly
775@@ -1011,7 +1014,8 @@
776 def test_start_builtin_server(self):
777 start_builtin_server(
778 JUJU_GUI_DIR, self.ssl_cert_path, serve_tests=False, sandbox=False,
779- builtin_server_logging='info', insecure=False)
780+ builtin_server_logging='info', insecure=False,
781+ charmworld_url='http://charmworld.example.com/')
782 self.assertEqual(self.svc_ctl_call_count, 1)
783 self.assertEqual(self.service_names, ['guiserver'])
784 self.assertEqual(self.actions, [charmhelpers.RESTART])
785@@ -1036,7 +1040,8 @@
786 self.assertIn('readOnly: true', js_conf)
787 self.assertIn("socket_url: 'wss://", js_conf)
788 self.assertIn('socket_protocol: "wss"', js_conf)
789- self.assertIn('charmworldURL: "http://charmworld.example"', js_conf)
790+ self.assertIn('charmworldURL: "http://charmworld.example.com/"',
791+ js_conf)
792 self.assertIn('GA_key: "UA-123456"', js_conf)
793
794 def test_write_gui_config_insecure(self):

Subscribers

People subscribed via source and target branches