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

[Aaron Bentley]

Remove remaining sub-Jobs from ingest.

This branch completes the work for bug #1190015: ingest is really a set of functions that are run consecutively, but is structured as a list of jobs.

It removes StoreIngestJob, ChangelogIngestJob and ProofIngestJob. The remaining code has some room for improvement, espeicially some very short functions, but this can be addressed piecemeal.

IngestJob and DBIngestJob are retained because they are parents of the remaining job, UpdateCharmJob.

As a driveby, the 'retry' parameter was removed from do_bzr_update, since it was never used.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2013-06-24 15:45:29 +0000
+++ charmworld/jobs/ingest.py	2013-06-24 19:49:29 +0000
@@ -88,14 +88,14 @@
         self.db = db
 
 
-def do_bzr_update(charm_data, db, fs, log, root_dir=None, retry=True):
+def do_bzr_update(charm_data, db, fs, log, root_dir=None):
     """Fetch a branch from bzr, and augment charm data."""
     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, fs, charm_data, log, retry)
+    update_charm_files(root_dir, fs, charm_data, log)
 
 
 def update_charm_branch(root_dir, fs, charm_data, branch_dir, log,
@@ -153,7 +153,7 @@
     return charm_data
 
 
-def update_charm_files(root_dir, fs, charm_data, log, retry):
+def update_charm_files(root_dir, fs, charm_data, log, retry=True):
     if charm_data['branch_deleted']:
         return
     branch_dir = os.path.abspath(
@@ -225,13 +225,10 @@
     fs = getfs(db)
     try:
         do_bzr_update(charm_data, db, fs, log)
-        if not run_job(StoreIngestJob(), charm_data):
-            return False
-        if not run_job(ProofIngestJob(), charm_data):
-            return False
+        update_from_store(charm_data, CharmStore(), log)
+        update_proof_data(charm_data, log)
         update_jenkins_data(db, charm_data, log)
-        if not run_job(ChangelogIngestJob(), charm_data):
-            return False
+        update_from_revisions(charm_data)
         update_date_created(charm_data, log)
         scan_charm(charm_data, db, fs, log)
     except Exception as e:
@@ -258,78 +255,69 @@
         CharmSource(self.db, index_client).save(charm_data)
 
 
-class ChangelogIngestJob(IngestJob):
-
-    name = 'changelog'
-
-    def __init__(self, limit=10, since=None):
-        self.limit = limit
-        self.since = since
-
-    def setup(self):
-        super(ChangelogIngestJob, self).setup()
-        if self.since is None:
-            days_of_revisions = settings.get('days_of_revisions')
-            if days_of_revisions is not None:
-                cutoff = datetime.utcnow() - timedelta(int(days_of_revisions))
-                self.since = calendar.timegm(cutoff.timetuple())
-
-    @staticmethod
-    def _rev_info(r, branch):
-        d = {
-            'authors': r.get_apparent_authors(),
-            "revno": branch.revision_id_to_revno(r.revision_id),
-            "committer": r.committer,
-            "created": r.timestamp,
-            "message": r.message
-        }
-        return d
-
-    def run(self, charm_data):
-        if charm_data['branch_deleted']:
-            return
-        branch_dir = charm_data["branch_dir"]
-        charm_data.update(self.get_changes(branch_dir))
-
-    def get_changes(self, branch_dir):
-        charm_data = {}
-        branch = Branch.open(branch_dir)
-        branch.lock_read()
-        try:
-            revisions = self.get_revisions(branch)
-            charm_data["changes"] = changes = []
-            for r in revisions:
-                changes.append(self._rev_info(r, branch))
-            if len(revisions) == 0:
-                last_change = None
-                first_change = None
-            else:
-                last_change = changes[0]
-                first = branch.repository.get_revision(branch.get_rev_id(1))
-                first_change = self._rev_info(first, branch)
-            charm_data.update({
-                'last_change': last_change,
-                'first_change': first_change,
-            })
-            return charm_data
-        finally:
-            branch.unlock()
-
-    def get_revisions(self, branch):
-        # We only want the last 10 changes, in descending order.
-        graph = branch.repository.get_graph()
-        cur_rev_id = branch.last_revision()
-        ancestry_iter = graph.iter_lefthand_ancestry(cur_rev_id)
-        revs = []
-        for num, revision_id in enumerate(ancestry_iter):
-            if revision_id == NULL_REVISION:
+def _rev_info(r, branch):
+    d = {
+        'authors': r.get_apparent_authors(),
+        "revno": branch.revision_id_to_revno(r.revision_id),
+        "committer": r.committer,
+        "created": r.timestamp,
+        "message": r.message
+    }
+    return d
+
+
+def update_from_revisions(charm_data, limit=10, since=None):
+    if charm_data['branch_deleted']:
+        return
+    if since is None:
+        days_of_revisions = settings.get('days_of_revisions')
+        if days_of_revisions is not None:
+            cutoff = datetime.utcnow() - timedelta(int(days_of_revisions))
+            since = calendar.timegm(cutoff.timetuple())
+    branch_dir = charm_data["branch_dir"]
+    charm_data.update(get_changes(branch_dir, limit, since))
+
+
+def get_changes(branch_dir, limit, since):
+    charm_data = {}
+    branch = Branch.open(branch_dir)
+    branch.lock_read()
+    try:
+        revisions = get_revisions(branch, limit, since)
+        charm_data["changes"] = changes = []
+        for r in revisions:
+            changes.append(_rev_info(r, branch))
+        if len(revisions) == 0:
+            last_change = None
+            first_change = None
+        else:
+            last_change = changes[0]
+            first = branch.repository.get_revision(branch.get_rev_id(1))
+            first_change = _rev_info(first, branch)
+        charm_data.update({
+            'last_change': last_change,
+            'first_change': first_change,
+        })
+        return charm_data
+    finally:
+        branch.unlock()
+
+
+def get_revisions(branch, limit, since):
+    # We only want the last 10 changes, in descending order.
+    graph = branch.repository.get_graph()
+    cur_rev_id = branch.last_revision()
+    ancestry_iter = graph.iter_lefthand_ancestry(cur_rev_id)
+    revs = []
+    for num, revision_id in enumerate(ancestry_iter):
+        if revision_id == NULL_REVISION:
+            break
+        revision = branch.repository.get_revision(revision_id)
+        if num >= limit:
+            if since is None or revision.timestamp < since:
                 break
-            revision = branch.repository.get_revision(revision_id)
-            if num >= self.limit:
-                if self.since is None or revision.timestamp < self.since:
-                    break
-            revs.append(revision)
-        return revs
+        revs.append(revision)
+    return revs
 
 
 def update_jenkins_data(db, charm, log):
@@ -414,66 +402,62 @@
     return (result_id, result['result'])
 
 
-class ProofIngestJob(IngestJob):
-
-    name = 'proof'
-
-    @contextlib.contextmanager
-    def _get_proof_lib(self, new_path):
-        if new_path not in sys.path:
-            sys.path.append(new_path)
-        try:
-            import lib.proof as prooflib
-            yield prooflib
-        except ImportError:
-            yield None
-        finally:
-            if new_path in sys.path:
-                sys.path.remove(new_path)
-
-    def setup(self, proofer=None, proof_path=None):
-        if not proofer:
-            proofer = self.get_proofer(proof_path)
-        self.proofer = proofer
-
-    def get_proofer(self, proof_path=None):
-        proofer = None
-        if not proof_path:
-            proof_path = CHARM_PROOF_PATH
-
-        # Use config.CHARM_PROOF_PATH for testing. Monkeypatching is scary.
-        if not os.path.isdir(proof_path):
-            err_msg = ("proof error before processing began: could not find "
-                       "charm proof path.")
-            self.log.exception(err_msg)
-            self.log.exception("CHARM_PROOF_PATH: %s", proof_path)
-        with self._get_proof_lib(proof_path) as prooflib:
-            if not prooflib:
-                err_msg = ("proof error before processing began: could not "
-                           "import charm proof lib.")
-                self.log.exception(err_msg)
-                self.log.exception(
-                    "CHARM_PROOF_PATH: %s", proof_path)
-            else:
-                proofer = prooflib.run
-        return proofer
-
-    def run(self, charm):
-        if not self.proofer:
-            self.log.exception("proof aborted.")
-            raise Exception("No proofer")
-        if charm['branch_deleted']:
-            return
-        proof = {}
-        lint, exit_code = self.proofer(charm['branch_dir'])
-        for line in lint:
-            if not ':' in line:
-                continue
-            level, msg = line.split(':', 1)
-            if level == "W" and 'name' in msg:
-                continue
-            proof.setdefault(level.lower(), []).append(msg)
-        charm['proof'] = proof
[e-mail]
+def get_proof_lib(new_path):
+    if new_path not in sys.path:
+        sys.path.append(new_path)
+    try:
+        import lib.proof as prooflib
+        yield prooflib
+    except ImportError:
+        yield None
+    finally:
+        if new_path in sys.path:
+            sys.path.remove(new_path)
+
+
+def get_proofer(log, proof_path=None):
+    proofer = None
+    if not proof_path:
+        proof_path = CHARM_PROOF_PATH
+
+    # Use config.CHARM_PROOF_PATH for testing. Monkeypatching is scary.
+    if not os.path.isdir(proof_path):
+        err_msg = ("proof error before processing began: could not find "
+                   "charm proof path.")
+        log.exception(err_msg)
+        log.exception("CHARM_PROOF_PATH: %s", proof_path)
+    with get_proof_lib(proof_path) as prooflib:
+        if not prooflib:
+            err_msg = ("proof error before processing began: could not "
+                       "import charm proof lib.")
+            log.exception(err_msg)
+            log.exception(
+                "CHARM_PROOF_PATH: %s", proof_path)
+        else:
+            proofer = prooflib.run
+    return proofer
+
+
+def update_proof_data(charm, log, _proofer=None):
+    proofer = _proofer
+    if proofer is None:
+        proofer = get_proofer(log)
+    if not proofer:
+        log.exception("proof aborted.")
+        raise Exception("No proofer")
+    if charm['branch_deleted']:
+        return
+    proof = {}
+    lint, exit_code = proofer(charm['branch_dir'])
+    for line in lint:
+        if not ':' in line:
+            continue
+        level, msg = line.split(':', 1)
+        if level == "W" and 'name' in msg:
+            continue
+        proof.setdefault(level.lower(), []).append(msg)
+    charm['proof'] = proof
 
 
 def scan_charm(charm_data, db, fs, log):
@@ -639,37 +623,24 @@
     charm['date_created'] = timestamp(date_created.replace(microsecond=0))
 
 
-class StoreIngestJob(IngestJob):
-
-    name = 'store'
-
-    def __init__(self, store=None):
-        super(StoreIngestJob, self).__init__()
-        if store is None:
-            store = CharmStore()
-        self.store = store
-
-    def run(self, charm):
-        old_address = None
-        for address in addresses(charm):
-            if old_address is not None:
-                self.log.info("rechecking %s with %s", old_address, address)
-            old_address = address
-            data = self._store_get(address)
-            if 'errors' not in data and 'warnings' not in data:
-                break
-
-        if 'errors' in data or 'warnings' in data:
-            self.log.warning("store error on %s %s" % (address, data))
-
-        data["store_checked"] = datetime.now().ctime()
-
-        charm['store_data'] = data
-        charm['store_url'] = make_store_url(data['revision'], address)
-        update_download_count(self.store, charm)
-
-    def _store_get(self, address):
-        return CharmStore().get_charm_info(address)
+def update_from_store(charm, store, log):
+    old_address = None
+    for address in addresses(charm):
+        if old_address is not None:
+            log.info("rechecking %s with %s", old_address, address)
+        old_address = address
+        data = store.get_charm_info(address)
+        if 'errors' not in data and 'warnings' not in data:
+            break
+
+    if 'errors' in data or 'warnings' in data:
+        log.warning("store error on %s %s" % (address, data))
+
+    data["store_checked"] = datetime.now().ctime()
+
+    charm['store_data'] = data
+    charm['store_url'] = make_store_url(data['revision'], address)
+    update_download_count(store, charm)
 
 
 # XXX j.c.sackett Jan 31 2013 Bug:1111708 scan_repo is swapped for

=== modified file 'charmworld/jobs/tests/test_bzr.py'
--- charmworld/jobs/tests/test_bzr.py	2013-06-24 15:45:29 +0000
+++ charmworld/jobs/tests/test_bzr.py	2013-06-24 19:49:29 +0000
@@ -6,15 +6,20 @@
 from logging import getLogger
 import os
 import shutil
+import time
 
 from bzrlib.bzrdir import BzrDir
 from mock import patch
 
 
+from charmworld.jobs import ingest
 from charmworld.jobs.ingest import (
     add_files,
     do_bzr_update,
-    ChangelogIngestJob,
+    get_changes,
+    get_revisions,
+    _rev_info,
+    update_from_revisions,
 )
 from charmworld.models import getfs
 from charmworld.testing import (
@@ -169,24 +174,23 @@
 
     def test_get_revisions(self):
         tree = self.make_locked_tree()
-        job = ChangelogIngestJob()
-        revisions = job.get_revisions(tree.branch)
+        revisions = get_revisions(tree.branch, 10, 0)
         self.assertEqual([], revisions)
         for x in range(9):
             self.num_commit(tree, x)
-        revisions = job.get_revisions(tree.branch)
+        revisions = get_revisions(tree.branch, 10, 0)
         self.assertEqual(["revision %d" % num for num in range(8, -1, -1)],
                          [revision.message for revision in revisions])
         self.num_commit(tree, 9)
-        revisions = job.get_revisions(tree.branch)
+        revisions = get_revisions(tree.branch, 10, time.time())
         self.assertEqual(["revision %d" % num for num in range(9, -1, -1)],
                          [revision.message for revision in revisions])
         self.num_commit(tree, 10)
-        revisions = job.get_revisions(tree.branch)
+        revisions = get_revisions(tree.branch, 10, time.time())
         self.assertEqual(["revision %d" % num for num in range(10, 0, -1)],
                          [revision.message for revision in revisions])
         self.num_commit(tree, 11)
-        revisions = job.get_revisions(tree.branch)
+        revisions = get_revisions(tree.branch, 10, time.time())
         self.assertEqual(["revision %d" % num for num in range(11, 1, -1)],
                          [revision.message for revision in revisions])
 
@@ -194,9 +198,9 @@
         tree = self.make_locked_tree()
         for x in range(15):
             self.num_commit(tree, x)
-        revisions = ChangelogIngestJob(limit=5).get_revisions(tree.branch)
+        revisions = get_revisions(tree.branch, 5, time.time())
         self.assertEqual(5, len(revisions))
-        revisions = ChangelogIngestJob(limit=20).get_revisions(tree.branch)
+        revisions = get_revisions(tree.branch, 20, time.time())
         self.assertEqual(15, len(revisions))
 
     def test_get_revisions_honours_since(self):
@@ -204,11 +208,9 @@
         tree.commit('', timestamp=0, committer='')
         tree.commit('', timestamp=1, committer='')
         tree.commit('', timestamp=2, committer='')
-        job = ChangelogIngestJob(since=0, limit=0)
-        revisions = job.get_revisions(tree.branch)
+        revisions = get_revisions(tree.branch, 0, 0)
         self.assertEqual(3, len(revisions))
-        job = ChangelogIngestJob(since=1, limit=0)
-        revisions = job.get_revisions(tree.branch)
+        revisions = get_revisions(tree.branch, 0, 1)
         self.assertEqual(2, len(revisions))
 
     def test_limit_overrides_since(self):
@@ -216,8 +218,7 @@
         tree.commit('', timestamp=0, committer='')
         tree.commit('', timestamp=1, committer='')
         tree.commit('', timestamp=2, committer='')
-        job = ChangelogIngestJob(since=1, limit=3)
-        revisions = job.get_revisions(tree.branch)
+        revisions = get_revisions(tree.branch, since=1, limit=3)
         self.assertEqual(3, len(revisions))
 
     def test_rev_info(self):
@@ -225,7 +226,7 @@
         revision_id = tree.commit('message1', committer=JRH)
         rev = tree.branch.repository.get_revision(revision_id)
         rev.timestamp = 34
-        rev_info = ChangelogIngestJob._rev_info(rev, tree.branch)
+        rev_info = _rev_info(rev, tree.branch)
         self.assertEqual({
             'authors': [JRH],
             'committer': JRH,
@@ -236,8 +237,7 @@
 
     @staticmethod
     def get_rev_info(branch, revision_id):
-        rev = branch.repository.get_revision(revision_id)
-        return ChangelogIngestJob._rev_info(rev, branch)
+        return _rev_info(branch.repository.get_revision(revision_id), branch)
 
     def test_rev_info_supplied_authors(self):
         tree = self.make_locked_tree()
@@ -250,15 +250,14 @@
     def test_get_changes(self):
         tree = self.make_locked_tree()
         first_id = self.num_commit(tree, 1)
-        job = ChangelogIngestJob()
-        charm_data = job.get_changes('tree')
+        charm_data = get_changes('tree', 10, time.time())
         self.assertItemsEqual(['changes', 'first_change', 'last_change'],
                               charm_data.keys())
         self.assertEqual(charm_data['first_change'], charm_data['last_change'])
         self.assertEqual([charm_data['first_change']], charm_data['changes'])
         for num in range(1, 11):
             self.num_commit(tree, 1)
-        charm_data = job.get_changes('tree')
+        charm_data = get_changes('tree', 10, time.time())
         self.assertNotEqual(charm_data['first_change'],
                             charm_data['last_change'])
         self.assertEqual(charm_data['last_change'], charm_data['changes'][0])
@@ -269,16 +268,15 @@
 
     def test_get_changes_no_revisions(self):
         self.make_locked_tree()
-        charm_data = ChangelogIngestJob().get_changes('tree')
+        charm_data = get_changes('tree', 10, time.time())
         self.assertEqual([], charm_data['changes'])
         self.assertIs(None, charm_data['first_change'])
         self.assertIs(None, charm_data['last_change'])
 
     def test_branch_deleted(self):
-        # ChangelogIngestJob does not do anything if the Launchpad branch
+        # update_from_revisions does not do anything if the Launchpad branch
         # of a charm is deleted.
-        job = ChangelogIngestJob()
         charm = factory.get_charm_json(branch_deleted=True)
-        with patch.object(job, 'get_changes') as mock:
-            job.run(charm)
+        with patch.object(ingest, 'get_changes') as mock:
+            update_from_revisions(charm)
         self.assertFalse(mock.called)

=== modified file 'charmworld/jobs/tests/test_proof.py'
--- charmworld/jobs/tests/test_proof.py	2013-06-07 12:36:30 +0000
+++ charmworld/jobs/tests/test_proof.py	2013-06-24 19:49:29 +0000
@@ -1,9 +1,14 @@
 # Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-from mock import patch
-
-from charmworld.jobs.ingest import ProofIngestJob
+from logging import getLogger
+
+from mock import MagicMock
+
+from charmworld.jobs.ingest import (
+    get_proofer,
+    update_proof_data,
+)
 from charmworld.testing import factory
 from charmworld.testing import JobTestBase
 
@@ -14,9 +19,8 @@
         # If the charm-proof library cannot be found, an error is logged
         # and the proof loop aborted.
         nonexistant_path = '/does/not/exist/'
-        job = ProofIngestJob()
         handler = self.get_handler('charm.proof')
-        job.setup(proof_path=nonexistant_path)
+        get_proofer(getLogger('charm.proof'), nonexistant_path)
         log_messages = [record.getMessage() for record in handler.buffer]
         err = ("proof error before processing began: "
                "could not find charm proof path.")
@@ -26,9 +30,8 @@
     def test_no_proof_for_deleted_branch(self):
         # If the branch of a charm is deleted from Launchpad, the proof
         # job does nothing.
-        job = ProofIngestJob()
-        job.setup()
+        self.proofer = get_proofer(getLogger('charm.proof'))
         charm = factory.get_charm_json(branch_deleted=True)
-        with patch.object(job, 'proofer') as mock:
-            job.run(charm)
-        self.assertFalse(mock.called)
+        proofer = MagicMock()
+        update_proof_data(charm, getLogger('charm.proof'), proofer)
+        self.assertFalse(proofer.called)

=== modified file 'charmworld/jobs/tests/test_store.py'
--- charmworld/jobs/tests/test_store.py	2013-05-27 12:45:51 +0000
+++ charmworld/jobs/tests/test_store.py	2013-06-24 19:49:29 +0000
@@ -1,17 +1,25 @@
 # Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from logging import getLogger
 from mock import patch
 from requests import Response
 
-from charmworld.jobs.ingest import StoreIngestJob
-from charmworld.jobs.ingest import addresses
+from charmworld.charmstore import CharmStore
+from charmworld.jobs.ingest import (
+    addresses,
+    update_from_store,
+)
 from charmworld.testing import factory
 from charmworld.testing import JobTestBase
 
 
 class TestStoreJob(JobTestBase):
 
+    def setUp(self):
+        super(TestStoreJob, self).setUp()
+        self.log = getLogger('charm.store')
+
     def _mock_data(self, warning=False, error=False):
         data = {'revision': 1}
         if warning:
@@ -20,7 +28,7 @@
             data['errors'] = 'Error forced by mock.'
         return data
 
-    def falsify_session(self, job, content='[[0]]'):
+    def falsify_session(self, store, content='[[0]]'):
 
         class FakeSession:
 
@@ -30,17 +38,16 @@
                 response._content = content
                 return response
 
-        job.store.session = FakeSession
+        store.session = FakeSession
 
     def test_error_handling_error(self):
         handler = self.get_handler('charm.store')
-        job = StoreIngestJob()
-        job.setup()
-        self.falsify_session(job)
+        store = CharmStore()
+        self.falsify_session(store)
         ignore, charm = factory.makeCharm(self.db, promulgated=True)
-        with patch.object(job, '_store_get',
+        with patch.object(store, 'get_charm_info',
                           lambda x: self._mock_data(error=True)):
-            job.run(charm)
+            update_from_store(charm, store, self.log)
 
         first_address = "cs:%s/%s" % (charm["series"], charm["name"])
         second_address = "cs:~%s/%s/%s" % (
@@ -54,13 +61,12 @@
 
     def test_error_handling_warning(self):
         handler = self.get_handler('charm.store')
-        job = StoreIngestJob()
-        job.setup()
-        self.falsify_session(job)
+        store = CharmStore()
+        self.falsify_session(store)
         ignore, charm = factory.makeCharm(self.db, promulgated=True)
-        with patch.object(job, '_store_get',
+        with patch.object(store, 'get_charm_info',
                           lambda x: self._mock_data(warning=True)):
-            job.run(charm)
+            update_from_store(charm, store, self.log)
 
         first_address = "cs:%s/%s" % (charm["series"], charm["name"])
         second_address = "cs:~%s/%s/%s" % (
@@ -73,13 +79,13 @@
         self.assertIn(err_msg, log_messages)
 
     def test_run(self):
-        job = StoreIngestJob()
-        job.setup()
-        self.falsify_session(job, '[["2010-12-24", 4], ["2010-12-25", 1]]')
+        store = CharmStore()
+        self.falsify_session(store, '[["2010-12-24", 4], ["2010-12-25", 1]]')
         ignore, charm = factory.makeCharm(self.db, promulgated=True)
         del charm['store_url']
-        with patch.object(job, '_store_get', lambda x: self._mock_data()):
-            job.run(charm)
+        with patch.object(store, 'get_charm_info',
+                          lambda x: self._mock_data()):
+            update_from_store(charm, store, self.log)
         data = charm['store_data']
         self.assertIn('store_checked', data.keys())
         self.assertEqual(1, data['revision'])