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

Proposed by Curtis Hovey
Status: Merged
Approved by: Brad Crittenden
Approved revision: 313
Merged at revision: 310
Proposed branch: lp:~sinzui/charmworld/stale-metadata
Merge into: lp:~juju-jitsu/charmworld/trunk
Diff against target: 112 lines (+34/-17)
3 files modified
charmworld/jobs/ingest.py (+7/-6)
charmworld/jobs/tests/test_scan.py (+26/-9)
charmworld/testing/factory.py (+1/-2)
To merge this branch: bzr merge lp:~sinzui/charmworld/stale-metadata
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Review via email: mp+175126@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
526 hooks.sort()526 hooks.sort()
527 metadata["hooks"] = hooks527 metadata["hooks"] = hooks
528528
529 # Stuff into the db529 # Update the historic or incomplete charm_data with the new metadata.
530 metadata.update(charm_data)530 # Stuff into the db.
531 metadata = process_charm(metadata)531 charm_data.update(metadata)
532 complete_charm_data = process_charm(charm_data)
532 # Modify charm_data itself.533 # Modify charm_data itself.
533 charm_data.clear()534 charm_data.clear()
534 charm_data.update(metadata)535 charm_data.update(complete_charm_data)
535536
536537
537def normalize_interfaces(interfaces):538def normalize_interfaces(interfaces):
@@ -551,9 +552,9 @@
551 interfaces[relation_name] = {'interface': relation_data}552 interfaces[relation_name] = {'interface': relation_data}
552553
553554
554def process_charm(charm):555def process_charm(base_charm):
555 # Enrich charm metadata for webapp.556 # Enrich charm metadata for webapp.
556557 charm = dict(base_charm)
557 # Charm url558 # Charm url
558 if charm["promulgated"]:559 if charm["promulgated"]:
559 charm["short_url"] = "/charms/%s/%s" % (560 charm["short_url"] = "/charms/%s/%s" % (
560561
=== 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 @@
101 }101 }
102 self.assertEqual(expected, self.charm['provides'])102 self.assertEqual(expected, self.charm['provides'])
103103
104 def test_short_interface_specification_reingest(self):
105 # Reingestion of changes will update the charm in the db.
106 self.charm['provides'] = {'api': {'interface': 'rest'}}
107 self.charm['requires'] = {'database': {'interface': 'mysql'}}
108 scan_charm(self.charm, self.db, self.fs, self.log)
109 # The sample charm's metadata uses this information.
110 p_expected = {
111 'ssl_website': {'interface': 'https'},
112 'website': {'interface': 'http'},
113 }
114 r_expected = {
115 'database': {'interface': 'mongodb'},
116 'sql_db': {'interface': 'pgsql'},
117 }
118 self.assertEqual(r_expected, self.charm['requires'])
119 self.assertEqual(p_expected, self.charm['provides'])
120
104 def test_invalid_metadata_raises_IngestError(self):121 def test_invalid_metadata_raises_IngestError(self):
105 metadata = 'invalid_syntax: ['122 metadata = 'invalid_syntax: ['
106 metadata_file = factory.make_charm_file(123 metadata_file = factory.make_charm_file(
@@ -136,10 +153,10 @@
136 'name': 'bar',153 'name': 'bar',
137 'promulgated': False,154 'promulgated': False,
138 }155 }
139 process_charm(charm)156 new_charm = process_charm(charm)
140 self.assertEqual('/~foo/precise/bar', charm['short_url'])157 self.assertEqual('/~foo/precise/bar', new_charm['short_url'])
141 # The label of unpromulgated charms starts with the owner name.158 # The label of unpromulgated charms starts with the owner name.
142 self.assertEqual('~foo:precise/bar', charm['label'])159 self.assertEqual('~foo:precise/bar', new_charm['label'])
143 # Promulgated charms do not have the owner name in the short URL160 # Promulgated charms do not have the owner name in the short URL
144 # and in the label.161 # and in the label.
145 charm = {162 charm = {
@@ -148,9 +165,9 @@
148 'name': 'bar',165 'name': 'bar',
149 'promulgated': True,166 'promulgated': True,
150 }167 }
151 process_charm(charm)168 new_charm = process_charm(charm)
152 self.assertEqual('/charms/precise/bar', charm['short_url'])169 self.assertEqual('/charms/precise/bar', new_charm['short_url'])
153 self.assertEqual('precise/bar', charm['label'])170 self.assertEqual('precise/bar', new_charm['label'])
154 # This is also true if the charm is not owned by charmes.171 # This is also true if the charm is not owned by charmes.
155 charm = {172 charm = {
156 'owner': 'foo',173 'owner': 'foo',
@@ -158,6 +175,6 @@
158 'name': 'bar',175 'name': 'bar',
159 'promulgated': True,176 'promulgated': True,
160 }177 }
161 process_charm(charm)178 new_charm = process_charm(charm)
162 self.assertEqual('/charms/precise/bar', charm['short_url'])179 self.assertEqual('/charms/precise/bar', new_charm['short_url'])
163 self.assertEqual('precise/bar', charm['label'])180 self.assertEqual('precise/bar', new_charm['label'])
164181
=== 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 @@
287 charm['name'], charm['branch_spec'], status, provider)287 charm['name'], charm['branch_spec'], status, provider)
288 if subordinate:288 if subordinate:
289 charm['subordinate'] = True289 charm['subordinate'] = True
290 process_charm(charm)290 return process_charm(charm)
291 return charm
292291
293292
294def makeCharm(db, *args, **kwargs):293def makeCharm(db, *args, **kwargs):

Subscribers

People subscribed via source and target branches