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

[Aaron Bentley]

Remove BzrIngestJob.

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.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2013-06-14 21:08:36 +0000
+++ charmworld/jobs/ingest.py	2013-06-21 20:10:35 +0000
@@ -76,7 +76,7 @@
         pass
 
     def run(self, charm_data):
-        raise NotImplemented
+        raise NotImplementedError
 
 
 class DBIngestJob(IngestJob):
@@ -97,95 +97,101 @@
         self.fs = fs
 
 
-class BzrIngestJob(FSIngestJob):
-
-    name = 'bzr'
-
-    def setup(self, root_dir=None, db=None, fs=None):
-        super(BzrIngestJob, self).setup(db, fs)
-        if not root_dir:
-            root_dir = CHARM_DIR
-        self.root_dir = root_dir
-        if not os.path.exists(self.root_dir):
-            os.makedirs(self.root_dir)
-
-    def store_branch_files(self, charm_data):
-        """Process the bzr branch for files that need to be stored in gridfs.
-        """
-        self.log.info('Storing files of branch into gridfs')
-        filestore = CharmFileSet.save_files(
-            self.fs, charm_data, charm_data['branch_dir'], self.log)
-        self.log.info('Completed gridfs storage.')
-        return filestore
-
-    def add_files(self, charm_data):
-        charm_data['files'] = dict([
-            (quote_key(cfile.filename), dict(cfile)) for cfile in
-            self.store_branch_files(charm_data)
-        ])
-        return charm_data
-
-    def checkout_charm(self, charm_data, branch_dir):
-        # The branch has never been seen before. Original branch.
-        self.log.info("Branching charm lp:%s", charm_data["branch_spec"])
+def do_bzr_update(charm_data, root_dir=None, db=None, fs=None, retry=True):
+    """Fetch a branch from bzr, and augment charm data."""
+    job = FSIngestJob()
+    job.setup(db, fs)
+    if not root_dir:
+        root_dir = CHARM_DIR
+    root_dir = root_dir
+    if not os.path.exists(root_dir):
+        os.makedirs(root_dir)
+    update_charm_files(root_dir, job.fs, charm_data, job.log, retry)
+
+
+def update_charm_branch(root_dir, fs, charm_data, branch_dir, log,
+                        retry=False):
+    log.debug("Updating branch lp:%s", charm_data["branch_spec"])
+    try:
         subprocess.check_output(
-            ["/usr/bin/bzr", "co", "-q",
-             "lp:%s" % charm_data["branch_spec"], branch_dir])
-        charm_data = self.add_files(charm_data)
-
-    def charm_is_current(self, charm_data, branch_dir):
-        # It exists and check if it's the latest revision already.
-        self.log.debug(
-            "Existing charm from lp:%s", charm_data["branch_spec"])
-        transport = get_transport(branch_dir)
-        branch = Branch.open_from_transport(transport)
-        cur_rev_id = branch.last_revision()
-        return cur_rev_id == charm_data['commit']
-
-    def update_charm(self, charm_data, branch_dir, retry=False):
-        self.log.debug("Updating branch lp:%s", charm_data["branch_spec"])
-        try:
-            subprocess.check_output(
-                ["/usr/bin/bzr", "update", "-q"],
-                cwd=branch_dir,
-                stderr=subprocess.STDOUT)
-            charm_data = self.add_files(charm_data)
-        except subprocess.CalledProcessError:
-            # Update failed for some reason; destroy it and start over.
-            if retry:
-                shutil.rmtree(branch_dir)
-                return self.run(charm_data, retry=False)
-            raise
-
-    def run(self, charm_data, retry=True):
-        """Fetch a branch from bzr, and augment charm data."""
-        if charm_data['branch_deleted']:
-            return
-        branch_dir = os.path.abspath(
-            str(os.path.join(self.root_dir,
-                             charm_data["series"],
-                             charm_data["owner"],
-                             charm_data["name"],
-                             charm_data["bname"])))
-
-        if not os.path.exists(os.path.dirname(branch_dir)):
-            os.makedirs(os.path.dirname(branch_dir))
-        # Store the branch directory
-        charm_data["branch_dir"] = branch_dir
-
-        if not os.path.exists(branch_dir):
-            # Charm doesn't exist; check it out.
-            self.checkout_charm(charm_data, branch_dir)
-            return
-        elif self.charm_is_current(charm_data, branch_dir):
-            # Charm exists, and is current; log and finish.
-            charm_data = self.add_files(charm_data)
-            self.log.debug(
-                "Already up to date lp:%s", charm_data["branch_spec"])
-            return
-        else:
-            # Charm exists, but needs updating; update it.
-            self.update_charm(charm_data, branch_dir, retry)
+            ["/usr/bin/bzr", "update", "-q"],
+            cwd=branch_dir,
+            stderr=subprocess.STDOUT)
+        charm_data = add_files(fs, charm_data, log)
+    except subprocess.CalledProcessError:
+        # Update failed for some reason; destroy it and start over.
+        if retry:
+            shutil.rmtree(branch_dir)
+            return update_charm_files(root_dir, fs, charm_data, log,
+                                      retry=False)
+        raise
+
+
+def charm_branch_is_current(charm_data, branch_dir, log):
+    # It exists and check if it's the latest revision already.
+    log.debug(
+        "Existing charm from lp:%s", charm_data["branch_spec"])
+    transport = get_transport(branch_dir)
+    branch = Branch.open_from_transport(transport)
+    cur_rev_id = branch.last_revision()
+    return cur_rev_id == charm_data['commit']
+
+
+def checkout_charm(fs, charm_data, branch_dir, log):
+    # The branch has never been seen before. Original branch.
+    log.info("Branching charm lp:%s", charm_data["branch_spec"])
+    subprocess.check_output(
+        ["/usr/bin/bzr", "co", "-q",
+         "lp:%s" % charm_data["branch_spec"], branch_dir])
+    charm_data = add_files(fs, charm_data, log)
+
+
+def store_branch_files(fs, charm_data, log):
+    """Process the bzr branch for files that need to be stored in gridfs.
+    """
+    log.info('Storing files of branch into gridfs')
+    filestore = CharmFileSet.save_files(
+        fs, charm_data, charm_data['branch_dir'], log)
+    log.info('Completed gridfs storage.')
+    return filestore
+
+
+def add_files(fs, charm_data, log):
+    charm_data['files'] = dict([
+        (quote_key(cfile.filename), dict(cfile)) for cfile in
+        store_branch_files(fs, charm_data, log)
+    ])
+    return charm_data
+
+
+def update_charm_files(root_dir, fs, charm_data, log, retry):
+    if charm_data['branch_deleted']:
+        return
+    branch_dir = os.path.abspath(
+        str(os.path.join(root_dir,
+                         charm_data["series"],
+                         charm_data["owner"],
+                         charm_data["name"],
+                         charm_data["bname"])))
+
+    if not os.path.exists(os.path.dirname(branch_dir)):
+        os.makedirs(os.path.dirname(branch_dir))
+    # Store the branch directory
+    charm_data["branch_dir"] = branch_dir
+
+    if not os.path.exists(branch_dir):
+        # Charm doesn't exist; check it out.
+        checkout_charm(fs, charm_data, branch_dir, log)
+        return
+    elif charm_branch_is_current(charm_data, branch_dir, log):
+        # Charm exists, and is current; log and finish.
+        charm_data = add_files(fs, charm_data, log)
+        log.debug(
+            "Already up to date lp:%s", charm_data["branch_spec"])
+        return
+    else:
+        # Charm exists, but needs updating; update it.
+        update_charm_branch(charm_data, branch_dir, retry)
 
 
 def log(stage, level, exc, charm_data, tb=None):
