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

[Aaron Bentley]

Remove ScanIngestJob

This branch removes ScanIngestJob, as part of the fix for 1190015: "ingest is really a set of functions that are run consecutively, but is structured as a list of jobs".

[Curtis Hovey (sinzui)]

Thank you.

This is fine to land. The diff reminds me of the cruft I was concerned about in the DB. Now that we have a Charm model, I don't think we need short_url, i_provides, i_requires (and their d_ counterparts). I fixed a UI bug recently by changing the model to create the short_url if one is not in the db. I will report a bug about this so that we can discuss which data is not needed in the db.

review: Approve (code)

[Aaron Bentley (abentley)]

I believe the i_requires and i_provides fields were meant to make these easy to query for a search engine ("i" being short for "index"). We should probably wait until we've restructured the representation of "requires" and "provides" before we consider deleting the i_ variants.

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2013-06-21 19:38:25 +0000
+++ charmworld/jobs/ingest.py	2013-06-24 14:42:35 +0000
@@ -244,8 +244,7 @@
         if not run_job(ChangelogIngestJob(), charm_data):
             return False
         update_date_created(charm_data, log)
-        if not run_job(ScanIngestJob(), charm_data, db=db):
-            return False
+        scan_charm(charm_data, db=db)
     except Exception as e:
         err_msg = "%s error: %s" % (charm_data, str(e))
         log.exception(err_msg)
@@ -509,140 +508,141 @@
         charm['proof'] = proof
 
 
