Merge lp:~bac/charmworld/bundles-display into lp:~juju-jitsu/charmworld/trunk
- bundles-display
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Brad Crittenden |
Approved revision: | 348 |
Merged at revision: | 344 |
Proposed branch: | lp:~bac/charmworld/bundles-display |
Merge into: | lp:~juju-jitsu/charmworld/trunk |
Diff against target: |
553 lines (+173/-74) 9 files modified
charmworld/jobs/ingest.py (+10/-8) charmworld/jobs/tests/test_ingest.py (+15/-11) charmworld/models.py (+50/-14) charmworld/search.py (+4/-3) charmworld/testing/factory.py (+19/-7) charmworld/tests/test_models.py (+19/-8) charmworld/tests/test_search.py (+35/-0) charmworld/views/api.py (+6/-5) charmworld/views/tests/test_api.py (+15/-18) |
To merge this branch: | bzr merge lp:~bac/charmworld/bundles-display |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Benji York (community) | Approve | ||
Review via email:
|
Commit message
Change the basket field in the Bundle to be a basket name and revision number in the form 'basket/revno'.
Description of the change
Change the basket field in the Bundle to be a basket name and revision number in the form 'basket/revno'. Previously there was inconsistency in the way the basket id was created and used.
Remove from the top-level any bundle items that are in the 'data' dictionary.
Change models.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brad Crittenden (bac) wrote : | # |
> The branch looks good. Here are some semi-bikesheddy comments.
>
> It's kind of strange that construct_basket_id returns two values while
> all the other "construct_*_id" functions just return the ID. Perhaps
> two functions would be better. (?)
I renamed to set_basket_info and modify the basket_data dictionary in place.
>
> Does "make lint" complain about the new unused variables?
Nope. Which ones?
>
>
> Re. get_bundle_data: so now if you don't specify an owner argument you
> get an empty string, but if you pass owner=None you get a random string?
No, both cases give you a random string.
> That seems a little odd. Oh, maybe you did that because the change in
> behavior broke a bunch of tests.
No, any broken tests were fixed.
>
> Another thing, about get_bundle_data: I always thought it was weird to
> use the local namespace as a sandbox in which to construct the return
> value. Now that you're doing more in the function and having to do the
> "del rep[k]" at the end it's getting more strained. You might consider
> revamping the function to use **charm_data as the parameter list instead
> and then manipulating charm_data instead of the locals.
I've fixed it to be more explicit.
Thanks for the review and good suggestions.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Charmworld Lander (charmworld-lander) wrote : | # |
There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.
Preview Diff
1 | === modified file 'charmworld/jobs/ingest.py' | |||
2 | --- charmworld/jobs/ingest.py 2013-08-09 18:53:40 +0000 | |||
3 | +++ charmworld/jobs/ingest.py 2013-08-11 23:10:56 +0000 | |||
4 | @@ -288,13 +288,14 @@ | |||
5 | 288 | self.working_dir = working_dir | 288 | self.working_dir = working_dir |
6 | 289 | super(UpdateBundleJob, self).__init__() | 289 | super(UpdateBundleJob, self).__init__() |
7 | 290 | 290 | ||
10 | 291 | def store_bundles(self, deployer_config, basket_id): | 291 | def store_bundles(self, deployer_config, owner, basket_id): |
11 | 292 | store_bundles(self.db.bundles, deployer_config, basket_id) | 292 | store_bundles(self.db.bundles, deployer_config, owner, basket_id) |
12 | 293 | 293 | ||
13 | 294 | @staticmethod | 294 | @staticmethod |
17 | 295 | def construct_basket_id(basket_data, revno): | 295 | def set_basket_info(data, revno): |
18 | 296 | owner_segment = '~%s/' % basket_data['owner'] | 296 | owner_segment = '~%s' % data['owner'] |
19 | 297 | return '%s%s-%d' % (owner_segment, basket_data['name'], revno) | 297 | data['name_revno'] = "%s/%d" % (data['name'], revno) |
20 | 298 | data['_id'] = '%s/%s' % (owner_segment, data['name_revno']) | ||
21 | 298 | 299 | ||
22 | 299 | @staticmethod | 300 | @staticmethod |
23 | 300 | def decorate_bundles(basket_contents, basket_id): | 301 | def decorate_bundles(basket_contents, basket_id): |
24 | @@ -306,7 +307,7 @@ | |||
25 | 306 | branch_dir = fetch_branch(self.working_dir, basket_data, self.log) | 307 | branch_dir = fetch_branch(self.working_dir, basket_data, self.log) |
26 | 307 | branch = Branch.open(branch_dir) | 308 | branch = Branch.open(branch_dir) |
27 | 308 | revno = branch.revision_id_to_revno(basket_data['commit']) | 309 | revno = branch.revision_id_to_revno(basket_data['commit']) |
29 | 309 | basket_data['_id'] = self.construct_basket_id(basket_data, revno) | 310 | self.set_basket_info(basket_data, revno) |
30 | 310 | with read_locked(branch): | 311 | with read_locked(branch): |
31 | 311 | tree = branch.repository.revision_tree(basket_data['commit']) | 312 | tree = branch.repository.revision_tree(basket_data['commit']) |
32 | 312 | basket_data['file_hashes'] = quote_yaml(slurp_files(fs, tree)) | 313 | basket_data['file_hashes'] = quote_yaml(slurp_files(fs, tree)) |
33 | @@ -324,7 +325,8 @@ | |||
34 | 324 | self.db.baskets.save(basket_data) | 325 | self.db.baskets.save(basket_data) |
35 | 325 | deployer_config = self.get_deployer_config(fs, basket_data) | 326 | deployer_config = self.get_deployer_config(fs, basket_data) |
36 | 326 | self.decorate_bundles(deployer_config, basket_data['_id']) | 327 | self.decorate_bundles(deployer_config, basket_data['_id']) |
38 | 327 | self.store_bundles(deployer_config, basket_data['_id']) | 328 | self.store_bundles( |
39 | 329 | deployer_config, basket_data['owner'], basket_data['name_revno']) | ||
40 | 328 | 330 | ||
41 | 329 | 331 | ||
42 | 330 | def _rev_info(r, branch): | 332 | def _rev_info(r, branch): |
43 | @@ -333,7 +335,7 @@ | |||
44 | 333 | "revno": branch.revision_id_to_revno(r.revision_id), | 335 | "revno": branch.revision_id_to_revno(r.revision_id), |
45 | 334 | "committer": r.committer, | 336 | "committer": r.committer, |
46 | 335 | "created": r.timestamp, | 337 | "created": r.timestamp, |
48 | 336 | "message": r.message | 338 | "message": r.message, |
49 | 337 | } | 339 | } |
50 | 338 | return d | 340 | return d |
51 | 339 | 341 | ||
52 | 340 | 342 | ||
53 | === modified file 'charmworld/jobs/tests/test_ingest.py' | |||
54 | --- charmworld/jobs/tests/test_ingest.py 2013-08-09 18:53:40 +0000 | |||
55 | +++ charmworld/jobs/tests/test_ingest.py 2013-08-11 23:10:56 +0000 | |||
56 | @@ -383,7 +383,7 @@ | |||
57 | 383 | with bundle_update_environment(source_branch_dir): | 383 | with bundle_update_environment(source_branch_dir): |
58 | 384 | job.decorate_basket(bundle_data, None) | 384 | job.decorate_basket(bundle_data, None) |
59 | 385 | 385 | ||
61 | 386 | self.assertTrue(bundle_data['_id'].endswith('-8')) | 386 | self.assertTrue(bundle_data['_id'].endswith('/8')) |
62 | 387 | 387 | ||
63 | 388 | def test_decorate_basket_stores_files(self): | 388 | def test_decorate_basket_stores_files(self): |
64 | 389 | job = UpdateBundleJob() | 389 | job = UpdateBundleJob() |
65 | @@ -423,21 +423,22 @@ | |||
66 | 423 | with self.assertRaises(bzrlib.errors.NoSuchRevision): | 423 | with self.assertRaises(bzrlib.errors.NoSuchRevision): |
67 | 424 | job.decorate_basket(bundle_data, None) | 424 | job.decorate_basket(bundle_data, None) |
68 | 425 | 425 | ||
70 | 426 | def test_construct_basket_id_includes_owner_if_promulgated(self): | 426 | def test_set_basket_info_includes_owner_if_promulgated(self): |
71 | 427 | payload = factory.get_payload_json(name='mysql', promulgated=True) | 427 | payload = factory.get_payload_json(name='mysql', promulgated=True) |
74 | 428 | basket_id = UpdateBundleJob.construct_basket_id(payload, 5) | 428 | UpdateBundleJob.set_basket_info(payload, 5) |
75 | 429 | self.assertEqual('~charmers/mysql-5', basket_id) | 429 | self.assertEqual('mysql/5', payload['name_revno']) |
76 | 430 | self.assertEqual('~charmers/mysql/5', payload['_id']) | ||
77 | 430 | 431 | ||
79 | 431 | def test_construct_basket_id_includes_owner_if_not_promulgated(self): | 432 | def test_set_basket_info_includes_owner_if_not_promulgated(self): |
80 | 432 | payload = factory.get_payload_json(name='mysql', promulgated=False) | 433 | payload = factory.get_payload_json(name='mysql', promulgated=False) |
83 | 433 | basket_id = UpdateBundleJob.construct_basket_id(payload, 5) | 434 | UpdateBundleJob.set_basket_info(payload, 5) |
84 | 434 | self.assertEqual('~charmers/mysql-5', basket_id) | 435 | self.assertEqual('mysql/5', payload['name_revno']) |
85 | 436 | self.assertEqual('~charmers/mysql/5', payload['_id']) | ||
86 | 435 | 437 | ||
87 | 436 | def test_job_can_store_bundles_in_the_db(self): | 438 | def test_job_can_store_bundles_in_the_db(self): |
88 | 437 | basket_data = factory.get_payload_json(name='wordpress') | 439 | basket_data = factory.get_payload_json(name='wordpress') |
92 | 438 | basket_id = UpdateBundleJob.construct_basket_id(basket_data, 5) | 440 | UpdateBundleJob.set_basket_info(basket_data, 5) |
93 | 439 | bundle_id = '%s/%s' % (basket_id, 'stage') | 441 | bundle_id = '%s/%s' % (basket_data['_id'], 'stage') |
91 | 440 | basket_data['_id'] = basket_id | ||
94 | 441 | job = UpdateBundleJob() | 442 | job = UpdateBundleJob() |
95 | 442 | job.setup(self.db) | 443 | job.setup(self.db) |
96 | 443 | DEPLOYER_CONFIG_HASH = '31337' | 444 | DEPLOYER_CONFIG_HASH = '31337' |
97 | @@ -457,11 +458,14 @@ | |||
98 | 457 | engine: apache | 458 | engine: apache |
99 | 458 | """) | 459 | """) |
100 | 459 | fs.put(deployer_config, _id=DEPLOYER_CONFIG_HASH) | 460 | fs.put(deployer_config, _id=DEPLOYER_CONFIG_HASH) |
102 | 460 | job.store_bundles(yaml.safe_load(deployer_config), basket_id) | 461 | job.store_bundles( |
103 | 462 | yaml.safe_load(deployer_config), | ||
104 | 463 | basket_data['owner'], basket_data['name_revno']) | ||
105 | 461 | self.assertIsNotNone(self.db.bundles.find_one(bundle_id)) | 464 | self.assertIsNotNone(self.db.bundles.find_one(bundle_id)) |
106 | 462 | 465 | ||
107 | 463 | def test_job_run_stores_bundles_in_the_db(self): | 466 | def test_job_run_stores_bundles_in_the_db(self): |
108 | 464 | basket_data = factory.get_payload_json(name='wordpress') | 467 | basket_data = factory.get_payload_json(name='wordpress') |
109 | 468 | basket_data['name_revno'] = 'dummy' | ||
110 | 465 | job = UpdateBundleJob() | 469 | job = UpdateBundleJob() |
111 | 466 | job.setup(self.db) | 470 | job.setup(self.db) |
112 | 467 | with patch.object(job, 'get_deployer_config'): | 471 | with patch.object(job, 'get_deployer_config'): |
113 | 468 | 472 | ||
114 | === modified file 'charmworld/models.py' | |||
115 | --- charmworld/models.py 2013-08-09 18:53:40 +0000 | |||
116 | +++ charmworld/models.py 2013-08-11 23:10:56 +0000 | |||
117 | @@ -7,14 +7,17 @@ | |||
118 | 7 | from bzrlib.branch import Branch | 7 | from bzrlib.branch import Branch |
119 | 8 | from bzrlib.errors import NoSuchRevision | 8 | from bzrlib.errors import NoSuchRevision |
120 | 9 | from calendar import timegm | 9 | from calendar import timegm |
121 | 10 | import copy | ||
122 | 10 | import hashlib | 11 | import hashlib |
123 | 11 | import logging | 12 | import logging |
124 | 12 | from mimetypes import guess_type | 13 | from mimetypes import guess_type |
125 | 13 | import os | 14 | import os |
130 | 14 | from os.path import dirname | 15 | from os.path import ( |
131 | 15 | from os.path import join | 16 | dirname, |
132 | 16 | from os.path import normpath | 17 | join, |
133 | 17 | from os.path import split | 18 | normpath, |
134 | 19 | split, | ||
135 | 20 | ) | ||
136 | 18 | import random | 21 | import random |
137 | 19 | import re | 22 | import re |
138 | 20 | 23 | ||
139 | @@ -1176,11 +1179,9 @@ | |||
140 | 1176 | 'basket': None, | 1179 | 'basket': None, |
141 | 1177 | 'name': '', | 1180 | 'name': '', |
142 | 1178 | 'inherits': None, | 1181 | 'inherits': None, |
143 | 1179 | 'series': '', | ||
144 | 1180 | 'title': '', | 1182 | 'title': '', |
145 | 1181 | 'description': '', | 1183 | 'description': '', |
148 | 1182 | 'services': dict(), | 1184 | 'data': dict(), |
147 | 1183 | 'relations': dict(), | ||
149 | 1184 | 'promulgated': False, | 1185 | 'promulgated': False, |
150 | 1185 | 'branch_deleted': False, | 1186 | 'branch_deleted': False, |
151 | 1186 | 'doctype': 'bundle', | 1187 | 'doctype': 'bundle', |
152 | @@ -1204,9 +1205,13 @@ | |||
153 | 1204 | else: | 1205 | else: |
154 | 1205 | raise AttributeError | 1206 | raise AttributeError |
155 | 1206 | 1207 | ||
156 | 1208 | @staticmethod | ||
157 | 1209 | def construct_id(owner, basket, name): | ||
158 | 1210 | return "~%s/%s/%s" % (owner, basket, name) | ||
159 | 1211 | |||
160 | 1207 | @property | 1212 | @property |
161 | 1208 | def id(self): | 1213 | def id(self): |
163 | 1209 | return "~{}/{}/{}".format(self.owner, self.basket, self.name) | 1214 | return self.construct_id(self.owner, self.basket, self.name) |
164 | 1210 | 1215 | ||
165 | 1211 | @property | 1216 | @property |
166 | 1212 | def permanent_url(self): | 1217 | def permanent_url(self): |
167 | @@ -1303,14 +1308,45 @@ | |||
168 | 1303 | return options | 1308 | return options |
169 | 1304 | 1309 | ||
170 | 1305 | 1310 | ||
175 | 1306 | def store_bundles(collection, deployer_config, basket_id): | 1311 | def store_bundles(collection, deployer_config, owner, basket_id, |
176 | 1307 | """Store a basket of bundles into both MongoDB and ElasticSearch.""" | 1312 | index_client=None): |
177 | 1308 | index_client = ElasticSearchClient.from_settings(settings) | 1313 | """Store a basket of bundles into MongoDB and/or ElasticSearch. |
178 | 1309 | index_client.index_bundles(deployer_config.values()) | 1314 | |
179 | 1315 | :param: collection: A db bundles collection. If None the bundles are not | ||
180 | 1316 | stored to Mongo. | ||
181 | 1317 | :param: deployer_config: A dictionary representing the basket of bundles. | ||
182 | 1318 | :param: owner: A string representing the owner. No leading ~. | ||
183 | 1319 | :param: basket_id: A slash-joined string of name and revision for the | ||
184 | 1320 | basket. Does not include the owner name. | ||
185 | 1321 | :param: index_client: The elastic search client. If None it will be | ||
186 | 1322 | retrieved. Useful for Mongo-only tests where the indexing is not | ||
187 | 1323 | needed. | ||
188 | 1324 | """ | ||
189 | 1325 | |||
190 | 1326 | if index_client is None: | ||
191 | 1327 | index_client = ElasticSearchClient.from_settings(settings) | ||
192 | 1328 | |||
193 | 1329 | # Augment the deployer_config with additional information for indexing. | ||
194 | 1330 | ids = {} | ||
195 | 1331 | config_copy = copy.deepcopy(deployer_config) | ||
196 | 1332 | for key in config_copy: | ||
197 | 1333 | ids[key] = Bundle.construct_id(owner, basket_id, key) | ||
198 | 1334 | config_copy[key]['id'] = ids[key] | ||
199 | 1335 | config_copy[key]['owner'] = owner | ||
200 | 1336 | config_copy[key]['name'] = key | ||
201 | 1337 | config_copy[key]['basket'] = basket_id | ||
202 | 1338 | |||
203 | 1339 | index_client.index_bundles(config_copy.values()) | ||
204 | 1340 | if collection is None: | ||
205 | 1341 | return | ||
206 | 1310 | for key in deployer_config: | 1342 | for key in deployer_config: |
207 | 1343 | data = get_flattened_deployment(deployer_config, key) | ||
208 | 1311 | bundle_doc = { | 1344 | bundle_doc = { |
211 | 1312 | '_id': '%s/%s' % (basket_id, key), | 1345 | '_id': ids[key], |
212 | 1313 | 'data': get_flattened_deployment(deployer_config, key), | 1346 | 'name': key, |
213 | 1347 | 'owner': owner, | ||
214 | 1348 | 'basket': basket_id, | ||
215 | 1349 | 'data': data, | ||
216 | 1314 | } | 1350 | } |
217 | 1315 | collection.save(bundle_doc) | 1351 | collection.save(bundle_doc) |
218 | 1316 | 1352 | ||
219 | 1317 | 1353 | ||
220 | === modified file 'charmworld/search.py' | |||
221 | --- charmworld/search.py 2013-08-09 18:53:40 +0000 | |||
222 | +++ charmworld/search.py 2013-08-11 23:10:56 +0000 | |||
223 | @@ -32,6 +32,7 @@ | |||
224 | 32 | 'description': 3, | 32 | 'description': 3, |
225 | 33 | 'config.options.description': None, | 33 | 'config.options.description': None, |
226 | 34 | 'relations': None, | 34 | 'relations': None, |
227 | 35 | 'services': None, | ||
228 | 35 | } | 36 | } |
229 | 36 | 37 | ||
230 | 37 | 38 | ||
231 | @@ -430,9 +431,9 @@ | |||
232 | 430 | 431 | ||
233 | 431 | with Timer() as timer: | 432 | with Timer() as timer: |
234 | 432 | status = self.get_status() | 433 | status = self.get_status() |
238 | 433 | for real_index in self.get_aliased(): | 434 | try: |
239 | 434 | break | 435 | real_index = self.get_aliased()[0] |
240 | 435 | else: | 436 | except IndexError: |
241 | 436 | real_index = self.index_name | 437 | real_index = self.index_name |
242 | 437 | docs = status[real_index].get('docs') | 438 | docs = status[real_index].get('docs') |
243 | 438 | if docs is None: | 439 | if docs is None: |
244 | 439 | 440 | ||
245 | === modified file 'charmworld/testing/factory.py' | |||
246 | --- charmworld/testing/factory.py 2013-08-09 18:53:40 +0000 | |||
247 | +++ charmworld/testing/factory.py 2013-08-11 23:10:56 +0000 | |||
248 | @@ -11,7 +11,6 @@ | |||
249 | 11 | ) | 11 | ) |
250 | 12 | import hashlib | 12 | import hashlib |
251 | 13 | import os | 13 | import os |
252 | 14 | from os.path import basename | ||
253 | 15 | from random import randint | 14 | from random import randint |
254 | 16 | import random | 15 | import random |
255 | 17 | import stat | 16 | import stat |
256 | @@ -311,17 +310,29 @@ | |||
257 | 311 | return record_id, charm | 310 | return record_id, charm |
258 | 312 | 311 | ||
259 | 313 | 312 | ||
261 | 314 | def get_bundle_data(name=None, owner='', basket=None, inherits=None, | 313 | def get_bundle_data(name=None, owner=None, basket=None, inherits=None, |
262 | 315 | series='precise', title='', description='', | 314 | series='precise', title='', description='', |
263 | 316 | services=dict(), relations=dict(), promulgated=False, | 315 | services=dict(), relations=dict(), promulgated=False, |
264 | 317 | branch_deleted=False): | 316 | branch_deleted=False): |
265 | 318 | if name is None: | 317 | if name is None: |
266 | 319 | name = random_string(10) | 318 | name = random_string(10) |
267 | 319 | if owner is None: | ||
268 | 320 | owner = random_string(10) | ||
269 | 320 | if basket is None: | 321 | if basket is None: |
274 | 321 | basket = random_string(10) | 322 | basket = "%s/%d" % (random_string(10), randint(0, 100)) |
275 | 322 | doctype = 'bundle' | 323 | data = dict(series=series, |
276 | 323 | doctype # Hush lint's dislike of defining variables for the vars() call. | 324 | relations=relations, |
277 | 324 | return dict(**vars()) | 325 | services=services) |
278 | 326 | return dict(basket=basket, | ||
279 | 327 | branch_deleted=branch_deleted, | ||
280 | 328 | data=data, | ||
281 | 329 | description=description, | ||
282 | 330 | doctype='bundle', | ||
283 | 331 | inherits=inherits, | ||
284 | 332 | name=name, | ||
285 | 333 | owner=owner, | ||
286 | 334 | promulgated=promulgated, | ||
287 | 335 | title=title) | ||
288 | 325 | 336 | ||
289 | 326 | 337 | ||
290 | 327 | def makeBundle(db, *args, **kwargs): | 338 | def makeBundle(db, *args, **kwargs): |
291 | @@ -334,7 +345,8 @@ | |||
292 | 334 | 345 | ||
293 | 335 | def make_charm_file(db, charm, path, content=None): | 346 | def make_charm_file(db, charm, path, content=None): |
294 | 336 | fs = getfs(db) | 347 | fs = getfs(db) |
296 | 337 | charm_file = CharmFileSet.make_charm_file(fs, charm, path, basename(path)) | 348 | charm_file = CharmFileSet.make_charm_file( |
297 | 349 | fs, charm, path, os.path.basename(path)) | ||
298 | 338 | if content is None: | 350 | if content is None: |
299 | 339 | content = random_string(10).encode('utf-8') | 351 | content = random_string(10).encode('utf-8') |
300 | 340 | charm_file.save(content) | 352 | charm_file.save(content) |
301 | 341 | 353 | ||
302 | === modified file 'charmworld/tests/test_models.py' | |||
303 | --- charmworld/tests/test_models.py 2013-08-09 18:53:40 +0000 | |||
304 | +++ charmworld/tests/test_models.py 2013-08-11 23:10:56 +0000 | |||
305 | @@ -1458,6 +1458,7 @@ | |||
306 | 1458 | 1458 | ||
307 | 1459 | def test_store_bundles(self): | 1459 | def test_store_bundles(self): |
308 | 1460 | # The bundles can be stored in the database. | 1460 | # The bundles can be stored in the database. |
309 | 1461 | |||
310 | 1461 | deployer_config = dedent("""\ | 1462 | deployer_config = dedent("""\ |
311 | 1462 | wordpress-stage: | 1463 | wordpress-stage: |
312 | 1463 | series: precise | 1464 | series: precise |
313 | @@ -1470,13 +1471,21 @@ | |||
314 | 1470 | engine: apache | 1471 | engine: apache |
315 | 1471 | """) | 1472 | """) |
316 | 1472 | parsed = yaml.safe_load(deployer_config) | 1473 | parsed = yaml.safe_load(deployer_config) |
318 | 1473 | store_bundles(self.db.bundles, parsed, 'wordpress-5') | 1474 | owner = 'bac' |
319 | 1475 | bundle_name = 'wordpress-stage' | ||
320 | 1476 | basket_id = 'wordpress-basket/5' | ||
321 | 1477 | _id = "~%s/%s/%s" % (owner, basket_id, bundle_name) | ||
322 | 1478 | store_bundles( | ||
323 | 1479 | self.db.bundles, parsed, 'bac', basket_id) | ||
324 | 1474 | self.assertEqual( | 1480 | self.assertEqual( |
325 | 1475 | { | 1481 | { |
327 | 1476 | '_id': 'wordpress-5/wordpress-stage', | 1482 | '_id': _id, |
328 | 1483 | 'basket': basket_id, | ||
329 | 1484 | 'name': bundle_name, | ||
330 | 1485 | 'owner': owner, | ||
331 | 1477 | 'data': parsed['wordpress-stage'], | 1486 | 'data': parsed['wordpress-stage'], |
332 | 1478 | }, | 1487 | }, |
334 | 1479 | self.db.bundles.find_one('wordpress-5/wordpress-stage')) | 1488 | self.db.bundles.find_one(_id)) |
335 | 1480 | 1489 | ||
336 | 1481 | def test_store_bundles_calls_get_flattened_deployment(self): | 1490 | def test_store_bundles_calls_get_flattened_deployment(self): |
337 | 1482 | # For every key in the top-level deployment data, | 1491 | # For every key in the top-level deployment data, |
338 | @@ -1509,7 +1518,7 @@ | |||
339 | 1509 | 1518 | ||
340 | 1510 | with patch('charmworld.models.get_flattened_deployment', | 1519 | with patch('charmworld.models.get_flattened_deployment', |
341 | 1511 | get_flattened_deployment): | 1520 | get_flattened_deployment): |
343 | 1512 | store_bundles(self.db.bundles, parsed, 'wordpress-5') | 1521 | store_bundles(self.db.bundles, parsed, 'bac', 'wordpress-basket/5') |
344 | 1513 | self.assertItemsEqual(['wordpress-stage', 'wordpress-prod'], keys) | 1522 | self.assertItemsEqual(['wordpress-stage', 'wordpress-prod'], keys) |
345 | 1514 | 1523 | ||
346 | 1515 | def test_storing_a_bundle_includes_indexing_it(self): | 1524 | def test_storing_a_bundle_includes_indexing_it(self): |
347 | @@ -1527,7 +1536,7 @@ | |||
348 | 1527 | with patch( | 1536 | with patch( |
349 | 1528 | 'charmworld.models.ElasticSearchClient', | 1537 | 'charmworld.models.ElasticSearchClient', |
350 | 1529 | FauxElasticSearchClient): | 1538 | FauxElasticSearchClient): |
352 | 1530 | store_bundles(self.db.bundles, {}, 'wordpress-5') | 1539 | store_bundles(self.db.bundles, {}, 'owner', 'wordpress-basket/5') |
353 | 1531 | 1540 | ||
354 | 1532 | self.assertTrue(FauxElasticSearchClient.index_bundles_called) | 1541 | self.assertTrue(FauxElasticSearchClient.index_bundles_called) |
355 | 1533 | 1542 | ||
356 | @@ -1547,12 +1556,14 @@ | |||
357 | 1547 | 'basket': 'mysql', | 1556 | 'basket': 'mysql', |
358 | 1548 | 'name': 'tiny', | 1557 | 'name': 'tiny', |
359 | 1549 | 'inherits': 'main-mysql', | 1558 | 'inherits': 'main-mysql', |
360 | 1550 | 'series': 'series', | ||
361 | 1551 | 'title': 'Tiny bundle', | 1559 | 'title': 'Tiny bundle', |
362 | 1552 | 'description': 'My Tiny Bundle', | 1560 | 'description': 'My Tiny Bundle', |
363 | 1553 | 'doctype': 'bundle', | 1561 | 'doctype': 'bundle', |
366 | 1554 | 'services': dict(apache=1), | 1562 | 'data': { |
367 | 1555 | 'relations': dict(a=1), | 1563 | 'services': dict(apache=1), |
368 | 1564 | 'relations': dict(a=1), | ||
369 | 1565 | 'series': 'series', | ||
370 | 1566 | }, | ||
371 | 1556 | 'promulgated': True, | 1567 | 'promulgated': True, |
372 | 1557 | 'branch_deleted': True, | 1568 | 'branch_deleted': True, |
373 | 1558 | } | 1569 | } |
374 | 1559 | 1570 | ||
375 | === modified file 'charmworld/tests/test_search.py' | |||
376 | --- charmworld/tests/test_search.py 2013-08-09 18:53:40 +0000 | |||
377 | +++ charmworld/tests/test_search.py 2013-08-11 23:10:56 +0000 | |||
378 | @@ -5,12 +5,14 @@ | |||
379 | 5 | 5 | ||
380 | 6 | 6 | ||
381 | 7 | from datetime import datetime | 7 | from datetime import datetime |
382 | 8 | from textwrap import dedent | ||
383 | 8 | 9 | ||
384 | 9 | from pyelasticsearch import ElasticSearch | 10 | from pyelasticsearch import ElasticSearch |
385 | 10 | from pyelasticsearch.exceptions import ( | 11 | from pyelasticsearch.exceptions import ( |
386 | 11 | ElasticHttpError, | 12 | ElasticHttpError, |
387 | 12 | ElasticHttpNotFoundError, | 13 | ElasticHttpNotFoundError, |
388 | 13 | ) | 14 | ) |
389 | 15 | import yaml | ||
390 | 14 | 16 | ||
391 | 15 | from charmworld.charmstore import ( | 17 | from charmworld.charmstore import ( |
392 | 16 | get_address, | 18 | get_address, |
393 | @@ -19,6 +21,7 @@ | |||
394 | 19 | from charmworld.models import ( | 21 | from charmworld.models import ( |
395 | 20 | Bundle, | 22 | Bundle, |
396 | 21 | Charm, | 23 | Charm, |
397 | 24 | store_bundles, | ||
398 | 22 | ) | 25 | ) |
399 | 23 | from charmworld.search import ( | 26 | from charmworld.search import ( |
400 | 24 | BUNDLE, | 27 | BUNDLE, |
401 | @@ -146,6 +149,38 @@ | |||
402 | 146 | self.assertEqual(1, len(results)) | 149 | self.assertEqual(1, len(results)) |
403 | 147 | self.assertEqual(bundle, results[0]['data']) | 150 | self.assertEqual(bundle, results[0]['data']) |
404 | 148 | 151 | ||
405 | 152 | def test_store_bundles(self): | ||
406 | 153 | # Ensure that bundles stored into the index via the `store_bundles` | ||
407 | 154 | # function are searchable via the index. | ||
408 | 155 | _id = '~abentley/wordpress-basket/5/wordpress-stage' | ||
409 | 156 | deployer_config = dedent("""\ | ||
410 | 157 | wordpress-stage: | ||
411 | 158 | series: precise | ||
412 | 159 | services: | ||
413 | 160 | blog: | ||
414 | 161 | charm: cs:precise/wordpress | ||
415 | 162 | constraints: mem=2 | ||
416 | 163 | options: | ||
417 | 164 | tuning: optimized | ||
418 | 165 | engine: apache | ||
419 | 166 | """) | ||
420 | 167 | parsed = yaml.safe_load(deployer_config) | ||
421 | 168 | client = self.index_client | ||
422 | 169 | store_bundles(None, parsed, 'abentley', 'wordpress-basket/5', | ||
423 | 170 | index_client=client) | ||
424 | 171 | # Bundle name is indexed. | ||
425 | 172 | results = client.search('wordpress-stage')['results']['bundle'] | ||
426 | 173 | self.assertEqual(1, len(results)) | ||
427 | 174 | self.assertEqual(_id, results[0]['data'].id) | ||
428 | 175 | # Series is indexed. | ||
429 | 176 | results = client.search('precise')['results']['bundle'] | ||
430 | 177 | self.assertEqual(1, len(results)) | ||
431 | 178 | self.assertEqual(_id, results[0]['data'].id) | ||
432 | 179 | # Owner is indexed. | ||
433 | 180 | results = client.search('abentley')['results']['bundle'] | ||
434 | 181 | self.assertEqual(1, len(results)) | ||
435 | 182 | self.assertEqual(_id, results[0]['data'].id) | ||
436 | 183 | |||
437 | 149 | def test_search_charms_and_bundles_same_name(self): | 184 | def test_search_charms_and_bundles_same_name(self): |
438 | 150 | charm = Charm(self.makeCharm(name='mozilla')) | 185 | charm = Charm(self.makeCharm(name='mozilla')) |
439 | 151 | bundle = Bundle(self.makeBundle(name='mozilla')) | 186 | bundle = Bundle(self.makeBundle(name='mozilla')) |
440 | 152 | 187 | ||
441 | === modified file 'charmworld/views/api.py' | |||
442 | --- charmworld/views/api.py 2013-08-07 13:31:20 +0000 | |||
443 | +++ charmworld/views/api.py 2013-08-11 23:10:56 +0000 | |||
444 | @@ -351,11 +351,12 @@ | |||
445 | 351 | if path is None: | 351 | if path is None: |
446 | 352 | raise HTTPNotFound(self.request.path) | 352 | raise HTTPNotFound(self.request.path) |
447 | 353 | fullpath = '/'.join(path) | 353 | fullpath = '/'.join(path) |
449 | 354 | if len(path) == 3: | 354 | if len(path) == 4: |
450 | 355 | # We have an owner. | 355 | # We have an owner. |
451 | 356 | query = {'_id': fullpath} | 356 | query = {'_id': fullpath} |
454 | 357 | elif len(path) == 2: | 357 | elif len(path) == 3: |
455 | 358 | query = {'basket': path[0], 'name': path[1], 'promulgated': True} | 358 | basket_id = join(path[0], path[1]) |
456 | 359 | query = {'basket': basket_id, 'name': path[2], 'promulgated': True} | ||
457 | 359 | else: | 360 | else: |
458 | 360 | raise HTTPNotFound(self.request.path) | 361 | raise HTTPNotFound(self.request.path) |
459 | 361 | bundle_data = self.request.db.bundles.find_one(query) | 362 | bundle_data = self.request.db.bundles.find_one(query) |
460 | @@ -363,7 +364,7 @@ | |||
461 | 363 | return json_response( | 364 | return json_response( |
462 | 364 | 404, {'type': 'no_such_bundle', 'bundle_id': fullpath}) | 365 | 404, {'type': 'no_such_bundle', 'bundle_id': fullpath}) |
463 | 365 | bundle = Bundle(bundle_data) | 366 | bundle = Bundle(bundle_data) |
465 | 366 | return bundle._representation # wrong | 367 | return {bundle.name: bundle.data} |
466 | 367 | 368 | ||
467 | 368 | @staticmethod | 369 | @staticmethod |
468 | 369 | def _get_file_headers(md5sum, content_type=None): | 370 | def _get_file_headers(md5sum, content_type=None): |
469 | @@ -371,7 +372,7 @@ | |||
470 | 371 | ("Access-Control-Allow-Origin", "*"), | 372 | ("Access-Control-Allow-Origin", "*"), |
471 | 372 | ("Access-Control-Allow-Headers", "X-Requested-With"), | 373 | ("Access-Control-Allow-Headers", "X-Requested-With"), |
472 | 373 | ("Cache-Control", "max-age=86400, public"), | 374 | ("Cache-Control", "max-age=86400, public"), |
474 | 374 | ("Etag", '"%s"' % md5sum) | 375 | ("Etag", '"%s"' % md5sum), |
475 | 375 | ] | 376 | ] |
476 | 376 | if content_type is not None: | 377 | if content_type is not None: |
477 | 377 | headerlist.append(('Content-Type', content_type)) | 378 | headerlist.append(('Content-Type', content_type)) |
478 | 378 | 379 | ||
479 | === modified file 'charmworld/views/tests/test_api.py' | |||
480 | --- charmworld/views/tests/test_api.py 2013-08-09 15:46:51 +0000 | |||
481 | +++ charmworld/views/tests/test_api.py 2013-08-11 23:10:56 +0000 | |||
482 | @@ -635,15 +635,16 @@ | |||
483 | 635 | bundle = self.makeBundle() | 635 | bundle = self.makeBundle() |
484 | 636 | expected = dict( | 636 | expected = dict( |
485 | 637 | name=bundle.name, | 637 | name=bundle.name, |
487 | 638 | owner='', | 638 | owner=bundle.owner, |
488 | 639 | basket=bundle.basket, | 639 | basket=bundle.basket, |
489 | 640 | inherits=None, | 640 | inherits=None, |
490 | 641 | series='precise', | ||
491 | 642 | title='', | 641 | title='', |
492 | 642 | data=dict(services=dict(), | ||
493 | 643 | relations=dict(), | ||
494 | 644 | series='precise', | ||
495 | 645 | ), | ||
496 | 643 | description='', | 646 | description='', |
497 | 644 | doctype='bundle', | 647 | doctype='bundle', |
498 | 645 | services=dict(), | ||
499 | 646 | relations=dict(), | ||
500 | 647 | promulgated=False, | 648 | promulgated=False, |
501 | 648 | branch_deleted=False, | 649 | branch_deleted=False, |
502 | 649 | ) | 650 | ) |
503 | @@ -667,7 +668,7 @@ | |||
504 | 667 | 668 | ||
505 | 668 | def test_not_found(self): | 669 | def test_not_found(self): |
506 | 669 | self.makeBundle(name='bat') | 670 | self.makeBundle(name='bat') |
508 | 670 | response = self.get_response('bundle', 'foo/bar') | 671 | response = self.get_response('bundle', 'foo/4/bar') |
509 | 671 | self.assertEqual(404, response.status_code) | 672 | self.assertEqual(404, response.status_code) |
510 | 672 | 673 | ||
511 | 673 | def test_no_path(self): | 674 | def test_no_path(self): |
512 | @@ -675,32 +676,28 @@ | |||
513 | 675 | self.get_response('bundle', remainder=None) | 676 | self.get_response('bundle', remainder=None) |
514 | 676 | 677 | ||
515 | 677 | def test_valid_lookup_unpromulgated(self): | 678 | def test_valid_lookup_unpromulgated(self): |
518 | 678 | self.makeBundle(name='bat', owner='bac', basket='byobu-4') | 679 | self.makeBundle(name='bat', owner='bac', basket='byobu/4') |
519 | 679 | response = self.get_response('bundle', '~bac/byobu-4/bat') | 680 | response = self.get_response('bundle', '~bac/byobu/4/bat') |
520 | 680 | self.assertEqual(200, response.status_code) | 681 | self.assertEqual(200, response.status_code) |
522 | 681 | response = self.get_response('bundle', 'byobu-4/bat') | 682 | response = self.get_response('bundle', 'byobu/4/bat') |
523 | 682 | self.assertEqual(404, response.status_code) | 683 | self.assertEqual(404, response.status_code) |
524 | 683 | 684 | ||
525 | 684 | def test_valid_lookup_promulgated(self): | 685 | def test_valid_lookup_promulgated(self): |
526 | 685 | self.makeBundle( | 686 | self.makeBundle( |
528 | 686 | name='bat', owner='bac', basket='byobu-4', | 687 | name='bat', owner='bac', basket='byobu/4', |
529 | 687 | promulgated=True) | 688 | promulgated=True) |
530 | 688 | # Can look up via owner-based id and short id. | 689 | # Can look up via owner-based id and short id. |
532 | 689 | response = self.get_response('bundle', '~bac/byobu-4/bat') | 690 | response = self.get_response('bundle', '~bac/byobu/4/bat') |
533 | 690 | self.assertEqual(200, response.status_code) | 691 | self.assertEqual(200, response.status_code) |
535 | 691 | response = self.get_response('bundle', 'byobu-4/bat') | 692 | response = self.get_response('bundle', 'byobu/4/bat') |
536 | 692 | self.assertEqual(200, response.status_code) | 693 | self.assertEqual(200, response.status_code) |
537 | 693 | 694 | ||
538 | 694 | def test_results_match(self): | 695 | def test_results_match(self): |
542 | 695 | self.maxDiff = None | 696 | bundle = self.makeBundle(name='bat', owner='bac', basket='byobu/4') |
543 | 696 | bundle = self.makeBundle(name='bat', owner='bac', basket='byobu-4') | 697 | response = self.get_response('bundle', '~bac/byobu/4/bat') |
541 | 697 | response = self.get_response('bundle', '~bac/byobu-4/bat') | ||
544 | 698 | self.assertEqual(200, response.status_code) | 698 | self.assertEqual(200, response.status_code) |
545 | 699 | self.assertEqual(bundle.id, response.json['_id']) | ||
546 | 700 | # Copy the json dict so we can modify it. | ||
547 | 701 | json = dict(response.json) | 699 | json = dict(response.json) |
550 | 702 | del json['_id'] | 700 | self.assertEqual(json['bat'], bundle.data) |
549 | 703 | self.assertEqual(json, bundle._representation) | ||
551 | 704 | 701 | ||
552 | 705 | 702 | ||
553 | 706 | class TestAPI2Bundles(TestAPIBundles, API2Mixin): | 703 | class TestAPI2Bundles(TestAPIBundles, API2Mixin): |
The branch looks good. Here are some semi-bikesheddy comments.
It's kind of strange that construct_basket_id returns two values while
all the other "construct_*_id" functions just return the ID. Perhaps
two functions would be better. (?)
Does "make lint" complain about the new unused variables?
Re. get_bundle_data: so now if you don't specify an owner argument you
get an empty string, but if you pass owner=None you get a random string?
That seems a little odd. Oh, maybe you did that because the change in
behavior broke a bunch of tests.
Another thing, about get_bundle_data: I always thought it was weird to
use the local namespace as a sandbox in which to construct the return
value. Now that you're doing more in the function and having to do the
"del rep[k]" at the end it's getting more strained. You might consider
revamping the function to use **charm_data as the parameter list instead
and then manipulating charm_data instead of the locals.