Merge lp:~bac/charmworld/bundles-display into lp:~juju-jitsu/charmworld/trunk

[Brad Crittenden]

Change the basket field in the Bundle to be a basket name and revision number in the form 'basket/revno'.

Change the basket field in the Bundle to be a basket name and revision number in the form 'basket/revno'. Previously there was inconsistency in the way the basket id was created and used.

Remove from the top-level any bundle items that are in the 'data' dictionary.

Change models.py:store_bundles to be easier to test by making the collection and indexer optional.

[Benji York (benji)]

The branch looks good. Here are some semi-bikesheddy comments.

It's kind of strange that construct_basket_id returns two values while
all the other "construct_*_id" functions just return the ID. Perhaps
two functions would be better. (?)

Does "make lint" complain about the new unused variables?

Re. get_bundle_data: so now if you don't specify an owner argument you
get an empty string, but if you pass owner=None you get a random string?
That seems a little odd. Oh, maybe you did that because the change in
behavior broke a bunch of tests.

Another thing, about get_bundle_data: I always thought it was weird to
use the local namespace as a sandbox in which to construct the return
value. Now that you're doing more in the function and having to do the
"del rep[k]" at the end it's getting more strained. You might consider
revamping the function to use **charm_data as the parameter list instead
and then manipulating charm_data instead of the locals.

review: Approve

[Brad Crittenden (bac)]

> The branch looks good. Here are some semi-bikesheddy comments.
>
> It's kind of strange that construct_basket_id returns two values while
> all the other "construct_*_id" functions just return the ID. Perhaps
> two functions would be better. (?)

I renamed to set_basket_info and modify the basket_data dictionary in place.

>
> Does "make lint" complain about the new unused variables?

Nope. Which ones?

>
>
> Re. get_bundle_data: so now if you don't specify an owner argument you
> get an empty string, but if you pass owner=None you get a random string?

No, both cases give you a random string.

> That seems a little odd. Oh, maybe you did that because the change in
> behavior broke a bunch of tests.

No, any broken tests were fixed.

>
> Another thing, about get_bundle_data: I always thought it was weird to
> use the local namespace as a sandbox in which to construct the return
> value. Now that you're doing more in the function and having to do the
> "del rep[k]" at the end it's getting more strained. You might consider
> revamping the function to use **charm_data as the parameter list instead
> and then manipulating charm_data instead of the locals.

I've fixed it to be more explicit.

Thanks for the review and good suggestions.

