Merge lp:~abentley/charmworld/remove-bzr-ingest-job into lp:~juju-jitsu/charmworld/trunk

Proposed by Aaron Bentley on 2013-06-21
Status: Merged
Approved by: Curtis Hovey on 2013-06-24
Approved revision: 282
Merged at revision: 280
Proposed branch: lp:~abentley/charmworld/remove-bzr-ingest-job
Merge into: lp:~juju-jitsu/charmworld/trunk
Diff against target: 471 lines (+150/-142)
5 files modified
charmworld/jobs/ingest.py (+96/-91)
charmworld/jobs/tests/test_bzr.py (+33/-34)
charmworld/jobs/tests/test_ingest.py (+8/-6)
charmworld/jobs/tests/test_scan.py (+6/-7)
charmworld/views/tests/test_charms.py (+7/-4)
To merge this branch: bzr merge lp:~abentley/charmworld/remove-bzr-ingest-job
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code 2013-06-21 Approve on 2013-06-24
Review via email: mp+170899@code.launchpad.net

Commit message

Remove BzrIngestJob.

Description of the change

This branch removes BzrIngestJob, converting its functionality into a set of functions.

Follow-on branches will remove other Job functionality, such as ScanIngestJob and FSIngestJob, and refactor the functions themselves into sensible collections of functionlality.

The main entry point is now do_bzr_update, which basically gathers resources and then calls update_charm_branch.

To post a comment you must log in.
Curtis Hovey (sinzui) wrote :

Thank you.

