Merge lp:~benji/charmworld/expose-bundle-changes into lp:~juju-jitsu/charmworld/trunk
- expose-bundle-changes
- Merge into trunk
Proposed by
Benji York
Status: | Merged |
---|---|
Merged at revision: | 427 |
Proposed branch: | lp:~benji/charmworld/expose-bundle-changes |
Merge into: | lp:~juju-jitsu/charmworld/trunk |
Diff against target: |
445 lines (+122/-69) 10 files modified
charmworld/jobs/ingest.py (+33/-23) charmworld/jobs/tests/test_bzr.py (+51/-22) charmworld/jobs/tests/test_ingest.py (+3/-3) charmworld/migrations/versions/tests/test_migrations.py (+1/-0) charmworld/models.py (+10/-5) charmworld/templates/bundle.pt (+0/-8) charmworld/testing/factory.py (+2/-1) charmworld/tests/test_models.py (+15/-4) charmworld/tests/test_search.py (+3/-2) charmworld/views/tests/test_api.py (+4/-1) |
To merge this branch: | bzr merge lp:~benji/charmworld/expose-bundle-changes |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju-Jitsu Hackers | Pending | ||
Review via email: mp+192389@code.launchpad.net |
Commit message
Description of the change
Add recent bzr changes to bundles.
To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote : | # |
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'charmworld/jobs/ingest.py' |
2 | --- charmworld/jobs/ingest.py 2013-10-17 21:32:57 +0000 |
3 | +++ charmworld/jobs/ingest.py 2013-10-23 18:29:57 +0000 |
4 | @@ -261,7 +261,7 @@ |
5 | update_download_count(store, charm_data) |
6 | update_proof_data(charm_data, log) |
7 | update_jenkins_data(db, charm_data, log) |
8 | - update_from_revisions(charm_data, newest_revision=revision) |
9 | + update_revision_details(charm_data, newest_revision=revision) |
10 | update_date_created(charm_data, log) |
11 | scan_charm(charm_data, fs, log) |
12 | update_hash(charm_data) |
13 | @@ -309,8 +309,11 @@ |
14 | self.working_dir = working_dir |
15 | super(UpdateBundleJob, self).__init__() |
16 | |
17 | - def store_bundles(self, deployer_config, owner, basket_id): |
18 | - store_bundles(self.db.bundles, deployer_config, owner, basket_id) |
19 | + def store_bundles(self, deployer_config, owner, basket_id, first_change, |
20 | + last_change, changes): |
21 | + store_bundles( |
22 | + self.db.bundles, deployer_config, owner, basket_id, first_change, |
23 | + last_change, changes) |
24 | |
25 | @staticmethod |
26 | def set_basket_info(data, revno): |
27 | @@ -319,6 +322,9 @@ |
28 | def decorate_basket(self, basket_data, fs): |
29 | branch_dir = fetch_branch(self.working_dir, basket_data, self.log, |
30 | revisionId=basket_data['commit']) |
31 | + basket_data['branch_dir'] = branch_dir |
32 | + update_revision_details( |
33 | + basket_data, newest_revision=basket_data['commit']) |
34 | branch = Branch.open(branch_dir) |
35 | revno = branch.revision_id_to_revno(basket_data['commit']) |
36 | self.set_basket_info(basket_data, revno) |
37 | @@ -340,7 +346,8 @@ |
38 | self.db.baskets.save(basket_data) |
39 | deployer_config = self.get_deployer_config(fs, basket_data) |
40 | self.store_bundles( |
41 | - deployer_config, basket_data['owner'], basket_data['name_revno']) |
42 | + deployer_config, basket_data['owner'], basket_data['name_revno'], |
43 | + None, None, None) |
44 | |
45 | |
46 | def _rev_info(r, branch): |
47 | @@ -354,27 +361,34 @@ |
48 | return d |
49 | |
50 | |
51 | -def update_from_revisions(charm_data, limit=10, since=None, |
52 | - newest_revision=None): |
53 | - if charm_data['branch_deleted']: |
54 | +def find_revision_cutoff(now=None, settings=settings): |
55 | + days_of_revisions = settings.get('days_of_revisions') |
56 | + if days_of_revisions is None: |
57 | + days_of_revisions = 30 |
58 | + if now is None: |
59 | + now = datetime.utcnow() |
60 | + cutoff = now - timedelta(int(days_of_revisions)) |
61 | + return calendar.timegm(cutoff.timetuple()) |
62 | + |
63 | + |
64 | +def update_revision_details(data, limit=10, newest_revision=None): |
65 | + # If the branch has been deleted, there can be no new revisions to pull in. |
66 | + if data['branch_deleted']: |
67 | return |
68 | - if since is None: |
69 | - days_of_revisions = settings.get('days_of_revisions') |
70 | - if days_of_revisions is not None: |
71 | - cutoff = datetime.utcnow() - timedelta(int(days_of_revisions)) |
72 | - since = calendar.timegm(cutoff.timetuple()) |
73 | - branch_dir = charm_data['branch_dir'] |
74 | - changes = get_changes(branch_dir, limit, since, newest_revision) |
75 | - charm_data.update(changes) |
76 | + since = find_revision_cutoff() |
77 | + changes, first_change, last_change = get_changes( |
78 | + data['branch_dir'], limit, since, newest_revision) |
79 | + data['changes'] = changes |
80 | + data['first_change'] = first_change |
81 | + data['last_change'] = last_change |
82 | |
83 | |
84 | def get_changes(branch_dir, limit, since, newest_revision=None): |
85 | - charm_data = {} |
86 | branch = Branch.open(branch_dir) |
87 | branch.lock_read() |
88 | try: |
89 | revisions = get_revisions(branch, limit, since) |
90 | - charm_data['changes'] = changes = [] |
91 | + changes = [] |
92 | # If there is no limit on how new a revision we can include, then |
93 | # consider the starting revision to be already found. |
94 | found_start = newest_revision is None |
95 | @@ -391,11 +405,7 @@ |
96 | last_change = changes[0] |
97 | first = branch.repository.get_revision(branch.get_rev_id(1)) |
98 | first_change = _rev_info(first, branch) |
99 | - charm_data.update({ |
100 | - 'last_change': last_change, |
101 | - 'first_change': first_change, |
102 | - }) |
103 | - return charm_data |
104 | + return changes, first_change, last_change |
105 | finally: |
106 | branch.unlock() |
107 | |
108 | @@ -411,7 +421,7 @@ |
109 | break |
110 | revision = branch.repository.get_revision(revision_id) |
111 | if num >= limit: |
112 | - if since is None or revision.timestamp < since: |
113 | + if revision.timestamp < since: |
114 | break |
115 | revs.append(revision) |
116 | return revs |
117 | |
118 | === modified file 'charmworld/jobs/tests/test_bzr.py' |
119 | --- charmworld/jobs/tests/test_bzr.py 2013-10-17 20:19:55 +0000 |
120 | +++ charmworld/jobs/tests/test_bzr.py 2013-10-23 18:29:57 +0000 |
121 | @@ -3,6 +3,7 @@ |
122 | |
123 | |
124 | from contextlib import contextmanager |
125 | +import datetime |
126 | from logging import getLogger |
127 | import os |
128 | import time |
129 | @@ -13,13 +14,14 @@ |
130 | |
131 | from charmworld.jobs import ingest |
132 | from charmworld.jobs.ingest import ( |
133 | + _rev_info, |
134 | add_files, |
135 | checkout_branch, |
136 | do_bzr_update, |
137 | + find_revision_cutoff, |
138 | get_changes, |
139 | get_revisions, |
140 | - _rev_info, |
141 | - update_from_revisions, |
142 | + update_revision_details, |
143 | ) |
144 | from charmworld.models import getfs |
145 | from charmworld.testing import ( |
146 | @@ -268,36 +270,63 @@ |
147 | self.assertEqual([YRH, ZRH], rev_info['authors']) |
148 | self.assertEqual(JRH, rev_info['committer']) |
149 | |
150 | - def test_get_changes(self): |
151 | + def test_get_changes_with_one_revision(self): |
152 | + tree = self.make_locked_tree() |
153 | + self.num_commit(tree, 1) |
154 | + changes, first_change, last_change = get_changes( |
155 | + 'tree', 10, time.time()) |
156 | + self.assertEqual(first_change, last_change) |
157 | + self.assertEqual([first_change], changes) |
158 | + |
159 | + def test_get_changes_with_multiple_revisions(self): |
160 | tree = self.make_locked_tree() |
161 | first_id = self.num_commit(tree, 1) |
162 | - charm_data = get_changes('tree', 10, time.time()) |
163 | - self.assertItemsEqual(['changes', 'first_change', 'last_change'], |
164 | - charm_data.keys()) |
165 | - self.assertEqual(charm_data['first_change'], charm_data['last_change']) |
166 | - self.assertEqual([charm_data['first_change']], charm_data['changes']) |
167 | for num in range(1, 11): |
168 | self.num_commit(tree, 1) |
169 | - charm_data = get_changes('tree', 10, time.time()) |
170 | - self.assertNotEqual(charm_data['first_change'], |
171 | - charm_data['last_change']) |
172 | - self.assertEqual(charm_data['last_change'], charm_data['changes'][0]) |
173 | - self.assertNotEqual(charm_data['first_change'], |
174 | - charm_data['changes'][-1]) |
175 | - self.assertEqual(self.get_rev_info(tree.branch, first_id), |
176 | - charm_data['first_change']) |
177 | + changes, first_change, last_change = get_changes( |
178 | + 'tree', 10, time.time()) |
179 | + self.assertNotEqual(first_change, last_change) |
180 | + self.assertEqual(last_change, changes[0]) |
181 | + self.assertNotEqual(first_change, changes[-1]) |
182 | + self.assertEqual( |
183 | + self.get_rev_info(tree.branch, first_id), |
184 | + first_change) |
185 | |
186 | def test_get_changes_no_revisions(self): |
187 | self.make_locked_tree() |
188 | - charm_data = get_changes('tree', 10, time.time()) |
189 | - self.assertEqual([], charm_data['changes']) |
190 | - self.assertIs(None, charm_data['first_change']) |
191 | - self.assertIs(None, charm_data['last_change']) |
192 | + changes, first_change, last_change = get_changes( |
193 | + 'tree', 10, time.time()) |
194 | + self.assertEqual([], changes) |
195 | + self.assertIs(None, first_change) |
196 | + self.assertIs(None, last_change) |
197 | |
198 | def test_branch_deleted(self): |
199 | - # update_from_revisions does not do anything if the Launchpad branch |
200 | + # update_revision_details does not do anything if the Launchpad branch |
201 | # of a charm is deleted. |
202 | charm = factory.get_charm_json(branch_deleted=True) |
203 | with patch.object(ingest, 'get_changes') as mock: |
204 | - update_from_revisions(charm) |
205 | + update_revision_details(charm) |
206 | self.assertFalse(mock.called) |
207 | + |
208 | + |
209 | +class TestFindRevisionCutoff(TestCase): |
210 | + """Tests for charmworld.jobs.ingest.find_revision_cutoff.""" |
211 | + |
212 | + def test_return_type(self): |
213 | + # find_revision_cutoff returns seconds since the epoch (an int). |
214 | + self.assertEqual(type(find_revision_cutoff()), int) |
215 | + |
216 | + def test_days_of_revisions_is_respected(self): |
217 | + # If the cutoff date is further in the past, the resulting seconds |
218 | + # since the epoch is further in the past too. |
219 | + old = find_revision_cutoff(settings=dict(days_of_revisions=10)) |
220 | + older = find_revision_cutoff(settings=dict(days_of_revisions=20)) |
221 | + self.assertGreater(old, older) |
222 | + |
223 | + def test_days_are_the_right_size(self): |
224 | + # If we start at time 0 and ask for one day in the past, we get the |
225 | + # obvious value. |
226 | + time = find_revision_cutoff( |
227 | + now=datetime.datetime.utcfromtimestamp(0), |
228 | + settings=dict(days_of_revisions=1)) |
229 | + self.assertEqual(time, -86400) |
230 | |
231 | === modified file 'charmworld/jobs/tests/test_ingest.py' |
232 | --- charmworld/jobs/tests/test_ingest.py 2013-10-17 21:32:57 +0000 |
233 | +++ charmworld/jobs/tests/test_ingest.py 2013-10-23 18:29:57 +0000 |
234 | @@ -533,8 +533,8 @@ |
235 | """) |
236 | fs.put(deployer_config, _id=DEPLOYER_CONFIG_HASH) |
237 | job.store_bundles( |
238 | - yaml.safe_load(deployer_config), |
239 | - basket_data['owner'], basket_data['name_revno']) |
240 | + yaml.safe_load(deployer_config), basket_data['owner'], |
241 | + basket_data['name_revno'], None, None, None) |
242 | self.assertIsNotNone(self.db.bundles.find_one(bundle_id)) |
243 | |
244 | def test_job_run_stores_bundles_in_the_db(self): |
245 | @@ -559,7 +559,7 @@ |
246 | with patch.object(job, 'store_bundles') as store_bundles: |
247 | job.run(basket_data) |
248 | store_bundles.assert_called_with( |
249 | - {'foo': {}}, 'charmers', 'dummy') |
250 | + {'foo': {}}, 'charmers', 'dummy', None, None, None) |
251 | |
252 | |
253 | class TestUpdateCharm(MongoTestBase): |
254 | |
255 | === modified file 'charmworld/migrations/versions/tests/test_migrations.py' |
256 | --- charmworld/migrations/versions/tests/test_migrations.py 2013-09-23 15:58:47 +0000 |
257 | +++ charmworld/migrations/versions/tests/test_migrations.py 2013-10-23 18:29:57 +0000 |
258 | @@ -60,6 +60,7 @@ |
259 | owner, basket_name, bundle_name) |
260 | store_bundles( |
261 | self.db.bundles, parsed, owner, basket_id, |
262 | + None, None, None, |
263 | index_client=self.index_client) |
264 | |
265 | def test_bundles_are_removed_from_elastic_search(self): |
266 | |
267 | === modified file 'charmworld/models.py' |
268 | --- charmworld/models.py 2013-10-22 22:57:51 +0000 |
269 | +++ charmworld/models.py 2013-10-23 18:29:57 +0000 |
270 | @@ -1743,7 +1743,8 @@ |
271 | return options |
272 | |
273 | |
274 | -def make_bundle_doc(data, owner, basket_id, bundle_name): |
275 | +def make_bundle_doc(data, owner, basket_id, bundle_name, first_change, |
276 | + last_change, changes): |
277 | basket_name, basket_revision = basket_id.split('/') |
278 | _id = Bundle.construct_id(owner, basket_name, bundle_name, basket_revision) |
279 | return { |
280 | @@ -1753,11 +1754,14 @@ |
281 | 'basket_name': basket_name, |
282 | 'basket_revision': int(basket_revision), |
283 | 'data': data, |
284 | + 'first_change': first_change, |
285 | + 'last_change': last_change, |
286 | + 'changes': changes, |
287 | } |
288 | |
289 | |
290 | -def store_bundles(collection, deployer_config, owner, basket_id, |
291 | - index_client=None): |
292 | +def store_bundles(collection, deployer_config, owner, basket_id, first_change, |
293 | + last_change, changes, index_client=None): |
294 | """Store a basket of bundles into MongoDB and/or ElasticSearch. |
295 | |
296 | :param: collection: A db bundles collection. If None the bundles are not |
297 | @@ -1779,8 +1783,9 @@ |
298 | for bundle_name in deployer_config: |
299 | # Set up indexing. |
300 | data = get_flattened_deployment(deployer_config, bundle_name) |
301 | - index_data[bundle_name] = make_bundle_doc(data, owner, basket_id, |
302 | - bundle_name) |
303 | + index_data[bundle_name] = make_bundle_doc( |
304 | + data, owner, basket_id, bundle_name, first_change, last_change, |
305 | + changes) |
306 | if collection is not None: |
307 | collection.save(index_data[bundle_name]) |
308 | |
309 | |
310 | === modified file 'charmworld/templates/bundle.pt' |
311 | --- charmworld/templates/bundle.pt 2013-10-11 13:51:05 +0000 |
312 | +++ charmworld/templates/bundle.pt 2013-10-23 18:29:57 +0000 |
313 | @@ -103,14 +103,6 @@ |
314 | </li> |
315 | </ul> |
316 | </div> |
317 | - <tal:comment condition="nothing"> |
318 | - <div tal:condition="is_owner"> |
319 | - You are the owner. In the future we'll show you extra stuff here. |
320 | - </div> |
321 | - <div tal:condition="not: is_owner"> |
322 | - If you were the author/owner you'd see extra stuff here. |
323 | - </div> |
324 | - </tal:comment> |
325 | </metal:block> |
326 | </body> |
327 | </html> |
328 | |
329 | === modified file 'charmworld/testing/factory.py' |
330 | --- charmworld/testing/factory.py 2013-10-04 19:45:06 +0000 |
331 | +++ charmworld/testing/factory.py 2013-10-23 18:29:57 +0000 |
332 | @@ -331,7 +331,8 @@ |
333 | data = dict(series=series, |
334 | relations=relations, |
335 | services=services) |
336 | - bundle_doc = make_bundle_doc(data, owner, basket_with_rev, name) |
337 | + bundle_doc = make_bundle_doc( |
338 | + data, owner, basket_with_rev, name, None, None, None) |
339 | bundle_doc.update(dict(branch_deleted=branch_deleted, |
340 | data=data, |
341 | description=description, |
342 | |
343 | === modified file 'charmworld/tests/test_models.py' |
344 | --- charmworld/tests/test_models.py 2013-10-22 22:57:51 +0000 |
345 | +++ charmworld/tests/test_models.py 2013-10-23 18:29:57 +0000 |
346 | @@ -1595,7 +1595,7 @@ |
347 | basket_id = "%s/%d" % (basket_name, basket_rev) |
348 | _id = Bundle.construct_id(owner, basket_name, bundle_name, basket_rev) |
349 | store_bundles( |
350 | - self.db.bundles, parsed, 'bac', basket_id) |
351 | + self.db.bundles, parsed, 'bac', basket_id, None, None, None) |
352 | self.assertEqual( |
353 | { |
354 | '_id': _id, |
355 | @@ -1604,6 +1604,9 @@ |
356 | 'name': bundle_name, |
357 | 'owner': owner, |
358 | 'data': parsed['wordpress-stage'], |
359 | + 'first_change': None, |
360 | + 'last_change': None, |
361 | + 'changes': None, |
362 | }, |
363 | self.db.bundles.find_one(_id)) |
364 | |
365 | @@ -1638,7 +1641,9 @@ |
366 | |
367 | with patch('charmworld.models.get_flattened_deployment', |
368 | get_flattened_deployment): |
369 | - store_bundles(self.db.bundles, parsed, 'bac', 'wordpress-basket/5') |
370 | + store_bundles( |
371 | + self.db.bundles, parsed, 'bac', 'wordpress-basket/5', None, |
372 | + None, None) |
373 | self.assertItemsEqual(['wordpress-stage', 'wordpress-prod'], keys) |
374 | |
375 | def test_storing_a_bundle_includes_indexing_it(self): |
376 | @@ -1656,7 +1661,9 @@ |
377 | with patch( |
378 | 'charmworld.models.ElasticSearchClient', |
379 | FauxElasticSearchClient): |
380 | - store_bundles(self.db.bundles, {}, 'owner', 'wordpress-basket/5') |
381 | + store_bundles( |
382 | + self.db.bundles, {}, 'owner', 'wordpress-basket/5', None, |
383 | + None, None) |
384 | |
385 | self.assertTrue(FauxElasticSearchClient.index_bundles_called) |
386 | |
387 | @@ -2002,7 +2009,8 @@ |
388 | class TestMakeBundleDoc(TestCase): |
389 | |
390 | def test_bundle_doc(self): |
391 | - doc = make_bundle_doc({'a': 'b'}, 'foo', 'bar/9', 'baz') |
392 | + doc = make_bundle_doc( |
393 | + {'a': 'b'}, 'foo', 'bar/9', 'baz', None, None, None) |
394 | self.assertEqual({ |
395 | 'owner': 'foo', |
396 | 'basket_name': 'bar', |
397 | @@ -2010,6 +2018,9 @@ |
398 | 'name': 'baz', |
399 | 'data': {'a': 'b'}, |
400 | '_id': '~foo/bar/9/baz', |
401 | + 'first_change': None, |
402 | + 'last_change': None, |
403 | + 'changes': None, |
404 | }, doc) |
405 | |
406 | |
407 | |
408 | === modified file 'charmworld/tests/test_search.py' |
409 | --- charmworld/tests/test_search.py 2013-09-16 21:14:06 +0000 |
410 | +++ charmworld/tests/test_search.py 2013-10-23 18:29:57 +0000 |
411 | @@ -166,8 +166,9 @@ |
412 | charm: cs:precise/mysql |
413 | """) |
414 | parsed = yaml.safe_load(deployer_config) |
415 | - store_bundles(None, parsed, 'abentley', 'wordpress-basket/5', |
416 | - index_client=self.index_client) |
417 | + store_bundles( |
418 | + None, parsed, 'abentley', 'wordpress-basket/5', None, None, None, |
419 | + index_client=self.index_client) |
420 | return _id |
421 | |
422 | def test_store_bundles_bundle_name_indexed(self): |
423 | |
424 | === modified file 'charmworld/views/tests/test_api.py' |
425 | --- charmworld/views/tests/test_api.py 2013-10-22 21:02:10 +0000 |
426 | +++ charmworld/views/tests/test_api.py 2013-10-23 18:29:57 +0000 |
427 | @@ -732,6 +732,9 @@ |
428 | description='', |
429 | promulgated=False, |
430 | branch_deleted=False, |
431 | + first_change=None, |
432 | + last_change=None, |
433 | + changes=None, |
434 | ) |
435 | self.assertEqual(expected, bundle._representation) |
436 | |
437 | @@ -858,7 +861,7 @@ |
438 | self.assertEqual(u'text/plain', response.content_type) |
439 | |
440 | def test_bundle_icon(self): |
441 | - # The current design is that all bundles, regardless of their id, will |
442 | + # The current design is that all bundles, regardless of their ID, will |
443 | # get the default bundle icon. |
444 | bundle = self.makeBundle() |
445 | response = self.get_response( |
LGTM
https:/ /codereview. appspot. com/15100046/ diff/1/ charmworld/ jobs/ingest. py jobs/ingest. py (right):
File charmworld/
https:/ /codereview. appspot. com/15100046/ diff/1/ charmworld/ jobs/ingest. py#newcode424 jobs/ingest. py:424: if revision.timestamp < since:
charmworld/
So since cannot be None?
https:/ /codereview. appspot. com/15100046/ diff/1/ charmworld/ jobs/ingest. py#newcode424 jobs/ingest. py:424: if revision.timestamp < since:
charmworld/
So we're sure since can never be None?
https:/ /codereview. appspot. com/15100046/