@@ -228,8 +234,7 @@
     charm_data.pop('error', None)
     log = logging.getLogger("charm.update_charm")
     try:
-        if not run_job(BzrIngestJob(), charm_data, db=db):
-            return False
+        do_bzr_update(charm_data, db=db)
         if not run_job(StoreIngestJob(), charm_data):
             return False
         if not run_job(ProofIngestJob(), charm_data):
 
=== modified file 'charmworld/jobs/tests/test_bzr.py'
--- charmworld/jobs/tests/test_bzr.py	2013-06-07 12:36:30 +0000
+++ charmworld/jobs/tests/test_bzr.py	2013-06-21 20:10:35 +0000
@@ -11,8 +11,10 @@
 
 
 from charmworld.jobs.ingest import (
-    BzrIngestJob,
+    add_files,
+    do_bzr_update,
     ChangelogIngestJob,
+    FSIngestJob,
 )
 from charmworld.testing import (
     factory,
@@ -27,38 +29,32 @@
 ZRH = 'Z. Random Hacker <[e-mail]>'
 
 
-class TestBzrJob(JobTestBase):
+class TestBzrOperations(JobTestBase):
 
     def setUp(self):
-        super(TestBzrJob, self).setUp()
-        self.test_dir = 'test_bzr_job'
+        super(TestBzrOperations, self).setUp()
+        self.test_dir = 'test_bzr_operations'
         os.mkdir(self.test_dir)
 
     def tearDown(self):
         shutil.rmtree(self.test_dir)
-        super(TestBzrJob, self).tearDown()
+        super(TestBzrOperations, self).tearDown()
 
     def test_run_creates_charm_directory(self):
-        job = BzrIngestJob()
-        job.setup(root_dir=self.test_dir, db=self.db)
         ignore, charm = factory.makeCharm(self.db, branch_root=self.test_dir)
-        with patch.object(job, 'checkout_charm'):
-            job.run(charm)
+        with patch('charmworld.jobs.ingest.checkout_charm'):
+            do_bzr_update(charm, self.test_dir, self.db)
         expected_path = os.path.abspath(os.path.join(
             self.test_dir, charm['series'], charm['owner'], charm['name']))
         self.assertTrue(os.path.exists(expected_path))
 
     def test_run_checkouts_new_charms(self):
-        job = BzrIngestJob()
-        job.setup(root_dir=self.test_dir, db=self.db)
         ignore, charm = factory.makeCharm(self.db, branch_root=self.test_dir)
-        with patch.object(job, 'checkout_charm') as mock:
-            job.run(charm)
+        with patch('charmworld.jobs.ingest.checkout_charm') as mock:
+            do_bzr_update(charm, self.test_dir, self.db)
         self.assertTrue(mock.called)
 
     def test_run_updates_exising_charms_if_not_current(self):
-        job = BzrIngestJob()
-        job.setup(root_dir=self.test_dir, db=self.db)
         ignore, charm = factory.makeCharm(self.db, branch_root=self.test_dir)
         charm_path = os.path.abspath(os.path.join(
             self.test_dir,
@@ -67,14 +63,13 @@
             charm['name'],
             charm['bname']))
         os.makedirs(charm_path)
-        with patch.object(job, 'charm_is_current', lambda x, y: False):
-            with patch.object(job, 'update_charm') as mock:
-                job.run(charm)
+        with patch('charmworld.jobs.ingest.charm_branch_is_current',
+                   lambda x, y, z: False):
+            with patch('charmworld.jobs.ingest.update_charm_branch') as mock:
+                do_bzr_update(charm, self.test_dir, self.db)
         self.assertTrue(mock.called)
 
     def test_run_does_not_update_exising_charms_if_current(self):
-        job = BzrIngestJob()
-        job.setup(root_dir=self.test_dir, db=self.db)
         ignore, charm = factory.makeCharm(self.db, branch_root=self.test_dir)
         charm_path = os.path.abspath(os.path.join(
             self.test_dir,
@@ -83,10 +78,12 @@
             charm['name'],
             charm['bname']))
         os.makedirs(charm_path)
-        with patch.object(job, 'charm_is_current', lambda x, y: True):
-            with patch.object(job, 'add_files'):
-                with patch.object(job, 'update_charm') as mock:
-                    job.run(charm)
+        with patch('charmworld.jobs.ingest.charm_branch_is_current',
+                   lambda x, y, z: True):
+            with patch('charmworld.jobs.ingest.add_files'):
+                with patch(
+                    'charmworld.jobs.ingest.update_charm_branch') as mock:
+                    do_bzr_update(charm, self.test_dir, self.db)
         self.assertFalse(mock.called)
 
     def test_add_files(self):
@@ -98,17 +95,15 @@
             'name': 'sample_charm',
             'bname': 'trunk',
         }