-class ScanIngestJob(FSIngestJob):
-
-    name = 'scan'
-
-    @staticmethod
-    def normalize_interfaces(interfaces):
-        # Transform the short defintion of relations into the
-        # long form. Quoted from
-        # https://juju.ubuntu.com/docs/charm.html#the-metadata-file :
-        #
-        #     As a shortcut, if these properties are not defined, and
-        #     instead a single string value is provided next to the
-        #     relation name, the string is taken as the interface
-        #     value, as seen in this example:
-        #
-        #         requires:
-        #             db: mysql
-        for relation_name, relation_data in interfaces.items():
-            if isinstance(relation_data, StringTypes):
-                interfaces[relation_name] = {'interface': relation_data}
-
-    @classmethod
-    def process_charm(cls, charm):
-        # Enrich charm metadata for webapp.
-
-        # Charm url
-        if charm["promulgated"]:
-            charm["short_url"] = "/charms/%s/%s" % (
-                charm["series"], charm["name"])
-        else:
-            charm["short_url"] = "/~%s/%s/%s" % (charm["owner"],
-                                                 charm["series"],
-                                                 charm["name"])
-
-        # Charm label
-        if charm["promulgated"]:
-            charm["label"] = "%s/%s" % (charm["series"], charm["name"])
-        else:
-            charm["label"] = "~%s:%s/%s" % (charm["owner"],
-                                            charm["series"],
-                                            charm["name"])
-
-        # Flatten the interfaces provided
-        i_provides = []
-        provides = charm.get("provides")
-        if provides:
-            cls.normalize_interfaces(provides)
-            for v in provides.values():
-                if not isinstance(v, dict):
-                    continue
-                i = v.get("interface")
-                if not i:
-                    continue
-                i_provides.append(i)
-        charm["i_provides"] = i_provides
-
-        # Flatten the interfaces required
-        i_requires = []
-        requires = charm.get("requires")
-        if requires:
-            cls.normalize_interfaces(requires)
-            for v in requires.values():
-                i = v.get("interface")
-                if not i:
-                    continue
-                i_requires.append(i)
-        charm["i_requires"] = i_requires
-        return charm
-
-    def run(self, charm_data):
-        # Note: charm_data is modified in-place.  IndexIngestJob requires
-        # these modifications.
-        files = charm_data['files']
-        # Some files have bad characters in them since they are used as mongo
-        # keys. Use their escaped forms instead.
-        metadata_file = quote_key('metadata.yaml')
-        config_file = quote_key('config.yaml')
-
-        if metadata_file not in files:
-            self.log.info(
-                "Charm has no metadata: %s", charm_data["branch_spec"])
-            return
-        else:
-            cfile = CharmFileSet.get_by_id(
-                self.fs, files[metadata_file]['fileid'])
-            try:
-                metadata = quote_yaml(yaml.load(cfile.read()))
-            except Exception, exc:
-                raise IngestError(
-                    'Invalid charm metadata %s: %s' % (
+def scan_charm(charm_data, db=None, fs=None):
+    # Note: charm_data is modified in-place.  IndexIngestJob requires
+    # these modifications.
+    job = FSIngestJob()
+    job.setup(db, fs)
+    db = job.db
+    fs = job.fs
+    log = job.log
+    files = charm_data['files']
+    # Some files have bad characters in them since they are used as mongo
+    # keys. Use their escaped forms instead.
+    metadata_file = quote_key('metadata.yaml')
+    config_file = quote_key('config.yaml')
+
+    if metadata_file not in files:
+        log.info(
+            "Charm has no metadata: %s", charm_data["branch_spec"])
+        return
+    else:
+        cfile = CharmFileSet.get_by_id(
+            fs, files[metadata_file]['fileid'])
+        try:
+            metadata = quote_yaml(yaml.load(cfile.read()))
+        except Exception, exc:
+            raise IngestError(
+                'Invalid charm metadata %s: %s' % (
+                charm_data['branch_spec'],
+                exc)
+            )
+
+    if config_file in files:
+        cfile = CharmFileSet.get_by_id(
+            fs, files[config_file]['fileid'])
+        config_raw = cfile.read()
+
+        try:
+            config = quote_yaml(yaml.load(config_raw))
+        except Exception, exc:
+            raise IngestError(
+                'Invalid charm config yaml. %s: %s' % (
                     charm_data['branch_spec'],
                     exc)
-                )
-
-        if config_file in files:
-            cfile = CharmFileSet.get_by_id(
-                self.fs, files[config_file]['fileid'])
-            config_raw = cfile.read()
-
-            try:
-                config = quote_yaml(yaml.load(config_raw))
-            except Exception, exc:
-                raise IngestError(
-                    'Invalid charm config yaml. %s: %s' % (
-                        charm_data['branch_spec'],
-                        exc)
-                )
-
-            metadata["config"] = config
-            metadata["config_raw"] = config_raw
-
-        if 'revision' in files:
-            cfile = CharmFileSet.get_by_id(
-                self.fs, files['revision']['fileid'])
-            rev_raw = cfile.read()
-            rev_id = int(rev_raw.strip())
-            metadata["revision"] = rev_id
-        elif not "revision" in metadata:
-            self.log.info("Invalid revision %s", charm_data["branch_spec"])
-            metadata["revision"] = 0
-
-        hooks = []
-        for filedata in files.values():
-            if filedata['subdir'] == 'hooks':
-                hooks.append(filedata['filename'])
-        hooks.sort()
-        metadata["hooks"] = hooks
-
-        # Stuff into the db
-        metadata.update(charm_data)
-        metadata = self.process_charm(metadata)
-        # Modify charm_data itself.
-        charm_data.clear()
-        charm_data.update(metadata)
+            )
+
+        metadata["config"] = config
+        metadata["config_raw"] = config_raw
+
+    if 'revision' in files:
+        cfile = CharmFileSet.get_by_id(
+            fs, files['revision']['fileid'])
+        rev_raw = cfile.read()
+        rev_id = int(rev_raw.strip())
+        metadata["revision"] = rev_id
+    elif not "revision" in metadata:
+        log.info("Invalid revision %s", charm_data["branch_spec"])
+        metadata["revision"] = 0
+
+    hooks = []
+    for filedata in files.values():
+        if filedata['subdir'] == 'hooks':
+            hooks.append(filedata['filename'])
+    hooks.sort()
+    metadata["hooks"] = hooks
+
+    # Stuff into the db
+    metadata.update(charm_data)
+    metadata = process_charm(metadata)
+    # Modify charm_data itself.
+    charm_data.clear()
+    charm_data.update(metadata)
+
+
+def normalize_interfaces(interfaces):
+    # Transform the short defintion of relations into the
+    # long form. Quoted from
+    # https://juju.ubuntu.com/docs/charm.html#the-metadata-file :
+    #
+    #     As a shortcut, if these properties are not defined, and
+    #     instead a single string value is provided next to the
+    #     relation name, the string is taken as the interface
+    #     value, as seen in this example:
+    #
+    #         requires:
+    #             db: mysql
+    for relation_name, relation_data in interfaces.items():
+        if isinstance(relation_data, StringTypes):
+            interfaces[relation_name] = {'interface': relation_data}
+
+
+def process_charm(charm):
+    # Enrich charm metadata for webapp.
+
+    # Charm url
+    if charm["promulgated"]:
+        charm["short_url"] = "/charms/%s/%s" % (
+            charm["series"], charm["name"])
+    else:
+        charm["short_url"] = "/~%s/%s/%s" % (charm["owner"],
+                                             charm["series"],
+                                             charm["name"])
+
+    # Charm label
+    if charm["promulgated"]:
+        charm["label"] = "%s/%s" % (charm["series"], charm["name"])
+    else:
+        charm["label"] = "~%s:%s/%s" % (charm["owner"],
+                                        charm["series"],
+                                        charm["name"])
+
+    # Flatten the interfaces provided
+    i_provides = []
+    provides = charm.get("provides")
+    if provides:
+        normalize_interfaces(provides)
+        for v in provides.values():
+            if not isinstance(v, dict):
+                continue
+            i = v.get("interface")
+            if not i:
+                continue
+            i_provides.append(i)
+    charm["i_provides"] = i_provides
+
+    # Flatten the interfaces required
+    i_requires = []
+    requires = charm.get("requires")
+    if requires:
+        normalize_interfaces(requires)
+        for v in requires.values():
+            i = v.get("interface")
+            if not i:
+                continue
+            i_requires.append(i)
+    charm["i_requires"] = i_requires
+    return charm
 
 
 def addresses(charm):

=== modified file 'charmworld/jobs/tests/test_scan.py'
--- charmworld/jobs/tests/test_scan.py	2013-06-21 19:53:57 +0000
+++ charmworld/jobs/tests/test_scan.py	2013-06-24 14:42:35 +0000
@@ -2,8 +2,10 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from charmworld.jobs.ingest import (
+    FSIngestJob,
     IngestError,
-    ScanIngestJob,
+    process_charm,
+    scan_charm,
     store_branch_files,
 )
 from charmworld.testing import factory
@@ -11,21 +13,21 @@
 from charmworld.utils import quote_key
 
 
-class TestScanJob(JobTestBase):
+class TestScanCharm(JobTestBase):
 
     def setUp(self):
-        super(TestScanJob, self).setUp()
-        self.scan_job = ScanIngestJob()
-        self.scan_job.setup(db=self.db)
+        super(TestScanCharm, self).setUp()
+        self.fs_job = FSIngestJob()
+        self.fs_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
-            store_branch_files(self.scan_job.fs, self.charm, self.scan_job.log)
+            store_branch_files(self.fs_job.fs, self.charm, self.fs_job.log)
         ])
 
     def test_config_metadata_with_dotted_keys(self):
         del self.charm['config']
-        self.scan_job.run(self.charm)
+        scan_charm(self.charm, self.db, self.fs_job.fs)
         # The key 'dotted.name' is replaced with 'dotted%2Ename', and
         # 'foo.bar' is replaced with 'foo%2Ebar'.
         self.assertFalse('dotted.name' in self.charm)
@@ -42,11 +44,11 @@
         # https://juju.ubuntu.com/docs/charm.html#the-metadata-file
         # for more details.)
         #
-        # ScanJob.normalize_interfaces() transforms the short form
+        # normalize_interfaces() transforms the short form
         # into the long form for further processing and for storage.
         del self.charm['provides']
         del self.charm['requires']
-        self.scan_job.run(self.charm)
+        scan_charm(self.charm, self.db, self.fs_job.fs)
         # The sample metadata file has entries for the short and long
         # form of the interface specification in the "provides" and
         # in the "requires" section. The short entries are converted
@@ -70,7 +72,7 @@
             'fileid': metadata_file._id
         }
         with self.assertRaises(IngestError) as ctx:
-            self.scan_job.run(self.charm)
+            scan_charm(self.charm, self.db, self.fs_job.fs)
         info = str(ctx.exception)
         self.assertTrue(info.startswith(
             'Invalid charm metadata ~charmers/charms/precise/sample'))
@@ -83,7 +85,7 @@
             'fileid': config_file._id
         }
         with self.assertRaises(IngestError) as ctx:
-            self.scan_job.run(self.charm)
+            scan_charm(self.charm, self.db, self.fs_job.fs)
         info = str(ctx.exception)
         self.assertTrue(info.startswith(
             'Invalid charm config yaml. ~charmers/charms/precise/sample'))
@@ -97,7 +99,7 @@
             'name': 'bar',
             'promulgated': False,
         }
-        self.scan_job.process_charm(charm)
+        process_charm(charm)
         self.assertEqual('/~foo/precise/bar', charm['short_url'])
         # The label of unpromulgated charms starts with the owner name.
         self.assertEqual('~foo:precise/bar', charm['label'])
@@ -109,7 +111,7 @@
             'name': 'bar',
             'promulgated': True,
         }
-        self.scan_job.process_charm(charm)
+        process_charm(charm)
         self.assertEqual('/charms/precise/bar', charm['short_url'])
         self.assertEqual('precise/bar', charm['label'])
         # This is also true if the charm is not owned by charmes.
@@ -119,6 +121,6 @@
             'name': 'bar',
             'promulgated': True,
         }
-        self.scan_job.process_charm(charm)
+        process_charm(charm)
         self.assertEqual('/charms/precise/bar', charm['short_url'])
         self.assertEqual('precise/bar', charm['label'])

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-06-17 15:02:59 +0000
+++ charmworld/testing/factory.py	2013-06-24 14:42:35 +0000
@@ -21,7 +21,7 @@
 )
 from charmworld.jobs.ingest import (
     JENKINS_PROVIDERS,
-    ScanIngestJob,
+    process_charm,
 )
 from charmworld.models import (
     getfs,
@@ -279,7 +279,7 @@
                 charm['name'], charm['branch_spec'], status, provider)
     if subordinate:
         charm['subordinate'] = True
-    ScanIngestJob.process_charm(charm)
+    process_charm(charm)
     return charm