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