-        job = BzrIngestJob()
+        job = FSIngestJob()
         job.setup(db=self.db)
-        job.add_files(charm_data)
+        add_files(job.fs, charm_data, job.log)
         self.assertIn('files', charm_data)
         self.assertNotIn('icon', charm_data)
 
     def test_deleted_branch(self):
-        # If the Launchpad branch of a charm is deleted, BzrJob does
+        # If the Launchpad branch of a charm is deleted, do_bzr_update does
         # not do anything.
-        job = BzrIngestJob()
-        job.setup(root_dir=self.test_dir, db=self.db)
         ignore, charm = factory.makeCharm(
             self.db, branch_root=self.test_dir, branch_deleted=True)
         charm_path = os.path.abspath(os.path.join(
@@ -117,11 +112,15 @@
             charm['owner'],
             charm['name'],
             charm['bname']))
-        with patch.object(job, 'checkout_charm') as checkout_mock:
-            with patch.object(job, 'charm_is_current') as is_current_mock:
-                with patch.object(job, 'add_files') as add_files_mock:
-                    with patch.object(job, 'update_charm') as update_mock:
-                        job.run(charm)
+        with patch('charmworld.jobs.ingest.checkout_charm') as checkout_mock:
+            with patch('charmworld.jobs.ingest.charm_branch_is_current') as (
+                    is_current_mock):
+                with patch('charmworld.jobs.ingest.add_files') as (
+                    add_files_mock):
+                    with patch(
+                        'charmworld.jobs.ingest.update_charm_branch') as (
+                            update_mock):
+                        do_bzr_update(charm, self.test_dir, self.db)
         self.assertFalse(os.path.exists(charm_path))
         self.assertFalse(checkout_mock.called)
         self.assertFalse(is_current_mock.called)
 