review: Approve (code)

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-06-14 21:08:36 +0000
3+++ charmworld/jobs/ingest.py 2013-06-21 20:10:35 +0000
4@@ -76,7 +76,7 @@
5 pass
6
7 def run(self, charm_data):
8- raise NotImplemented
9+ raise NotImplementedError
10
11
12 class DBIngestJob(IngestJob):
13@@ -97,95 +97,101 @@
14 self.fs = fs
15
16
17-class BzrIngestJob(FSIngestJob):
18-
19- name = 'bzr'
20-
21- def setup(self, root_dir=None, db=None, fs=None):
22- super(BzrIngestJob, self).setup(db, fs)
23- if not root_dir:
24- root_dir = CHARM_DIR
25- self.root_dir = root_dir
26- if not os.path.exists(self.root_dir):
27- os.makedirs(self.root_dir)
28-
29- def store_branch_files(self, charm_data):
30- """Process the bzr branch for files that need to be stored in gridfs.
31- """
32- self.log.info('Storing files of branch into gridfs')
33- filestore = CharmFileSet.save_files(
34- self.fs, charm_data, charm_data['branch_dir'], self.log)
35- self.log.info('Completed gridfs storage.')
36- return filestore
37-
38- def add_files(self, charm_data):
39- charm_data['files'] = dict([
40- (quote_key(cfile.filename), dict(cfile)) for cfile in
41- self.store_branch_files(charm_data)
42- ])
43- return charm_data
44-
45- def checkout_charm(self, charm_data, branch_dir):
46- # The branch has never been seen before. Original branch.
47- self.log.info("Branching charm lp:%s", charm_data["branch_spec"])
48+def do_bzr_update(charm_data, root_dir=None, db=None, fs=None, retry=True):
49+ """Fetch a branch from bzr, and augment charm data."""
50+ job = FSIngestJob()
51+ job.setup(db, fs)
52+ if not root_dir:
53+ root_dir = CHARM_DIR
54+ root_dir = root_dir
55+ if not os.path.exists(root_dir):
56+ os.makedirs(root_dir)
57+ update_charm_files(root_dir, job.fs, charm_data, job.log, retry)
58+
59+
60+def update_charm_branch(root_dir, fs, charm_data, branch_dir, log,
61+ retry=False):
62+ log.debug("Updating branch lp:%s", charm_data["branch_spec"])
63+ try:
64 subprocess.check_output(
65- ["/usr/bin/bzr", "co", "-q",
66- "lp:%s" % charm_data["branch_spec"], branch_dir])
67- charm_data = self.add_files(charm_data)
68-
69- def charm_is_current(self, charm_data, branch_dir):
70- # It exists and check if it's the latest revision already.
71- self.log.debug(
72- "Existing charm from lp:%s", charm_data["branch_spec"])
73- transport = get_transport(branch_dir)
74- branch = Branch.open_from_transport(transport)
75- cur_rev_id = branch.last_revision()
76- return cur_rev_id == charm_data['commit']
77-
78- def update_charm(self, charm_data, branch_dir, retry=False):
79- self.log.debug("Updating branch lp:%s", charm_data["branch_spec"])
80- try:
81- subprocess.check_output(
82- ["/usr/bin/bzr", "update", "-q"],
83- cwd=branch_dir,
84- stderr=subprocess.STDOUT)
85- charm_data = self.add_files(charm_data)
86- except subprocess.CalledProcessError:
87- # Update failed for some reason; destroy it and start over.
88- if retry:
89- shutil.rmtree(branch_dir)
90- return self.run(charm_data, retry=False)
91- raise
92-
93- def run(self, charm_data, retry=True):
94- """Fetch a branch from bzr, and augment charm data."""
95- if charm_data['branch_deleted']:
96- return
97- branch_dir = os.path.abspath(
98- str(os.path.join(self.root_dir,
99- charm_data["series"],
100- charm_data["owner"],
101- charm_data["name"],
102- charm_data["bname"])))
103-
104- if not os.path.exists(os.path.dirname(branch_dir)):
105- os.makedirs(os.path.dirname(branch_dir))
106- # Store the branch directory
107- charm_data["branch_dir"] = branch_dir
108-
109- if not os.path.exists(branch_dir):
110- # Charm doesn't exist; check it out.
111- self.checkout_charm(charm_data, branch_dir)
112- return
113- elif self.charm_is_current(charm_data, branch_dir):
114- # Charm exists, and is current; log and finish.
115- charm_data = self.add_files(charm_data)
116- self.log.debug(
117- "Already up to date lp:%s", charm_data["branch_spec"])
118- return
119- else:
120- # Charm exists, but needs updating; update it.
121- self.update_charm(charm_data, branch_dir, retry)
122+ ["/usr/bin/bzr", "update", "-q"],
123+ cwd=branch_dir,
124+ stderr=subprocess.STDOUT)
125+ charm_data = add_files(fs, charm_data, log)
126+ except subprocess.CalledProcessError:
127+ # Update failed for some reason; destroy it and start over.
128+ if retry:
129+ shutil.rmtree(branch_dir)
130+ return update_charm_files(root_dir, fs, charm_data, log,
131+ retry=False)
132+ raise
133+
134+
135+def charm_branch_is_current(charm_data, branch_dir, log):
136+ # It exists and check if it's the latest revision already.
137+ log.debug(
138+ "Existing charm from lp:%s", charm_data["branch_spec"])
139+ transport = get_transport(branch_dir)
140+ branch = Branch.open_from_transport(transport)
141+ cur_rev_id = branch.last_revision()
142+ return cur_rev_id == charm_data['commit']
143+
144+
145+def checkout_charm(fs, charm_data, branch_dir, log):
146+ # The branch has never been seen before. Original branch.
147+ log.info("Branching charm lp:%s", charm_data["branch_spec"])
148+ subprocess.check_output(
149+ ["/usr/bin/bzr", "co", "-q",
150+ "lp:%s" % charm_data["branch_spec"], branch_dir])
151+ charm_data = add_files(fs, charm_data, log)
152+
153+
154+def store_branch_files(fs, charm_data, log):
155+ """Process the bzr branch for files that need to be stored in gridfs.
156+ """
157+ log.info('Storing files of branch into gridfs')
158+ filestore = CharmFileSet.save_files(
159+ fs, charm_data, charm_data['branch_dir'], log)
160+ log.info('Completed gridfs storage.')
161+ return filestore
162+
163+
164+def add_files(fs, charm_data, log):
165+ charm_data['files'] = dict([
166+ (quote_key(cfile.filename), dict(cfile)) for cfile in
167+ store_branch_files(fs, charm_data, log)
168+ ])
169+ return charm_data
170+
171+
172+def update_charm_files(root_dir, fs, charm_data, log, retry):
173+ if charm_data['branch_deleted']:
174+ return
175+ branch_dir = os.path.abspath(
176+ str(os.path.join(root_dir,
177+ charm_data["series"],
178+ charm_data["owner"],
179+ charm_data["name"],
180+ charm_data["bname"])))
181+
182+ if not os.path.exists(os.path.dirname(branch_dir)):
183+ os.makedirs(os.path.dirname(branch_dir))
184+ # Store the branch directory
185+ charm_data["branch_dir"] = branch_dir
186+
187+ if not os.path.exists(branch_dir):
188+ # Charm doesn't exist; check it out.
189+ checkout_charm(fs, charm_data, branch_dir, log)
190+ return
191+ elif charm_branch_is_current(charm_data, branch_dir, log):
192+ # Charm exists, and is current; log and finish.
193+ charm_data = add_files(fs, charm_data, log)
194+ log.debug(
195+ "Already up to date lp:%s", charm_data["branch_spec"])
196+ return
197+ else:
198+ # Charm exists, but needs updating; update it.
199+ update_charm_branch(charm_data, branch_dir, retry)
200
201
202 def log(stage, level, exc, charm_data, tb=None):
203@@ -228,8 +234,7 @@
204 charm_data.pop('error', None)
205 log = logging.getLogger("charm.update_charm")
206 try:
207- if not run_job(BzrIngestJob(), charm_data, db=db):
208- return False
209+ do_bzr_update(charm_data, db=db)
210 if not run_job(StoreIngestJob(), charm_data):
211 return False
212 if not run_job(ProofIngestJob(), charm_data):
213
214=== modified file 'charmworld/jobs/tests/test_bzr.py'
215--- charmworld/jobs/tests/test_bzr.py 2013-06-07 12:36:30 +0000
216+++ charmworld/jobs/tests/test_bzr.py 2013-06-21 20:10:35 +0000
217@@ -11,8 +11,10 @@
218
219
220 from charmworld.jobs.ingest import (
221- BzrIngestJob,
222+ add_files,
223+ do_bzr_update,
224 ChangelogIngestJob,
225+ FSIngestJob,
226 )
227 from charmworld.testing import (
228 factory,
229@@ -27,38 +29,32 @@
230 ZRH = 'Z. Random Hacker <zrandom@example.com>'
231
232
233-class TestBzrJob(JobTestBase):
234+class TestBzrOperations(JobTestBase):
235
236 def setUp(self):
237- super(TestBzrJob, self).setUp()
238- self.test_dir = 'test_bzr_job'
239+ super(TestBzrOperations, self).setUp()
240+ self.test_dir = 'test_bzr_operations'
241 os.mkdir(self.test_dir)
242
243 def tearDown(self):
244 shutil.rmtree(self.test_dir)
245- super(TestBzrJob, self).tearDown()
246+ super(TestBzrOperations, self).tearDown()
247
248 def test_run_creates_charm_directory(self):
249- job = BzrIngestJob()
250- job.setup(root_dir=self.test_dir, db=self.db)
251 ignore, charm = factory.makeCharm(self.db, branch_root=self.test_dir)
252- with patch.object(job, 'checkout_charm'):
253- job.run(charm)
254+ with patch('charmworld.jobs.ingest.checkout_charm'):
255+ do_bzr_update(charm, self.test_dir, self.db)
256 expected_path = os.path.abspath(os.path.join(
257 self.test_dir, charm['series'], charm['owner'], charm['name']))
258 self.assertTrue(os.path.exists(expected_path))
259
260 def test_run_checkouts_new_charms(self):
261- job = BzrIngestJob()
262- job.setup(root_dir=self.test_dir, db=self.db)
263 ignore, charm = factory.makeCharm(self.db, branch_root=self.test_dir)
264- with patch.object(job, 'checkout_charm') as mock:
265- job.run(charm)
266+ with patch('charmworld.jobs.ingest.checkout_charm') as mock:
267+ do_bzr_update(charm, self.test_dir, self.db)
268 self.assertTrue(mock.called)
269
270 def test_run_updates_exising_charms_if_not_current(self):
271- job = BzrIngestJob()
272- job.setup(root_dir=self.test_dir, db=self.db)
273 ignore, charm = factory.makeCharm(self.db, branch_root=self.test_dir)
274 charm_path = os.path.abspath(os.path.join(
275 self.test_dir,
276@@ -67,14 +63,13 @@
277 charm['name'],
278 charm['bname']))
279 os.makedirs(charm_path)
280- with patch.object(job, 'charm_is_current', lambda x, y: False):
281- with patch.object(job, 'update_charm') as mock:
282- job.run(charm)
283+ with patch('charmworld.jobs.ingest.charm_branch_is_current',
284+ lambda x, y, z: False):
285+ with patch('charmworld.jobs.ingest.update_charm_branch') as mock:
286+ do_bzr_update(charm, self.test_dir, self.db)
287 self.assertTrue(mock.called)
288
289 def test_run_does_not_update_exising_charms_if_current(self):
290- job = BzrIngestJob()
291- job.setup(root_dir=self.test_dir, db=self.db)
292 ignore, charm = factory.makeCharm(self.db, branch_root=self.test_dir)
293 charm_path = os.path.abspath(os.path.join(
294 self.test_dir,
295@@ -83,10 +78,12 @@
296 charm['name'],
297 charm['bname']))
298 os.makedirs(charm_path)
299- with patch.object(job, 'charm_is_current', lambda x, y: True):
300- with patch.object(job, 'add_files'):
301- with patch.object(job, 'update_charm') as mock:
302- job.run(charm)
303+ with patch('charmworld.jobs.ingest.charm_branch_is_current',
304+ lambda x, y, z: True):
305+ with patch('charmworld.jobs.ingest.add_files'):
306+ with patch(
307+ 'charmworld.jobs.ingest.update_charm_branch') as mock:
308+ do_bzr_update(charm, self.test_dir, self.db)
309 self.assertFalse(mock.called)
310
311 def test_add_files(self):
312@@ -98,17 +95,15 @@
313 'name': 'sample_charm',
314 'bname': 'trunk',
315 }
316- job = BzrIngestJob()
317+ job = FSIngestJob()
318 job.setup(db=self.db)
319- job.add_files(charm_data)
320+ add_files(job.fs, charm_data, job.log)
321 self.assertIn('files', charm_data)
322 self.assertNotIn('icon', charm_data)
323
324 def test_deleted_branch(self):
325- # If the Launchpad branch of a charm is deleted, BzrJob does
326+ # If the Launchpad branch of a charm is deleted, do_bzr_update does
327 # not do anything.
328- job = BzrIngestJob()
329- job.setup(root_dir=self.test_dir, db=self.db)
330 ignore, charm = factory.makeCharm(
331 self.db, branch_root=self.test_dir, branch_deleted=True)
332 charm_path = os.path.abspath(os.path.join(
333@@ -117,11 +112,15 @@
334 charm['owner'],
335 charm['name'],
336 charm['bname']))
337- with patch.object(job, 'checkout_charm') as checkout_mock:
338- with patch.object(job, 'charm_is_current') as is_current_mock:
339- with patch.object(job, 'add_files') as add_files_mock:
340- with patch.object(job, 'update_charm') as update_mock:
341- job.run(charm)
342+ with patch('charmworld.jobs.ingest.checkout_charm') as checkout_mock:
343+ with patch('charmworld.jobs.ingest.charm_branch_is_current') as (
344+ is_current_mock):
345+ with patch('charmworld.jobs.ingest.add_files') as (
346+ add_files_mock):
347+ with patch(
348+ 'charmworld.jobs.ingest.update_charm_branch') as (
349+ update_mock):
350+ do_bzr_update(charm, self.test_dir, self.db)
351 self.assertFalse(os.path.exists(charm_path))
352 self.assertFalse(checkout_mock.called)
353 self.assertFalse(is_current_mock.called)
354
355=== modified file 'charmworld/jobs/tests/test_ingest.py'
356--- charmworld/jobs/tests/test_ingest.py 2013-06-07 12:36:30 +0000
357+++ charmworld/jobs/tests/test_ingest.py 2013-06-21 20:10:35 +0000
358@@ -19,6 +19,7 @@
359
360 from charmworld.charmstore import get_address
361 from charmworld.jobs.ingest import (
362+ add_files,
363 IngestError,
364 IngestJob,
365 run_job,
366@@ -64,16 +65,16 @@
367
368 @contextmanager
369 def update_environment(real_charm_data, index_client):
370- def checkout_charm(self, charm_data, branch_dir):
371+ def checkout_charm(fs, charm_data, branch_dir, log):
372 wt = BzrDir.create_standalone_workingtree(branch_dir)
373 wt.bzrdir.root_transport.mkdir('hooks')
374 wt.bzrdir.root_transport.put_bytes('metadata.yaml', json.dumps({
375 'name': charm_data['name'],
376 'summary': real_charm_data['summary'],
377 }))
378- self.add_files(charm_data)
379+ add_files(fs, charm_data, log)
380 checkout_patch = patch(
381- 'charmworld.jobs.ingest.BzrIngestJob.checkout_charm', checkout_charm)
382+ 'charmworld.jobs.ingest.checkout_charm', checkout_charm)
383
384 @urlmatch(path='/charm-info')
385 def mock_store_data(url, request):
386@@ -150,15 +151,16 @@
387 # Induce improbable error -- bname must be a string.
388 charm_data['bname'] = None
389 update_handler = self.get_handler('charm.update')
390- bzr_handler = self.get_handler('charm.bzr')
391+ update_charm_handler = self.get_handler('charm.update_charm')
392 run_job(UpdateCharmJob(), self.payload(charm_data), db=self.db)
393 self.assertIsNot(
394 None, self.db.charms.find_one(charm_data['_id']))
395 logs = ''.join(r.getMessage() for r in update_handler.buffer)
396- bzr_logs = ''.join(r.getMessage() for r in bzr_handler.buffer)
397+ update_charm_logs = ''.join(r.getMessage()
398+ for r in update_charm_handler.buffer)
399 self.assertEqual('Saving %s' % charm_data['_id'], logs)
400 self.assertIn("'NoneType' object has no attribute 'startswith'",
401- bzr_logs)
402+ update_charm_logs)
403
404 def test_updates_promulgated(self):
405 charm_data = factory.get_charm_json(promulgated=False)
406
407=== modified file 'charmworld/jobs/tests/test_scan.py'
408--- charmworld/jobs/tests/test_scan.py 2013-05-27 10:26:40 +0000
409+++ charmworld/jobs/tests/test_scan.py 2013-06-21 20:10:35 +0000
410@@ -1,12 +1,13 @@
411 # Copyright 2012, 2013 Canonical Ltd. This software is licensed under the
412 # GNU Affero General Public License version 3 (see the file LICENSE).
413
414-from charmworld.jobs.ingest import BzrIngestJob
415-from charmworld.jobs.ingest import IngestError
416-from charmworld.jobs.ingest import ScanIngestJob
417+from charmworld.jobs.ingest import (
418+ IngestError,
419+ ScanIngestJob,
420+ store_branch_files,
421+)
422 from charmworld.testing import factory
423 from charmworld.testing import JobTestBase
424-from charmworld.testing import REAL_FILE_CHARM_DIR
425 from charmworld.utils import quote_key
426
427
428@@ -14,14 +15,12 @@
429
430 def setUp(self):
431 super(TestScanJob, self).setUp()
432- self.bzr_job = BzrIngestJob()
433- self.bzr_job.setup(db=self.db, root_dir=REAL_FILE_CHARM_DIR)
434 self.scan_job = ScanIngestJob()
435 self.scan_job.setup(db=self.db)
436 ignore, self.charm = factory.makeCharm(self.db, with_real_files=True)
437 self.charm['files'] = dict([
438 (quote_key(cfile.filename), dict(cfile)) for cfile in
439- self.bzr_job.store_branch_files(self.charm)
440+ store_branch_files(self.scan_job.fs, self.charm, self.scan_job.log)
441 ])
442
443 def test_config_metadata_with_dotted_keys(self):
444
445=== modified file 'charmworld/views/tests/test_charms.py'
446--- charmworld/views/tests/test_charms.py 2013-06-19 20:45:35 +0000
447+++ charmworld/views/tests/test_charms.py 2013-06-21 20:10:35 +0000
448@@ -1,7 +1,10 @@
449 # Copyright 2012, 2013 Canonical Ltd. This software is licensed under the
450 # GNU Affero General Public License version 3 (see the file LICENSE).
451
452-from charmworld.jobs.ingest import BzrIngestJob
453+from charmworld.jobs.ingest import (
454+ add_files,
455+ FSIngestJob,
456+)
457 from charmworld.models import Charm
458 from charmworld.models import CharmSource
459 from charmworld.testing import factory
460@@ -282,9 +285,9 @@
461 ignore, charm = factory.makeCharm(
462 self.db, owner='bar', series='precise',
463 promulgated=promulgated, with_real_files=True)
464- bzr_job = BzrIngestJob()
465- bzr_job.setup(root_dir=None, db=self.db)
466- bzr_job.add_files(charm)
467+ fs_job = FSIngestJob()
468+ fs_job.setup(db=self.db)
469+ add_files(fs_job.fs, charm, fs_job.log)
470 self.db.charms.update({'_id': charm['_id']}, charm)
471 return charm
472

Subscribers

People subscribed via source and target branches