Merge lp:~abentley/charmworld/polish-ingest-bundle into lp:~juju-jitsu/charmworld/trunk

[Aaron Bentley]

Stop ingest from crashing with bundles.

This branch fixes two obvious problems with ingesting bundles:
1. When retrieving files from a branch, the branch is not read-locked first.
2. When decorating bundles, the basket _id is needed, but not supplied.

2. was more confused because the variable was called basket_data, but it was the basket contents, not its data (which does have an _id).

With these changes, local ingests work.

[Benji York (benji)]

This looks good. I think I introduced the non-lock bug you fixed, so I'm glad to see it fixed. Having tests is wonderful too. Thanks.

review: Approve (code)

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2013-08-01 15:12:08 +0000
+++ charmworld/jobs/ingest.py	2013-08-02 20:15:57 +0000
@@ -49,8 +49,9 @@
 from charmworld.utils import (
     quote_key,
     quote_yaml,
+    read_locked,
+    timestamp,
     unquote_yaml,
-    timestamp,
 )
 
 # XXX Why not "from charmworld.jobs.config import..."? (Benji)
@@ -285,18 +286,19 @@
         return '%s%s-%d' % (owner_segment, basket_data['name'], revno)
 
     @staticmethod
-    def decorate_bundles(basket_data):
+    def decorate_bundles(basket_contents, basket_id):
         # Give each bundle a unique ID.
-        for bundle_name, bundle in basket_data.items():
-            bundle['_id'] = '%s/%s' % (basket_data['_id'], bundle_name)
+        for bundle_name, bundle in basket_contents.items():
+            bundle['_id'] = '%s/%s' % (basket_id, bundle_name)
 
     def decorate_basket(self, basket_data, fs):
         branch_dir = fetch_branch(self.working_dir, basket_data, self.log)
         branch = Branch.open(branch_dir)
         revno = branch.revision_id_to_revno(basket_data['commit'])
-        tree = branch.repository.revision_tree(basket_data['commit'])
         basket_data['_id'] = self.construct_basket_id(basket_data, revno)
-        basket_data['file_hashes'] = quote_yaml(slurp_files(fs, tree))
+        with read_locked(branch):
+            tree = branch.repository.revision_tree(basket_data['commit'])
+            basket_data['file_hashes'] = quote_yaml(slurp_files(fs, tree))
 
     @staticmethod
     def get_deployer_config(fs, basket_data):
@@ -310,7 +312,7 @@
         self.decorate_basket(basket_data, fs)
         self.db.baskets.save(basket_data)
         deployer_config = self.get_deployer_config(fs, basket_data)
-        self.decorate_bundles(deployer_config)
+        self.decorate_bundles(deployer_config, basket_data['_id'])
         self.store_bundles(deployer_config, basket_data['_id'])
 
 

=== modified file 'charmworld/jobs/tests/test_ingest.py'
--- charmworld/jobs/tests/test_ingest.py	2013-07-31 14:41:29 +0000
+++ charmworld/jobs/tests/test_ingest.py	2013-08-02 20:15:57 +0000
@@ -385,6 +385,27 @@
 
         self.assertTrue(bundle_data['_id'].endswith('-8'))
 
+    def test_decorate_basket_stores_files(self):
+        job = UpdateBundleJob()
+        fs = getfs(self.db, collection='hashed-files')
+        bundle_data = factory.get_payload_json()
+        with bzr_isolation() as source_branch_dir:
+            wt = BzrDir.create_standalone_workingtree(source_branch_dir)
+            wt.bzrdir.root_transport.put_bytes(
+                'bundles.yaml', '{foo: {}}')
+            wt.add('bundles.yaml')
+            bundle_data['commit'] = wt.commit('commit message')
+            with bundle_update_environment(source_branch_dir):
+                job.decorate_basket(bundle_data, fs)
+        bundles_hash = bundle_data['file_hashes'].get('bundles%2Eyaml')
+        self.assertIsNot(None, bundles_hash)
+        self.assertEqual('{foo: {}}', fs.get(bundles_hash).read())
+
+    def test_decorate_bundles(self):
+        basket_contents = {'foo': {}}
+        UpdateBundleJob.decorate_bundles(basket_contents, '~bar/baz')
+        self.assertEqual({'foo': {'_id': '~bar/baz/foo'}}, basket_contents)
+
     def test_missing_revisions_generate_errors(self):
         job = UpdateBundleJob()
         job.setup(self.db)