=== modified file 'charmworld/jobs/tests/test_ingest.py'
--- charmworld/jobs/tests/test_ingest.py	2013-06-07 12:36:30 +0000
+++ charmworld/jobs/tests/test_ingest.py	2013-06-21 20:10:35 +0000
@@ -19,6 +19,7 @@
 
 from charmworld.charmstore import get_address
 from charmworld.jobs.ingest import (
+    add_files,
     IngestError,
     IngestJob,
     run_job,
@@ -64,16 +65,16 @@
 
 @contextmanager
 def update_environment(real_charm_data, index_client):
-    def checkout_charm(self, charm_data, branch_dir):
+    def checkout_charm(fs, charm_data, branch_dir, log):
         wt = BzrDir.create_standalone_workingtree(branch_dir)
         wt.bzrdir.root_transport.mkdir('hooks')
         wt.bzrdir.root_transport.put_bytes('metadata.yaml', json.dumps({
             'name': charm_data['name'],
             'summary': real_charm_data['summary'],
         }))
-        self.add_files(charm_data)
+        add_files(fs, charm_data, log)
     checkout_patch = patch(
-        'charmworld.jobs.ingest.BzrIngestJob.checkout_charm', checkout_charm)
+        'charmworld.jobs.ingest.checkout_charm', checkout_charm)
 
     @urlmatch(path='/charm-info')
     def mock_store_data(url, request):
