Merge lp:~frankban/charms/precise/juju-gui/support-default-charm-icon into lp:~juju-gui/charms/precise/juju-gui/trunk
- Precise Pangolin (12.04)
- support-default-charm-icon
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 176 |
Proposed branch: | lp:~frankban/charms/precise/juju-gui/support-default-charm-icon |
Merge into: | lp:~juju-gui/charms/precise/juju-gui/trunk |
Diff against target: |
287 lines (+146/-21) 5 files modified
revision (+1/-1) server/guiserver/__init__.py (+1/-1) server/guiserver/apps.py (+6/-2) server/guiserver/handlers.py (+78/-15) server/guiserver/tests/test_handlers.py (+60/-2) |
To merge this branch: | bzr merge lp:~frankban/charms/precise/juju-gui/support-default-charm-icon |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
charmers | Pending | ||
Review via email:
|
Commit message
Description of the change
Make the GUI server redirect to the default icon.
Create a specialized proxy handler for handling
Juju HTTP API requests. In this subclass, handle
the case a request is for a local charm icon
that cannot be found on the Juju server.
Tests: `make unittest`.
QA:
- `juju bootstrap`;
- from the branch root, run `make deploy`;
- wait for the GUI service to be ready;
- switch to the trunk branch:
`juju set juju-gui juju-gui-
- wait for the GUI to be ready;
- deploy local charms including an icon:
you should see the icons are correctly displayed in the
service blocks and inspector header;
- deploy a local charm not including an icon:
you should see the fallback icon displayed both in
the service block and the inspector;
- destroy the environment, done.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Francesco Banconi (frankban) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brad Crittenden (bac) wrote : | # |
Code LGTM
https:/
File server/
https:/
server/
GET ones are handled.
s/ones/requests/ -- just reads better.
https:/
server/
charm icon is not found.
s/a when/when/ or /for when/?
- 180. By Francesco Banconi
-
Changes as per review.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Francesco Banconi (frankban) wrote : | # |
Please take a look.
https:/
File server/
https:/
server/
GET ones are handled.
On 2014/04/09 16:32:42, bac wrote:
> s/ones/requests/ -- just reads better.
Done.
https:/
server/
charm icon is not found.
On 2014/04/09 16:32:42, bac wrote:
> s/a when/when/ or /for when/?
Done.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brad Crittenden (bac) wrote : | # |
QA OK too
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Francesco Banconi (frankban) wrote : | # |
*** Submitted:
Make the GUI server redirect to the default icon.
Create a specialized proxy handler for handling
Juju HTTP API requests. In this subclass, handle
the case a request is for a local charm icon
that cannot be found on the Juju server.
Tests: `make unittest`.
QA:
- `juju bootstrap`;
- from the branch root, run `make deploy`;
- wait for the GUI service to be ready;
- switch to the trunk branch:
`juju set juju-gui juju-gui-
- wait for the GUI to be ready;
- deploy local charms including an icon:
you should see the icons are correctly displayed in the
service blocks and inspector header;
- deploy a local charm not including an icon:
you should see the fallback icon displayed both in
the service block and the inspector;
- destroy the environment, done.
R=bac
CC=
https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Francesco Banconi (frankban) wrote : | # |
Thank you Brad.
Preview Diff
1 | === modified file 'revision' | |||
2 | --- revision 2014-03-17 12:45:59 +0000 | |||
3 | +++ revision 2014-04-09 16:48:27 +0000 | |||
4 | @@ -1,1 +1,1 @@ | |||
6 | 1 | 109 | 1 | 110 |
7 | 2 | 2 | ||
8 | === modified file 'server/guiserver/__init__.py' | |||
9 | --- server/guiserver/__init__.py 2014-01-27 15:12:32 +0000 | |||
10 | +++ server/guiserver/__init__.py 2014-04-09 16:48:27 +0000 | |||
11 | @@ -37,7 +37,7 @@ | |||
12 | 37 | which originally made the request. | 37 | which originally made the request. |
13 | 38 | """ | 38 | """ |
14 | 39 | 39 | ||
16 | 40 | VERSION = (0, 3, 0) | 40 | VERSION = (0, 4, 0) |
17 | 41 | 41 | ||
18 | 42 | 42 | ||
19 | 43 | def get_version(): | 43 | def get_version(): |
20 | 44 | 44 | ||
21 | === modified file 'server/guiserver/apps.py' | |||
22 | --- server/guiserver/apps.py 2014-02-04 17:23:57 +0000 | |||
23 | +++ server/guiserver/apps.py 2014-04-09 16:48:27 +0000 | |||
24 | @@ -56,13 +56,17 @@ | |||
25 | 56 | # The tokens collection for authentication token requests. | 56 | # The tokens collection for authentication token requests. |
26 | 57 | 'tokens': tokens, | 57 | 'tokens': tokens, |
27 | 58 | } | 58 | } |
28 | 59 | juju_proxy_handler_options = { | ||
29 | 60 | 'target_url': utils.ws_to_http(options.apiurl), | ||
30 | 61 | 'charmworld_url': options.charmworldurl, | ||
31 | 62 | } | ||
32 | 59 | server_handlers.extend([ | 63 | server_handlers.extend([ |
33 | 60 | # Handle WebSocket connections. | 64 | # Handle WebSocket connections. |
34 | 61 | (r'^/ws$', handlers.WebSocketHandler, websocket_handler_options), | 65 | (r'^/ws$', handlers.WebSocketHandler, websocket_handler_options), |
35 | 62 | # Handle connections to the juju-core HTTPS server. | 66 | # Handle connections to the juju-core HTTPS server. |
36 | 63 | # The juju-core HTTPS and WebSocket servers share the same URL. | 67 | # The juju-core HTTPS and WebSocket servers share the same URL. |
39 | 64 | (r'^/juju-core/(.*)', handlers.ProxyHandler, | 68 | (r'^/juju-core/(.*)', handlers.JujuProxyHandler, |
40 | 65 | {'target_url': utils.ws_to_http(options.apiurl)}), | 69 | juju_proxy_handler_options), |
41 | 66 | ]) | 70 | ]) |
42 | 67 | if options.testsroot: | 71 | if options.testsroot: |
43 | 68 | params = {'path': options.testsroot, 'default_filename': 'index.html'} | 72 | params = {'path': options.testsroot, 'default_filename': 'index.html'} |
44 | 69 | 73 | ||
45 | === modified file 'server/guiserver/handlers.py' | |||
46 | --- server/guiserver/handlers.py 2014-01-29 10:21:03 +0000 | |||
47 | +++ server/guiserver/handlers.py 2014-04-09 16:48:27 +0000 | |||
48 | @@ -20,6 +20,7 @@ | |||
49 | 20 | import logging | 20 | import logging |
50 | 21 | import os | 21 | import os |
51 | 22 | import time | 22 | import time |
52 | 23 | import urlparse | ||
53 | 23 | 24 | ||
54 | 24 | from tornado import ( | 25 | from tornado import ( |
55 | 25 | escape, | 26 | escape, |
56 | @@ -47,6 +48,10 @@ | |||
57 | 47 | ) | 48 | ) |
58 | 48 | 49 | ||
59 | 49 | 50 | ||
60 | 51 | # Define the path to the fallback charm icon hosted by charmworld. | ||
61 | 52 | DEFAULT_CHARM_ICON_PATH = '/static/img/charm_160.svg' | ||
62 | 53 | |||
63 | 54 | |||
64 | 50 | class WebSocketHandler(websocket.WebSocketHandler): | 55 | class WebSocketHandler(websocket.WebSocketHandler): |
65 | 51 | """WebSocket handler supporting secure WebSockets. | 56 | """WebSocket handler supporting secure WebSockets. |
66 | 52 | 57 | ||
67 | @@ -225,12 +230,14 @@ | |||
68 | 225 | class ProxyHandler(web.RequestHandler): | 230 | class ProxyHandler(web.RequestHandler): |
69 | 226 | """An HTTP(S) proxy from the server to the given target URL.""" | 231 | """An HTTP(S) proxy from the server to the given target URL.""" |
70 | 227 | 232 | ||
72 | 228 | def initialize(self, target_url): | 233 | def initialize(self, target_url, validate_cert=True): |
73 | 229 | """Initialize the proxy. | 234 | """Initialize the proxy. |
74 | 230 | 235 | ||
76 | 231 | Receive the target URL where to redirect to. | 236 | Receive the target URL where to redirect to, and a flag indicating |
77 | 237 | whether to validate remote server certificates. | ||
78 | 232 | """ | 238 | """ |
79 | 233 | self.target_url = target_url | 239 | self.target_url = target_url |
80 | 240 | self.validate_cert = validate_cert | ||
81 | 234 | 241 | ||
82 | 235 | @gen.coroutine | 242 | @gen.coroutine |
83 | 236 | def get(self, path): | 243 | def get(self, path): |
84 | @@ -241,12 +248,23 @@ | |||
85 | 241 | The response will then be sent back to the client. | 248 | The response will then be sent back to the client. |
86 | 242 | """ | 249 | """ |
87 | 243 | url = join_url(self.target_url, path, self.request.query) | 250 | url = join_url(self.target_url, path, self.request.query) |
94 | 244 | # Server certificates are not validated: we use this function to | 251 | response = yield self.send_request(url) |
95 | 245 | # connect to juju-core, and we would need to obtain ca-certificates | 252 | if response is not None: |
96 | 246 | # from it. Unfortunately we don't have that information, and for this | 253 | self.send_response(response) |
97 | 247 | # reason we skip validation for both WebSocket and HTTPS connections. | 254 | |
98 | 248 | # This is not ideal but currently is our best option. | 255 | # Handle POST requests the same way GET requests are handled. |
99 | 249 | request = clone_request(self.request, url, validate_cert=False) | 256 | post = get |
100 | 257 | |||
101 | 258 | @gen.coroutine | ||
102 | 259 | def send_request(self, url): | ||
103 | 260 | """Send an asynchronous request to the given URL. | ||
104 | 261 | |||
105 | 262 | Return the server response. | ||
106 | 263 | If an error occurs in the communication, return None and call | ||
107 | 264 | self._send_error with the given error. | ||
108 | 265 | """ | ||
109 | 266 | request = clone_request( | ||
110 | 267 | self.request, url, validate_cert=self.validate_cert) | ||
111 | 250 | client = httpclient.AsyncHTTPClient() | 268 | client = httpclient.AsyncHTTPClient() |
112 | 251 | try: | 269 | try: |
113 | 252 | response = yield client.fetch(request) | 270 | response = yield client.fetch(request) |
114 | @@ -254,13 +272,9 @@ | |||
115 | 254 | response = getattr(err, 'response', None) | 272 | response = getattr(err, 'response', None) |
116 | 255 | if not response: | 273 | if not response: |
117 | 256 | self._send_error(url, err) | 274 | self._send_error(url, err) |
125 | 257 | return | 275 | raise gen.Return(response) |
126 | 258 | self._send_response(response) | 276 | |
127 | 259 | 277 | def send_response(self, response): | |
121 | 260 | # Handle POST requests the same way GET ones are handled. | ||
122 | 261 | post = get | ||
123 | 262 | |||
124 | 263 | def _send_response(self, response): | ||
128 | 264 | """Prepare and send the response to the client.""" | 278 | """Prepare and send the response to the client.""" |
129 | 265 | self.set_status(response.code) | 279 | self.set_status(response.code) |
130 | 266 | set_header = self.set_header | 280 | set_header = self.set_header |
131 | @@ -279,6 +293,55 @@ | |||
132 | 279 | self.write('Internal server error:\n{}'.format(msg)) | 293 | self.write('Internal server error:\n{}'.format(msg)) |
133 | 280 | 294 | ||
134 | 281 | 295 | ||
135 | 296 | class JujuProxyHandler(ProxyHandler): | ||
136 | 297 | """A specialized proxy handler used for the juju-core HTTP API.""" | ||
137 | 298 | |||
138 | 299 | def initialize(self, target_url, charmworld_url): | ||
139 | 300 | """Initialize the proxy. | ||
140 | 301 | |||
141 | 302 | Receive the target URL where to redirect to, and the charmworld URL | ||
142 | 303 | used to retrieve the default charm icon. | ||
143 | 304 | """ | ||
144 | 305 | # Server certificates are not validated: we use this handler to connect | ||
145 | 306 | # to juju-core, and we would need to obtain ca-certificates from it. | ||
146 | 307 | # Unfortunately we don't have that information, and for this reason we | ||
147 | 308 | # skip validation for both WebSocket and HTTPS connections. This is not | ||
148 | 309 | # ideal but currently is our best option. | ||
149 | 310 | super(JujuProxyHandler, self).initialize( | ||
150 | 311 | target_url, validate_cert=False) | ||
151 | 312 | self.default_charm_icon_url = urlparse.urljoin( | ||
152 | 313 | charmworld_url, DEFAULT_CHARM_ICON_PATH) | ||
153 | 314 | |||
154 | 315 | @gen.coroutine | ||
155 | 316 | def get(self, path): | ||
156 | 317 | """Handle GET requests. | ||
157 | 318 | See the ProxyHandler.get method. | ||
158 | 319 | |||
159 | 320 | Override to handle the case when a charm icon is not found. | ||
160 | 321 | """ | ||
161 | 322 | url = join_url(self.target_url, path, self.request.query) | ||
162 | 323 | response = yield self.send_request(url) | ||
163 | 324 | if response is not None: | ||
164 | 325 | if response.code == 404 and self._charm_icon_requested(path): | ||
165 | 326 | # This is a request for a charm icon file, and the icon is not | ||
166 | 327 | # found: redirect to the fallback icon hosted on charmworld. | ||
167 | 328 | self.redirect(self.default_charm_icon_url) | ||
168 | 329 | else: | ||
169 | 330 | # Return the response to the client as usual. | ||
170 | 331 | self.send_response(response) | ||
171 | 332 | |||
172 | 333 | def _charm_icon_requested(self, path): | ||
173 | 334 | """Return True if the current request is for a charm icon.""" | ||
174 | 335 | return ( | ||
175 | 336 | # The request is for a local charm. | ||
176 | 337 | path == 'charms' and | ||
177 | 338 | # The charm URL is specified. | ||
178 | 339 | self.get_argument('url', None) and | ||
179 | 340 | # The icon file is requested. | ||
180 | 341 | self.get_argument('file', None) == 'icon.svg' | ||
181 | 342 | ) | ||
182 | 343 | |||
183 | 344 | |||
184 | 282 | class InfoHandler(web.RequestHandler): | 345 | class InfoHandler(web.RequestHandler): |
185 | 283 | """Return information about the GUI server.""" | 346 | """Return information about the GUI server.""" |
186 | 284 | 347 | ||
187 | 285 | 348 | ||
188 | === modified file 'server/guiserver/tests/test_handlers.py' | |||
189 | --- server/guiserver/tests/test_handlers.py 2014-01-28 15:48:59 +0000 | |||
190 | +++ server/guiserver/tests/test_handlers.py 2014-04-09 16:48:27 +0000 | |||
191 | @@ -517,6 +517,7 @@ | |||
192 | 517 | 'Server': 'Apache/2.4.1 (Unix)', | 517 | 'Server': 'Apache/2.4.1 (Unix)', |
193 | 518 | 'WWW-Authenticate': 'Basic', | 518 | 'WWW-Authenticate': 'Basic', |
194 | 519 | } | 519 | } |
195 | 520 | expected_validate_cert = True | ||
196 | 520 | 521 | ||
197 | 521 | def get_app(self): | 522 | def get_app(self): |
198 | 522 | # Set up an application exposing the proxy handler. | 523 | # Set up an application exposing the proxy handler. |
199 | @@ -570,8 +571,6 @@ | |||
200 | 570 | self.assertEqual(self.target_url + '/remote-path/', remote_request.url) | 571 | self.assertEqual(self.target_url + '/remote-path/', remote_request.url) |
201 | 571 | self.assert_include_headers( | 572 | self.assert_include_headers( |
202 | 572 | self.request_headers, remote_request.headers) | 573 | self.request_headers, remote_request.headers) |
203 | 573 | # Certificates are automatically accepted. | ||
204 | 574 | self.assertFalse(remote_request.validate_cert) | ||
205 | 575 | 574 | ||
206 | 576 | def test_post_request(self): | 575 | def test_post_request(self): |
207 | 577 | # POST requests are properly sent to the target URL. | 576 | # POST requests are properly sent to the target URL. |
208 | @@ -595,6 +594,19 @@ | |||
209 | 595 | # Also the body is propagated. | 594 | # Also the body is propagated. |
210 | 596 | self.assertEqual('original body', remote_request.body) | 595 | self.assertEqual('original body', remote_request.body) |
211 | 597 | 596 | ||
212 | 597 | def test_validate_certificates(self): | ||
213 | 598 | # Server certificates are properly handled. | ||
214 | 599 | remote_response = helpers.make_response(200) | ||
215 | 600 | with self.patch_http_client(remote_response) as mock_client: | ||
216 | 601 | self.fetch('/base/remote-path/', headers=self.request_headers) | ||
217 | 602 | mock_fetch = mock_client().fetch | ||
218 | 603 | # The client's fetch method has been used to fetch the remote resource. | ||
219 | 604 | mock_fetch = mock_client().fetch | ||
220 | 605 | self.assertEqual(1, mock_fetch.call_count) | ||
221 | 606 | remote_request = mock_fetch.call_args[0][0] | ||
222 | 607 | self.assertEqual( | ||
223 | 608 | self.expected_validate_cert, remote_request.validate_cert) | ||
224 | 609 | |||
225 | 598 | def test_remote_path(self): | 610 | def test_remote_path(self): |
226 | 599 | # The corresponding path on the remote server is properly generated. | 611 | # The corresponding path on the remote server is properly generated. |
227 | 600 | remote_response = helpers.make_response(200) | 612 | remote_response = helpers.make_response(200) |
228 | @@ -617,6 +629,15 @@ | |||
229 | 617 | self.assertEqual('try harder', response.body) | 629 | self.assertEqual('try harder', response.body) |
230 | 618 | self.assertEqual('Bad Request', response.reason) | 630 | self.assertEqual('Bad Request', response.reason) |
231 | 619 | 631 | ||
232 | 632 | def test_not_found_response(self): | ||
233 | 633 | # 404 responses are returned to the original client. | ||
234 | 634 | remote_response = helpers.make_response(404, body='try later') | ||
235 | 635 | with self.patch_http_client(remote_response): | ||
236 | 636 | response = self.fetch('/base/remote-path/') | ||
237 | 637 | self.assertEqual(404, response.code) | ||
238 | 638 | self.assertEqual('try later', response.body) | ||
239 | 639 | self.assertEqual('Not Found', response.reason) | ||
240 | 640 | |||
241 | 620 | def test_internal_server_error(self): | 641 | def test_internal_server_error(self): |
242 | 621 | # A 500 error is returned if an HTTP error occurs during the remote | 642 | # A 500 error is returned if an HTTP error occurs during the remote |
243 | 622 | # request/response process. | 643 | # request/response process. |
244 | @@ -632,6 +653,43 @@ | |||
245 | 632 | self.assertEqual('Internal Server Error', response.reason) | 653 | self.assertEqual('Internal Server Error', response.reason) |
246 | 633 | 654 | ||
247 | 634 | 655 | ||
248 | 656 | class TestJujuProxyHandler(TestProxyHandler): | ||
249 | 657 | |||
250 | 658 | charmworld_url = 'https://charmworld.example.com' | ||
251 | 659 | expected_validate_cert = False | ||
252 | 660 | |||
253 | 661 | def get_app(self): | ||
254 | 662 | # Set up an application exposing the proxy handler. | ||
255 | 663 | options = { | ||
256 | 664 | 'target_url': self.target_url, | ||
257 | 665 | 'charmworld_url': self.charmworld_url, | ||
258 | 666 | } | ||
259 | 667 | return web.Application([ | ||
260 | 668 | (r'^/base/(.*)', handlers.JujuProxyHandler, options)]) | ||
261 | 669 | |||
262 | 670 | def test_default_charm_icon(self): | ||
263 | 671 | # If a charm icon is not found, a GET request redirects to the fallback | ||
264 | 672 | # icon available on charmworld. | ||
265 | 673 | remote_response = helpers.make_response(404) | ||
266 | 674 | path = '/base/charms?url=local:trusty/django=42&file=icon.svg' | ||
267 | 675 | with self.patch_http_client(remote_response): | ||
268 | 676 | response = self.fetch(path, follow_redirects=False) | ||
269 | 677 | self.assertEqual(302, response.code) | ||
270 | 678 | self.assertEqual( | ||
271 | 679 | self.charmworld_url + handlers.DEFAULT_CHARM_ICON_PATH, | ||
272 | 680 | response.headers['location']) | ||
273 | 681 | |||
274 | 682 | def test_charm_file_not_found(self): | ||
275 | 683 | # If a charm file is not found and it is not the icon, a 404 is | ||
276 | 684 | # correctly returned to the original client. | ||
277 | 685 | remote_response = helpers.make_response(404) | ||
278 | 686 | path = '/base/charms?url=local:trusty/django=42&file=readme.rst' | ||
279 | 687 | with self.patch_http_client(remote_response): | ||
280 | 688 | response = self.fetch(path) | ||
281 | 689 | self.assertEqual(404, response.code) | ||
282 | 690 | self.assertEqual('Not Found', response.reason) | ||
283 | 691 | |||
284 | 692 | |||
285 | 635 | class TestInfoHandler(LogTrapTestCase, AsyncHTTPTestCase): | 693 | class TestInfoHandler(LogTrapTestCase, AsyncHTTPTestCase): |
286 | 636 | 694 | ||
287 | 637 | def get_app(self): | 695 | def get_app(self): |
Reviewers: mp+214985_ code.launchpad. net,
Message:
Please take a look.
Description:
Make the GUI server redirect to the default icon.
Create a specialized proxy handler for handling
Juju HTTP API requests. In this subclass, handle
the case a request is for a local charm icon
that cannot be found on the Juju server.
Tests: `make unittest`.
QA: source= develop`
- `juju bootstrap`;
- from the branch root, run `make deploy`;
- wait for the GUI service to be ready;
- switch to the trunk branch:
`juju set juju-gui juju-gui-
- wait for the GUI to be ready;
- deploy local charms including an icon:
you should see the icons are correctly displayed in the
service blocks and inspector header;
- deploy a local charm not including an icon:
you should see the fallback icon displayed both in
the service block and the inspector;
- destroy the environment, done.
https:/ /code.launchpad .net/~frankban/ charms/ precise/ juju-gui/ support- default- charm-icon/ +merge/ 214985
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/86100043/
Affected files (+148, -21 lines): guiserver/ __init_ _.py guiserver/ apps.py guiserver/ handlers. py guiserver/ tests/test_ handlers. py
A [revision details]
M revision
M server/
M server/
M server/
M server/