charmworld

Merge lp:~bac/charmworld/bundle-redirect into lp:charmworld

  1. bundle-redirect
  2. Merge into trunk
Proposed by Brad Crittenden
Status: Merged
Approved by: Brad Crittenden
Approved revision: 524
Merged at revision: 523
Proposed branch: lp:~bac/charmworld/bundle-redirect
Merge into: lp:charmworld
Diff against target: 374 lines (+114/-68)
7 files modified
charmworld/__init__.py (+2/-2)
charmworld/jobs/config.py (+3/-1)
charmworld/testing/factory.py (+1/-4)
charmworld/views/bundles.py (+19/-5)
charmworld/views/helpers.py (+5/-0)
charmworld/views/tests/test_bundles.py (+77/-56)
default.ini (+7/-0)
To merge this branch: bzr merge lp:~bac/charmworld/bundle-redirect
Reviewer Review Type Date Requested Status
Juju Gui Bot continuous-integration Approve
Brad Crittenden (community) code Approve
Fabrice Matrat (community) Approve
Francesco Banconi Approve
Review via email: mp+249420@code.launchpad.net

Commit message

Redirect bundles.

Description of the change

Redirect bundles to jujucharms.com

QA: a bit hard as getting bundles to load that are visible in jujucharms.com is difficult since 1) many bundles name a specific version of the charm and that version may not be ingestable, 2) jujucharms.com does not seem to have a complete set of available bundles. (?)

But if you can find a local bundle that has a corresponding one in jujucharms.com, test that the redirect works.

In all honesty, I'm not going to feel 100% about this branch until we get it on staging and can do very thorough QA.

To post a comment you must log in.
Revision history for this message
Francesco Banconi (frankban) wrote :

Looks good to me with minor suggestions.
Thank you!

review: Approve
Revision history for this message
Fabrice Matrat (fabricematrat) wrote :

I just wonder if redirecting to a search in jujucharms.com won't be safer ??

Revision history for this message
Brad Crittenden (bac) wrote :

QA:
# Personal bundle
http://127.0.0.1:2464/bundle/~a.rosales/storm/instantstorm
# Personal bundle with revision
http://127.0.0.1:2464/bundle/~a.rosales/storm/5/instantstorm
# Official bundle
http://127.0.0.1:2464/bundle/hadoop/7/cluster

Revision history for this message
Fabrice Matrat (fabricematrat) wrote :

Working great, did browse a few bundles and get redirected.
Code LGTM.

review: Approve
Revision history for this message
Juju Gui Bot (juju-gui-bot) wrote :

FAILED: Autolanding.
No commit message was specified in the merge proposal. Hit 'Add commit message' on the merge proposal web page or follow the link below. You can approve the merge proposal yourself to rerun.
https://code.launchpad.net/~bac/charmworld/bundle-redirect/+merge/249420/+edit-commit-message

review: Needs Fixing (continuous-integration)
Revision history for this message
Brad Crittenden (bac) wrote :

Kick CI