@@ -150,15 +151,16 @@
         # Induce improbable error -- bname must be a string.
         charm_data['bname'] = None
         update_handler = self.get_handler('charm.update')
-        bzr_handler = self.get_handler('charm.bzr')
+        update_charm_handler = self.get_handler('charm.update_charm')
         run_job(UpdateCharmJob(), self.payload(charm_data), db=self.db)
         self.assertIsNot(
             None, self.db.charms.find_one(charm_data['_id']))
         logs = ''.join(r.getMessage() for r in update_handler.buffer)
-        bzr_logs = ''.join(r.getMessage() for r in bzr_handler.buffer)
+        update_charm_logs = ''.join(r.getMessage()
+                                    for r in update_charm_handler.buffer)
         self.assertEqual('Saving %s' % charm_data['_id'], logs)
         self.assertIn("'NoneType' object has no attribute 'startswith'",
-                      bzr_logs)
+                      update_charm_logs)
 
     def test_updates_promulgated(self):
         charm_data = factory.get_charm_json(promulgated=False)
 
=== modified file 'charmworld/jobs/tests/test_scan.py'
--- charmworld/jobs/tests/test_scan.py	2013-05-27 10:26:40 +0000
+++ charmworld/jobs/tests/test_scan.py	2013-06-21 20:10:35 +0000
@@ -1,12 +1,13 @@
 # Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-from charmworld.jobs.ingest import BzrIngestJob
-from charmworld.jobs.ingest import IngestError
-from charmworld.jobs.ingest import ScanIngestJob
+from charmworld.jobs.ingest import (
+    IngestError,
+    ScanIngestJob,
+    store_branch_files,
+)
 from charmworld.testing import factory
 from charmworld.testing import JobTestBase
-from charmworld.testing import REAL_FILE_CHARM_DIR
 from charmworld.utils import quote_key
 
 
@@ -14,14 +15,12 @@
 
     def setUp(self):
         super(TestScanJob, self).setUp()
-        self.bzr_job = BzrIngestJob()
-        self.bzr_job.setup(db=self.db, root_dir=REAL_FILE_CHARM_DIR)
         self.scan_job = ScanIngestJob()
         self.scan_job.setup(db=self.db)
         ignore, self.charm = factory.makeCharm(self.db, with_real_files=True)
         self.charm['files'] = dict([
             (quote_key(cfile.filename), dict(cfile)) for cfile in
-            self.bzr_job.store_branch_files(self.charm)
+            store_branch_files(self.scan_job.fs, self.charm, self.scan_job.log)
         ])
 
     def test_config_metadata_with_dotted_keys(self):
 
=== modified file 'charmworld/views/tests/test_charms.py'
--- charmworld/views/tests/test_charms.py	2013-06-19 20:45:35 +0000
+++ charmworld/views/tests/test_charms.py	2013-06-21 20:10:35 +0000
@@ -1,7 +1,10 @@
 # Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-from charmworld.jobs.ingest import BzrIngestJob
+from charmworld.jobs.ingest import (
+    add_files,
+    FSIngestJob,
+)
 from charmworld.models import Charm
 from charmworld.models import CharmSource
 from charmworld.testing import factory
@@ -282,9 +285,9 @@
         ignore, charm = factory.makeCharm(
             self.db, owner='bar', series='precise',
             promulgated=promulgated, with_real_files=True)
-        bzr_job = BzrIngestJob()
-        bzr_job.setup(root_dir=None, db=self.db)
-        bzr_job.add_files(charm)
+        fs_job = FSIngestJob()
+        fs_job.setup(db=self.db)
+        add_files(fs_job.fs, charm, fs_job.log)
         self.db.charms.update({'_id': charm['_id']}, charm)
         return charm