[Charmworld Lander (charmworld-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2013-08-09 18:53:40 +0000
+++ charmworld/jobs/ingest.py	2013-08-11 23:10:56 +0000
@@ -288,13 +288,14 @@
         self.working_dir = working_dir
         super(UpdateBundleJob, self).__init__()
 
-    def store_bundles(self, deployer_config, basket_id):
-        store_bundles(self.db.bundles, deployer_config, basket_id)
+    def store_bundles(self, deployer_config, owner, basket_id):
+        store_bundles(self.db.bundles, deployer_config, owner, basket_id)
 
     @staticmethod
-    def construct_basket_id(basket_data, revno):
-        owner_segment = '~%s/' % basket_data['owner']
-        return '%s%s-%d' % (owner_segment, basket_data['name'], revno)
+    def set_basket_info(data, revno):
+        owner_segment = '~%s' % data['owner']
+        data['name_revno'] = "%s/%d" % (data['name'], revno)
+        data['_id'] = '%s/%s' % (owner_segment, data['name_revno'])
 
     @staticmethod
     def decorate_bundles(basket_contents, basket_id):
@@ -306,7 +307,7 @@
         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'])
-        basket_data['_id'] = self.construct_basket_id(basket_data, revno)
+        self.set_basket_info(basket_data, revno)
         with read_locked(branch):
             tree = branch.repository.revision_tree(basket_data['commit'])
             basket_data['file_hashes'] = quote_yaml(slurp_files(fs, tree))
@@ -324,7 +325,8 @@
         self.db.baskets.save(basket_data)
         deployer_config = self.get_deployer_config(fs, basket_data)
         self.decorate_bundles(deployer_config, basket_data['_id'])
-        self.store_bundles(deployer_config, basket_data['_id'])
+        self.store_bundles(
+            deployer_config, basket_data['owner'], basket_data['name_revno'])
 
 
 def _rev_info(r, branch):
@@ -333,7 +335,7 @@
         "revno": branch.revision_id_to_revno(r.revision_id),
         "committer": r.committer,
         "created": r.timestamp,
-        "message": r.message
+        "message": r.message,
     }
     return d
 

=== modified file 'charmworld/jobs/tests/test_ingest.py'
--- charmworld/jobs/tests/test_ingest.py	2013-08-09 18:53:40 +0000
+++ charmworld/jobs/tests/test_ingest.py	2013-08-11 23:10:56 +0000
@@ -383,7 +383,7 @@
             with bundle_update_environment(source_branch_dir):
                 job.decorate_basket(bundle_data, None)
 
-        self.assertTrue(bundle_data['_id'].endswith('-8'))
+        self.assertTrue(bundle_data['_id'].endswith('/8'))
 
     def test_decorate_basket_stores_files(self):
         job = UpdateBundleJob()
@@ -423,21 +423,22 @@
                 with self.assertRaises(bzrlib.errors.NoSuchRevision):
                     job.decorate_basket(bundle_data, None)
 
-    def test_construct_basket_id_includes_owner_if_promulgated(self):
+    def test_set_basket_info_includes_owner_if_promulgated(self):
         payload = factory.get_payload_json(name='mysql', promulgated=True)
-        basket_id = UpdateBundleJob.construct_basket_id(payload, 5)
-        self.assertEqual('~charmers/mysql-5', basket_id)
+        UpdateBundleJob.set_basket_info(payload, 5)
+        self.assertEqual('mysql/5', payload['name_revno'])
+        self.assertEqual('~charmers/mysql/5', payload['_id'])
 
-    def test_construct_basket_id_includes_owner_if_not_promulgated(self):
+    def test_set_basket_info_includes_owner_if_not_promulgated(self):
         payload = factory.get_payload_json(name='mysql', promulgated=False)
-        basket_id = UpdateBundleJob.construct_basket_id(payload, 5)
-        self.assertEqual('~charmers/mysql-5', basket_id)
+        UpdateBundleJob.set_basket_info(payload, 5)
+        self.assertEqual('mysql/5', payload['name_revno'])
+        self.assertEqual('~charmers/mysql/5', payload['_id'])
 
     def test_job_can_store_bundles_in_the_db(self):
         basket_data = factory.get_payload_json(name='wordpress')
-        basket_id = UpdateBundleJob.construct_basket_id(basket_data, 5)
-        bundle_id = '%s/%s' % (basket_id, 'stage')
-        basket_data['_id'] = basket_id
+        UpdateBundleJob.set_basket_info(basket_data, 5)
+        bundle_id = '%s/%s' % (basket_data['_id'], 'stage')
         job = UpdateBundleJob()
         job.setup(self.db)
         DEPLOYER_CONFIG_HASH = '31337'
@@ -457,11 +458,14 @@
                             engine: apache
         """)
         fs.put(deployer_config, _id=DEPLOYER_CONFIG_HASH)
-        job.store_bundles(yaml.safe_load(deployer_config), basket_id)
+        job.store_bundles(
+            yaml.safe_load(deployer_config),
+            basket_data['owner'], basket_data['name_revno'])
         self.assertIsNotNone(self.db.bundles.find_one(bundle_id))
 
     def test_job_run_stores_bundles_in_the_db(self):
         basket_data = factory.get_payload_json(name='wordpress')
+        basket_data['name_revno'] = 'dummy'
         job = UpdateBundleJob()
         job.setup(self.db)
         with patch.object(job, 'get_deployer_config'):

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-08-09 18:53:40 +0000
+++ charmworld/models.py	2013-08-11 23:10:56 +0000
@@ -7,14 +7,17 @@
 from bzrlib.branch import Branch
 from bzrlib.errors import NoSuchRevision
 from calendar import timegm
+import copy
 import hashlib
 import logging
 from mimetypes import guess_type
 import os
-from os.path import dirname
-from os.path import join
-from os.path import normpath
-from os.path import split
+from os.path import (
+    dirname,
+    join,
+    normpath,
+    split,
+)
 import random
 import re
 
@@ -1176,11 +1179,9 @@
         'basket': None,
         'name': '',
         'inherits': None,
-        'series': '',
         'title': '',
         'description': '',
-        'services': dict(),
-        'relations': dict(),
+        'data': dict(),
         'promulgated': False,
         'branch_deleted': False,
         'doctype': 'bundle',
@@ -1204,9 +1205,13 @@
         else:
             raise AttributeError
 
+    @staticmethod
+    def construct_id(owner, basket, name):
+        return "~%s/%s/%s" % (owner, basket, name)
+
     @property
     def id(self):
-        return "~{}/{}/{}".format(self.owner, self.basket, self.name)
+        return self.construct_id(self.owner, self.basket, self.name)
 
     @property
     def permanent_url(self):
@@ -1303,14 +1308,45 @@
     return options
 
 
-def store_bundles(collection, deployer_config, basket_id):
-    """Store a basket of bundles into both MongoDB and ElasticSearch."""
-    index_client = ElasticSearchClient.from_settings(settings)
-    index_client.index_bundles(deployer_config.values())
+def store_bundles(collection, deployer_config, owner, basket_id,
+                  index_client=None):
+    """Store a basket of bundles into MongoDB and/or ElasticSearch.
+
+    :param: collection: A db bundles collection.  If None the bundles are not
+        stored to Mongo.
+    :param: deployer_config: A dictionary representing the basket of bundles.
+    :param: owner: A string representing the owner.  No leading ~.
+    :param: basket_id: A slash-joined string of name and revision for the
+        basket.  Does not include the owner name.
+    :param: index_client: The elastic search client.  If None it will be
+        retrieved.  Useful for Mongo-only tests where the indexing is not
+        needed.
+    """
+
+    if index_client is None:
+        index_client = ElasticSearchClient.from_settings(settings)
+
+    # Augment the deployer_config with additional information for indexing.
+    ids = {}
+    config_copy = copy.deepcopy(deployer_config)
+    for key in config_copy:
+        ids[key] = Bundle.construct_id(owner, basket_id, key)
+        config_copy[key]['id'] = ids[key]
+        config_copy[key]['owner'] = owner
+        config_copy[key]['name'] = key
+        config_copy[key]['basket'] = basket_id
+
+    index_client.index_bundles(config_copy.values())
+    if collection is None:
+        return
     for key in deployer_config:
+        data = get_flattened_deployment(deployer_config, key)
         bundle_doc = {
-            '_id': '%s/%s' % (basket_id, key),
-            'data': get_flattened_deployment(deployer_config, key),
+            '_id': ids[key],
+            'name': key,
+            'owner': owner,
+            'basket': basket_id,
+            'data': data,
         }
         collection.save(bundle_doc)
 

=== modified file 'charmworld/search.py'
--- charmworld/search.py	2013-08-09 18:53:40 +0000
+++ charmworld/search.py	2013-08-11 23:10:56 +0000
@@ -32,6 +32,7 @@
     'description': 3,
     'config.options.description': None,
     'relations': None,
+    'services': None,
 }
 
 
@@ -430,9 +431,9 @@
 
         with Timer() as timer:
             status = self.get_status()
-            for real_index in self.get_aliased():
-                break
-            else:
+            try:
+                real_index = self.get_aliased()[0]
+            except IndexError:
                 real_index = self.index_name
             docs = status[real_index].get('docs')
             if docs is None:

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-08-09 18:53:40 +0000
+++ charmworld/testing/factory.py	2013-08-11 23:10:56 +0000
@@ -11,7 +11,6 @@
 )
 import hashlib
 import os
-from os.path import basename
 from random import randint
 import random
 import stat
@@ -311,17 +310,29 @@
     return record_id, charm
 
 
-def get_bundle_data(name=None, owner='', basket=None, inherits=None,
+def get_bundle_data(name=None, owner=None, basket=None, inherits=None,
                     series='precise', title='', description='',
                     services=dict(), relations=dict(), promulgated=False,
                     branch_deleted=False):
     if name is None:
         name = random_string(10)
+    if owner is None:
+        owner = random_string(10)
     if basket is None:
-        basket = random_string(10)
-    doctype = 'bundle'
-    doctype  # Hush lint's dislike of defining variables for the vars() call.
-    return dict(**vars())
+        basket = "%s/%d" % (random_string(10), randint(0, 100))
+    data = dict(series=series,
+                relations=relations,
+                services=services)
+    return dict(basket=basket,
+                branch_deleted=branch_deleted,
+                data=data,
+                description=description,
+                doctype='bundle',
+                inherits=inherits,
+                name=name,
+                owner=owner,
+                promulgated=promulgated,
+                title=title)
 
 
 def makeBundle(db, *args, **kwargs):
@@ -334,7 +345,8 @@
 
 def make_charm_file(db, charm, path, content=None):
     fs = getfs(db)
-    charm_file = CharmFileSet.make_charm_file(fs, charm, path, basename(path))
+    charm_file = CharmFileSet.make_charm_file(
+        fs, charm, path, os.path.basename(path))
     if content is None:
         content = random_string(10).encode('utf-8')
     charm_file.save(content)

=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py	2013-08-09 18:53:40 +0000
+++ charmworld/tests/test_models.py	2013-08-11 23:10:56 +0000
@@ -1458,6 +1458,7 @@
 
     def test_store_bundles(self):
         # The bundles can be stored in the database.
+
         deployer_config = dedent("""\
             wordpress-stage:
                 series: precise
@@ -1470,13 +1471,21 @@
                             engine: apache
         """)
         parsed = yaml.safe_load(deployer_config)
-        store_bundles(self.db.bundles, parsed, 'wordpress-5')
+        owner = 'bac'
+        bundle_name = 'wordpress-stage'
+        basket_id = 'wordpress-basket/5'
+        _id = "~%s/%s/%s" % (owner, basket_id, bundle_name)
+        store_bundles(
+            self.db.bundles, parsed, 'bac', basket_id)
         self.assertEqual(
             {
-                '_id': 'wordpress-5/wordpress-stage',
+                '_id': _id,
+                'basket': basket_id,
+                'name': bundle_name,
+                'owner': owner,
                 'data': parsed['wordpress-stage'],
             },
-            self.db.bundles.find_one('wordpress-5/wordpress-stage'))
+            self.db.bundles.find_one(_id))
 
     def test_store_bundles_calls_get_flattened_deployment(self):
         # For every key in the top-level deployment data,
@@ -1509,7 +1518,7 @@
 
         with patch('charmworld.models.get_flattened_deployment',
                    get_flattened_deployment):
-            store_bundles(self.db.bundles, parsed, 'wordpress-5')
+            store_bundles(self.db.bundles, parsed, 'bac', 'wordpress-basket/5')
         self.assertItemsEqual(['wordpress-stage', 'wordpress-prod'], keys)
 
     def test_storing_a_bundle_includes_indexing_it(self):
@@ -1527,7 +1536,7 @@
         with patch(
                 'charmworld.models.ElasticSearchClient',
                 FauxElasticSearchClient):
-            store_bundles(self.db.bundles, {}, 'wordpress-5')
+            store_bundles(self.db.bundles, {}, 'owner', 'wordpress-basket/5')
 
         self.assertTrue(FauxElasticSearchClient.index_bundles_called)
 
@@ -1547,12 +1556,14 @@
             'basket': 'mysql',
             'name': 'tiny',
             'inherits': 'main-mysql',
-            'series': 'series',
             'title': 'Tiny bundle',
             'description': 'My Tiny Bundle',
             'doctype': 'bundle',
-            'services': dict(apache=1),
-            'relations': dict(a=1),
+            'data': {
+                'services': dict(apache=1),
+                'relations': dict(a=1),
+                'series': 'series',
+            },
             'promulgated': True,
             'branch_deleted': True,
         }

=== modified file 'charmworld/tests/test_search.py'
--- charmworld/tests/test_search.py	2013-08-09 18:53:40 +0000
+++ charmworld/tests/test_search.py	2013-08-11 23:10:56 +0000
@@ -5,12 +5,14 @@
 
 
 from datetime import datetime
+from textwrap import dedent
 
 from pyelasticsearch import ElasticSearch
 from pyelasticsearch.exceptions import (
     ElasticHttpError,
     ElasticHttpNotFoundError,
 )
+import yaml
 
 from charmworld.charmstore import (
     get_address,
@@ -19,6 +21,7 @@
 from charmworld.models import (
     Bundle,
     Charm,
+    store_bundles,
 )
 from charmworld.search import (
     BUNDLE,
@@ -146,6 +149,38 @@
         self.assertEqual(1, len(results))
         self.assertEqual(bundle, results[0]['data'])
 
+    def test_store_bundles(self):
+        # Ensure that bundles stored into the index via the `store_bundles`
+        # function are searchable via the index.
+        _id = '~abentley/wordpress-basket/5/wordpress-stage'
+        deployer_config = dedent("""\
+            wordpress-stage:
+                series: precise
+                services:
+                    blog:
+                        charm: cs:precise/wordpress
+                        constraints: mem=2
+                        options:
+                            tuning: optimized
+                            engine: apache
+        """)
+        parsed = yaml.safe_load(deployer_config)
+        client = self.index_client
+        store_bundles(None, parsed, 'abentley', 'wordpress-basket/5',
+                      index_client=client)
+        # Bundle name is indexed.
+        results = client.search('wordpress-stage')['results']['bundle']
+        self.assertEqual(1, len(results))
+        self.assertEqual(_id, results[0]['data'].id)
+        # Series is indexed.
+        results = client.search('precise')['results']['bundle']
+        self.assertEqual(1, len(results))
+        self.assertEqual(_id, results[0]['data'].id)
+        # Owner is indexed.
+        results = client.search('abentley')['results']['bundle']
+        self.assertEqual(1, len(results))
+        self.assertEqual(_id, results[0]['data'].id)
+
     def test_search_charms_and_bundles_same_name(self):
         charm = Charm(self.makeCharm(name='mozilla'))
         bundle = Bundle(self.makeBundle(name='mozilla'))

=== modified file 'charmworld/views/api.py'
--- charmworld/views/api.py	2013-08-07 13:31:20 +0000
+++ charmworld/views/api.py	2013-08-11 23:10:56 +0000
@@ -351,11 +351,12 @@
         if path is None:
             raise HTTPNotFound(self.request.path)
         fullpath = '/'.join(path)
-        if len(path) == 3:
+        if len(path) == 4:
             # We have an owner.
             query = {'_id': fullpath}
-        elif len(path) == 2:
-            query = {'basket': path[0], 'name': path[1], 'promulgated': True}
+        elif len(path) == 3:
+            basket_id = join(path[0], path[1])
+            query = {'basket': basket_id, 'name': path[2], 'promulgated': True}
         else:
             raise HTTPNotFound(self.request.path)
         bundle_data = self.request.db.bundles.find_one(query)
@@ -363,7 +364,7 @@
             return json_response(
                 404, {'type': 'no_such_bundle', 'bundle_id': fullpath})
         bundle = Bundle(bundle_data)
-        return bundle._representation      # wrong
+        return {bundle.name: bundle.data}
 
     @staticmethod
     def _get_file_headers(md5sum, content_type=None):
@@ -371,7 +372,7 @@
             ("Access-Control-Allow-Origin", "*"),
             ("Access-Control-Allow-Headers", "X-Requested-With"),
             ("Cache-Control", "max-age=86400, public"),
-            ("Etag", '"%s"' % md5sum)
+            ("Etag", '"%s"' % md5sum),
         ]
         if content_type is not None:
             headerlist.append(('Content-Type', content_type))

=== modified file 'charmworld/views/tests/test_api.py'
--- charmworld/views/tests/test_api.py	2013-08-09 15:46:51 +0000
+++ charmworld/views/tests/test_api.py	2013-08-11 23:10:56 +0000
@@ -635,15 +635,16 @@
         bundle = self.makeBundle()
         expected = dict(
             name=bundle.name,
-            owner='',
+            owner=bundle.owner,
             basket=bundle.basket,
             inherits=None,
-            series='precise',
             title='',
+            data=dict(services=dict(),
+                      relations=dict(),
+                      series='precise',
+                      ),
             description='',
             doctype='bundle',
-            services=dict(),
-            relations=dict(),
             promulgated=False,
             branch_deleted=False,
         )
@@ -667,7 +668,7 @@
 
     def test_not_found(self):
         self.makeBundle(name='bat')
-        response = self.get_response('bundle', 'foo/bar')
+        response = self.get_response('bundle', 'foo/4/bar')
         self.assertEqual(404, response.status_code)
 
     def test_no_path(self):
@@ -675,32 +676,28 @@
             self.get_response('bundle', remainder=None)
 
     def test_valid_lookup_unpromulgated(self):
-        self.makeBundle(name='bat', owner='bac', basket='byobu-4')
-        response = self.get_response('bundle', '~bac/byobu-4/bat')
+        self.makeBundle(name='bat', owner='bac', basket='byobu/4')
+        response = self.get_response('bundle', '~bac/byobu/4/bat')
         self.assertEqual(200, response.status_code)
-        response = self.get_response('bundle', 'byobu-4/bat')
+        response = self.get_response('bundle', 'byobu/4/bat')
         self.assertEqual(404, response.status_code)
 
     def test_valid_lookup_promulgated(self):
         self.makeBundle(
-            name='bat', owner='bac', basket='byobu-4',
+            name='bat', owner='bac', basket='byobu/4',
             promulgated=True)
         # Can look up via owner-based id and short id.
-        response = self.get_response('bundle', '~bac/byobu-4/bat')
+        response = self.get_response('bundle', '~bac/byobu/4/bat')
         self.assertEqual(200, response.status_code)
-        response = self.get_response('bundle', 'byobu-4/bat')
+        response = self.get_response('bundle', 'byobu/4/bat')
         self.assertEqual(200, response.status_code)
 
     def test_results_match(self):
-        self.maxDiff = None
-        bundle = self.makeBundle(name='bat', owner='bac', basket='byobu-4')
-        response = self.get_response('bundle', '~bac/byobu-4/bat')
+        bundle = self.makeBundle(name='bat', owner='bac', basket='byobu/4')
+        response = self.get_response('bundle', '~bac/byobu/4/bat')
         self.assertEqual(200, response.status_code)
-        self.assertEqual(bundle.id, response.json['_id'])
-        # Copy the json dict so we can modify it.
         json = dict(response.json)
-        del json['_id']
-        self.assertEqual(json, bundle._representation)
+        self.assertEqual(json['bat'], bundle.data)
 
 
 class TestAPI2Bundles(TestAPIBundles, API2Mixin):