review: Approve (code)
Revision history for this message
Juju Gui Bot (juju-gui-bot) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charmworld/__init__.py'
2--- charmworld/__init__.py 2013-11-14 13:38:24 +0000
3+++ charmworld/__init__.py 2015-02-18 14:26:31 +0000
4@@ -59,12 +59,12 @@
5
6 if 'http_cache' not in settings:
7 http_cache = app_settings.get('http_cache')
8- if http_cache is not None:
9+ if http_cache:
10 http_cache = int(http_cache)
11 settings['http_cache'] = http_cache
12 if 'http_cache_modifier' in settings:
13 mod = settings['http_cache_modifier']
14- if settings['http_cache'] is not None:
15+ if settings['http_cache']:
16 settings['http_cache'] = int(settings['http_cache'] * mod)
17 # We can't let the new setting to get add_view or it blows up
18 # with an unexpected kwarg.
19
20=== modified file 'charmworld/jobs/config.py'
21--- charmworld/jobs/config.py 2013-10-10 11:27:55 +0000
22+++ charmworld/jobs/config.py 2015-02-18 14:26:31 +0000
23@@ -7,12 +7,14 @@
24 import shutil
25 import tempfile
26
27+from pyramid.settings import asbool
28+
29 from charmworld.utils import get_ini
30
31
32 settings = get_ini()
33
34-if settings.get('is_testing', 'false').lower() == 'true':
35+if asbool(settings.get('is_testing')):
36 CHARM_DIR = tempfile.mkdtemp()
37 atexit.register(shutil.rmtree, CHARM_DIR, ignore_errors=True)
38 else:
39
40=== modified file 'charmworld/testing/factory.py'
41--- charmworld/testing/factory.py 2014-05-15 15:57:23 +0000
42+++ charmworld/testing/factory.py 2015-02-18 14:26:31 +0000
43@@ -375,10 +375,7 @@
44
45
46 def make_bundle(db, *args, **kwargs):
47- with_basket = False
48- if 'with_basket' in kwargs:
49- with_basket = True
50- del kwargs['with_basket']
51+ with_basket = kwargs.pop('with_basket', False)
52
53 bundle_data = get_bundle_data(*args, **kwargs)
54 bundle = Bundle(bundle_data)
55
56=== modified file 'charmworld/views/bundles.py'
57--- charmworld/views/bundles.py 2014-03-20 14:00:11 +0000
58+++ charmworld/views/bundles.py 2015-02-18 14:26:31 +0000
59@@ -5,6 +5,8 @@
60
61 import os.path
62
63+from pyramid.httpexceptions import HTTPMovedPermanently
64+
65 from charmworld import cached_view_config
66 from charmworld.models import (
67 Bundle,
68@@ -15,8 +17,8 @@
69 from charmworld.views.api import json_response
70 from charmworld.views.helpers import (
71 find_bundle,
72- found,
73 format_change,
74+ redirect_url,
75 )
76
77
78@@ -123,6 +125,14 @@
79 )
80
81
82+def _bundle_search_redirect(request):
83+ """Return the redirect search location."""
84+ return '{url}/q/{basket}?type=bundle'.format(
85+ url=redirect_url(request),
86+ basket=request.matchdict['basket'],
87+ )
88+
89+
90 @cached_view_config(
91 route_name="personal-bundle-revision",
92 renderer="charmworld:templates/bundle.pt")
93@@ -130,8 +140,10 @@
94 route_name="personal-bundle",
95 renderer="charmworld:templates/bundle.pt")
96 def personal_bundle(request):
97- bundle = find_bundle(request)
98- return _bundle_view(request, found(bundle))
99+ # For simplicity and safety, all bundles will redirect to a search on the
100+ # basket name.
101+ location = _bundle_search_redirect(request)
102+ raise HTTPMovedPermanently(location=location)
103
104
105 @cached_view_config(
106@@ -159,8 +171,10 @@
107 route_name="official-bundle-revision",
108 renderer="charmworld:templates/bundle.pt")
109 def official_bundle(request):
110- bundle = find_bundle(request, promulgated=True)
111- return _bundle_view(request, found(bundle))
112+ # For simplicity and safety, all bundles will redirect to a search on the
113+ # basket name.
114+ location = _bundle_search_redirect(request)
115+ raise HTTPMovedPermanently(location=location)
116
117
118 @cached_view_config(
119
120=== modified file 'charmworld/views/helpers.py'
121--- charmworld/views/helpers.py 2014-02-10 21:30:20 +0000
122+++ charmworld/views/helpers.py 2015-02-18 14:26:31 +0000
123@@ -161,3 +161,8 @@
124 else:
125 spec["owner"] = request.matchdict['owner']
126 return Bundle.from_query(spec, request.db)
127+
128+
129+def redirect_url(request):
130+ """Get the redirect_jujucharms configuration setting."""
131+ return request.registry.settings.get('redirect_jujucharms')
132
133=== modified file 'charmworld/views/tests/test_bundles.py'
134--- charmworld/views/tests/test_bundles.py 2014-03-06 16:01:13 +0000
135+++ charmworld/views/tests/test_bundles.py 2015-02-18 14:26:31 +0000
136@@ -1,10 +1,12 @@
137-# Copyright 2013-2014 Canonical Ltd. This software is licensed under the
138+# Copyright 2013-2015 Canonical Ltd. This software is licensed under the
139 # GNU Affero General Public License version 3 (see the file LICENSE).
140
141 from StringIO import StringIO
142 import datetime
143 import unittest
144
145+from pyramid.httpexceptions import HTTPMovedPermanently
146+
147 from charmworld.models import (
148 Bundle,
149 DatedMetric,
150@@ -152,7 +154,7 @@
151 found = find_bundle(request)
152 self.assertEqual(bundle, found)
153
154- def test_multiple_bundles_picks_newest(self):
155+ def _multiple_bundle_setup(self):
156 basket_rev1 = 'riak/1'
157 name = 'xyz'
158 owner = 'matt'
159@@ -161,12 +163,16 @@
160 basket_rev2 = 'riak/10'
161 bundle2, bundle2_data = factory.makeBundle(
162 self.db, name=name, owner=owner, basket_with_rev=basket_rev2)
163+ return name, owner, bundle2
164+
165+ def test_multiple_bundles_picks_newest(self):
166+ name, owner, expected_bundle = self._multiple_bundle_setup()
167 request = self.getRequest()
168 request.matchdict = dict(bundle=name,
169 basket='riak',
170 owner=owner)
171 found = find_bundle(request)
172- self.assertEqual(bundle2, found)
173+ self.assertEqual(expected_bundle, found)
174
175 def test_found_bundle_promulgated(self):
176 bundle, bundle_data = factory.makeBundle(self.db, promulgated=True)
177@@ -198,13 +204,10 @@
178 basket=basket,
179 bundle=bundle.name,
180 )
181- expected = dict(
182- series='raring',
183- relations={},
184- services={},
185- )
186- response = personal_bundle(request)
187- self.assertEqual(expected, response['bundle'].data)
188+ with self.assertRaises(HTTPMovedPermanently) as ctx:
189+ personal_bundle(request)
190+ expected = '/q/{}?type=bundle'.format(basket)
191+ self.assertTrue(ctx.exception.location.endswith(expected))
192
193 def test_personal_bundle_json(self):
194 basket = 'ball'
195@@ -298,37 +301,63 @@
196 # Test that the route works.
197 path = '/bundle/~{owner}/{basket}/{bundle}'.format(
198 owner=owner, basket=basket, bundle=bundle_name)
199- resp = self.app.get(path, status=200)
200- self.assertIn(bundle.owner, resp.body)
201- self.assertIn(bundle.data['series'], resp.body)
202- self.assertIn(bundle.basket_name, resp.body)
203- self.assertIn(bundle.owner, resp.body)
204- # Ensure the short url to the branch on Launchpad is in the body.
205- lp_path = '~{owner}/charms/bundles/{basket}/bundle'.format(
206- owner=owner, basket=basket)
207- lp_short_url = 'lp:{path}'.format(path=lp_path)
208- self.assertIn(lp_short_url, resp.body)
209- # Ensure the long url to the branch on Launchpad is in the body.
210- lp_long_url = 'https://code.launchpad.net/{path}'.format(path=lp_path)
211- self.assertIn(lp_long_url, resp.body)
212+ resp = self.app.get(path, status=301)
213+ expected = '/q/{}?type=bundle'.format(basket)
214+ self.assertTrue(resp.location.endswith(expected))
215+
216+ def test_personal_bundle_route_multi_bundle_basket(self):
217+ owner = 'ladonna'
218+ basket = 'treme'
219+ bundle_name = 'gigis'
220+ rev = 1
221+ basket_with_rev = '%s/%d' % (basket, rev)
222+ bundle_1, bundle_data_1 = factory.makeBundle(
223+ self.db, name=bundle_name, owner=owner,
224+ basket_with_rev=basket_with_rev, series='raring',
225+ with_basket=True,
226+ )
227+ bundle_2, bundle_data_2 = factory.makeBundle(
228+ self.db, name='other_bundle', owner=owner,
229+ basket_with_rev=basket_with_rev, series='raring',
230+ with_basket=True,
231+ )
232+ # Test that the route works.
233+ path = '/bundle/~{owner}/{basket}/{bundle}'.format(
234+ owner=owner, basket=basket, bundle=bundle_name)
235+ resp = self.app.get(path, status=301)
236+ expected = '/q/{}?type=bundle'.format(basket)
237+ self.assertTrue(resp.location.endswith(expected))
238+
239+ def test_personal_bundle_route_not_found(self):
240+ # We just forward without checking the database, so a 404 is not
241+ # thrown.
242+ basket = 'nobasket'
243+ path = '/bundle/~noowner/{}/nobundle'.format(basket)
244+ resp = self.app.get(path, status=301)
245+ expected = '/q/{}?type=bundle'.format(basket)
246+ self.assertTrue(resp.location.endswith(expected))
247
248 def test_personal_bundle_route_with_revision(self):
249+ basket = 'wicker'
250 bundle, bundle_data = factory.makeBundle(
251 self.db, name='gigis', owner='ladonna',
252- basket_with_rev='basket/1', series='raring',
253+ basket_with_rev='{}/1'.format(basket), series='raring',
254 )
255- path = '/bundle/~ladonna/basket/1/gigis'
256- response = self.app.get(path, status=200)
257- self.assertIn(
258- 'lp:~ladonna/charms/bundles/basket/bundle', response.body)
259+ path = '/bundle/~ladonna/{}/1/gigis'.format(basket)
260+ resp = self.app.get(path, status=301)
261+ expected = '/q/{}?type=bundle'.format(basket)
262+ self.assertTrue(resp.location.endswith(expected))
263
264 def test_personal_bundle_route_404(self):
265+ basket = 'wicker'
266 bundle, bundle_data = factory.makeBundle(
267 self.db, name='gigis', owner='ladonna',
268- basket_with_rev='basket/1', series='raring',
269+ basket_with_rev='{}/1'.format(basket), series='raring',
270 )
271- path = '/bundle/~ladonna/basket/1/XXX'
272- self.app.get(path, status=404)
273+ path = '/bundle/~ladonna/{}/1/XXX'.format(basket)
274+ resp = self.app.get(path, status=301)
275+ expected = '/q/{}?type=bundle'.format(basket)
276+ self.assertTrue(resp.location.endswith(expected))
277
278 def test_personal_bundle_with_revision_json_route(self):
279 # A JSON-encoded bundle can be requested in which the ID includes both
280@@ -364,7 +393,7 @@
281 path = '/bundle/~ladonna/basket/1/XXX/json'
282 self.app.get(path, status=404)
283
284- def test_official_bundle_route(self):
285+ def test_official_bundle_json_route(self):
286 bundle, bundle_data = factory.makeBundle(
287 self.db, name='wiki', owner='charmers',
288 basket_with_rev='wiki-basket/1', series='raring', promulgated=True,
289@@ -376,14 +405,16 @@
290 self.assertIn(str(bundle.data['services']), resp.body)
291
292 def test_official_bundle_with_revision_route(self):
293+ basket = 'wiki-basket'
294 bundle, bundle_data = factory.makeBundle(
295 self.db, name='wiki', owner='charmers',
296- basket_with_rev='wiki-basket/1', series='raring', promulgated=True,
297+ basket_with_rev='{}/1'.format(basket), series='raring',
298+ promulgated=True,
299 )
300- path = '/bundle/wiki-basket/1/wiki'
301- response = self.app.get(path, status=200)
302- self.assertIn(
303- 'lp:~charmers/charms/bundles/wiki-basket/bundle', response.body)
304+ path = '/bundle/{}/1/wiki'.format(basket)
305+ resp = self.app.get(path, status=301)
306+ expected = '/q/{}?type=bundle'.format(basket)
307+ self.assertTrue(resp.location.endswith(expected))
308
309 def test_official_bundle_route_404(self):
310 bundle, bundle_data = factory.makeBundle(
311@@ -394,25 +425,15 @@
312 self.app.get(path, status=404)
313
314 def test_bundle_short_url_route(self):
315- bundle, bundle_data = factory.makeBundle(
316- self.db, name='gigis', owner='ladonna',
317- basket_with_rev='basket/1', series='raring', promulgated=True,
318- )
319- self.app.get('/' + bundle.short_url, status=200)
320-
321-
322-class BundleDisplayTests(WebTestBase):
323- """Verify that bundles are displayed correctly."""
324-
325- def test_download_statistics_are_displayed(self):
326- # The rendered page includes details about the number of times this
327- # bundle has been "downloaded" (actually, it means "deployed").
328- bundle, bundle_data = factory.makeBundle(
329- self.db, name='gigis', owner='ladonna',
330- basket_with_rev='basket/1', series='raring', promulgated=True,
331- )
332- response = self.app.get('/' + bundle.short_url, status=200)
333- self.assertIn('>Downloads<', response.body)
334+ basket = 'wicker'
335+ bundle, bundle_data = factory.makeBundle(
336+ self.db, name='gigis', owner='ladonna',
337+ basket_with_rev='{}/1'.format(basket), series='raring',
338+ promulgated=True,
339+ )
340+ resp = self.app.get('/' + bundle.short_url, status=301)
341+ expected = '/q/{}?type=bundle'.format(basket)
342+ self.assertTrue(resp.location.endswith(expected))
343
344
345 class StatisticsTests(unittest.TestCase):
346
347=== modified file 'default.ini'
348--- default.ini 2015-02-09 14:16:33 +0000
349+++ default.ini 2015-02-18 14:26:31 +0000
350@@ -29,6 +29,9 @@
351 # App Settings
352 project_name = Charm Browser
353
354+# Caching settings
355+http_cache =
356+
357 # List of groups/teams that have edit access.
358 openid_teams = charmers,juju-jitsu
359
360@@ -71,6 +74,9 @@
361 # New web site for redirection
362 redirect_jujucharms = https://jujucharms.com
363
364+# This signals that we are running under test.
365+is_testing = false
366+
367 [server:main]
368 use = egg:Paste#http
369 host = 0.0.0.0
370@@ -134,3 +140,4 @@
371 format = %(asctime)s %(message)s
372
373 # End logging configuration
374+

Subscribers

People subscribed via source and target branches

to all changes: