Merge lp:~sinzui/charmworld/stale-metadata into lp:~juju-jitsu/charmworld/trunk

[Curtis Hovey]

Always update the historic charm data with the new metadata and config data.

RULES

    Pre-implementation: no one
    * New metadata like requires and provides are not appearing in
      manage.jujucharms.com. First time ingests show the correct data.
    * The scan_charm function gathers the new metadata (and config data)
      as dict, then wrongly updates the dict with the historic charm data.
      The assumption was that the merger of the two dicts was safe because
      they never share the same keys.
      * This has not been true for several weeks. A change to the ingest
        process always starts with the historic charm data to ensure
        failures during ingestion have some sane data to save.
      * The simple fix is to reverse the update of the dicts, so that the
        new data overwrites the old data.

    ADDENDUM
    * Aaron remarked that process_charm() is mutating the charm_data, but
      we want to instead use the returned dict. We want to work with a
      copy of the data to minimise the mutations to the dict --be safe.
      * Update process_charm() to work with a copy of the charm_data
      * Update callsites to always use the returned charm_data.

QA

    * Wait for ingestion to run on staging.
    * Verify that the mysql charm requires 3 interfaces instead of 1.

[Brad Crittenden (bac)]

This change looks good.

I was initially concerned that the copy in 'process_charm' was not a deep copy given that the charm is a dict of dicts. But looking at the code, no lower-level dicts are mutated so it should be fine.

review: Approve (code)

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2013-07-11 10:53:45 +0000
+++ charmworld/jobs/ingest.py	2013-07-16 20:31:24 +0000
@@ -526,12 +526,13 @@
     hooks.sort()
     metadata["hooks"] = hooks
 
-    # Stuff into the db
-    metadata.update(charm_data)
-    metadata = process_charm(metadata)
+    # Update the historic or incomplete charm_data with the new metadata.
+    # Stuff into the db.
+    charm_data.update(metadata)
+    complete_charm_data = process_charm(charm_data)
     # Modify charm_data itself.
     charm_data.clear()
-    charm_data.update(metadata)
+    charm_data.update(complete_charm_data)
 
 
 def normalize_interfaces(interfaces):
@@ -551,9 +552,9 @@
             interfaces[relation_name] = {'interface': relation_data}
 
 
-def process_charm(charm):
+def process_charm(base_charm):
     # Enrich charm metadata for webapp.
-
+    charm = dict(base_charm)
     # Charm url
     if charm["promulgated"]:
         charm["short_url"] = "/charms/%s/%s" % (

=== modified file 'charmworld/jobs/tests/test_scan.py'
--- charmworld/jobs/tests/test_scan.py	2013-07-11 16:11:40 +0000
+++ charmworld/jobs/tests/test_scan.py	2013-07-16 20:31:24 +0000
@@ -101,6 +101,23 @@
         }
         self.assertEqual(expected, self.charm['provides'])
 
+    def test_short_interface_specification_reingest(self):
+        # Reingestion of changes will update the charm in the db.
+        self.charm['provides'] = {'api': {'interface': 'rest'}}
+        self.charm['requires'] = {'database': {'interface': 'mysql'}}
+        scan_charm(self.charm, self.db, self.fs, self.log)
+        # The sample charm's metadata uses this information.
+        p_expected = {
+            'ssl_website': {'interface': 'https'},
+            'website': {'interface': 'http'},
+        }
+        r_expected = {
+            'database': {'interface': 'mongodb'},
+            'sql_db': {'interface': 'pgsql'},
+        }
+        self.assertEqual(r_expected, self.charm['requires'])
+        self.assertEqual(p_expected, self.charm['provides'])
+
     def test_invalid_metadata_raises_IngestError(self):
         metadata = 'invalid_syntax: ['
         metadata_file = factory.make_charm_file(
@@ -136,10 +153,10 @@
             'name': 'bar',
             'promulgated': False,
         }
-        process_charm(charm)
-        self.assertEqual('/~foo/precise/bar', charm['short_url'])
+        new_charm = process_charm(charm)
+        self.assertEqual('/~foo/precise/bar', new_charm['short_url'])
         # The label of unpromulgated charms starts with the owner name.
-        self.assertEqual('~foo:precise/bar', charm['label'])
+        self.assertEqual('~foo:precise/bar', new_charm['label'])
         # Promulgated charms do not have the owner name in the short URL
         # and in the label.
         charm = {
@@ -148,9 +165,9 @@
             'name': 'bar',
             'promulgated': True,
         }
-        process_charm(charm)
-        self.assertEqual('/charms/precise/bar', charm['short_url'])
-        self.assertEqual('precise/bar', charm['label'])
+        new_charm = process_charm(charm)
+        self.assertEqual('/charms/precise/bar', new_charm['short_url'])
+        self.assertEqual('precise/bar', new_charm['label'])
         # This is also true if the charm is not owned by charmes.
         charm = {
             'owner': 'foo',
@@ -158,6 +175,6 @@
             'name': 'bar',
             'promulgated': True,
         }
-        process_charm(charm)
-        self.assertEqual('/charms/precise/bar', charm['short_url'])
-        self.assertEqual('precise/bar', charm['label'])
+        new_charm = process_charm(charm)
+        self.assertEqual('/charms/precise/bar', new_charm['short_url'])
+        self.assertEqual('precise/bar', new_charm['label'])

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-07-12 14:53:18 +0000
+++ charmworld/testing/factory.py	2013-07-16 20:31:24 +0000
@@ -287,8 +287,7 @@
                 charm['name'], charm['branch_spec'], status, provider)
     if subordinate:
         charm['subordinate'] = True
-    process_charm(charm)
-    return charm
+    return process_charm(charm)
 
 
 def makeCharm(db, *args, **kwargs):