Merge lp:~rharding/charmworld/bundle-metadata2 into lp:~juju-jitsu/charmworld/trunk

[Richard Harding]

Add extra file/charm_metadata support to the bundle details api call.

Add extra file/charm_metadata support to the bundle details api call.

The bundle details view needs to provide metadata for the Gui to properly render the token and information about it. This provides the list of files (to find the README for instance) in the bundle as well as charm details of the charms in the bundle.

The charms are keyed off their keys in the bundle services list so that the gui can match from the name used in the bundle to the charm model in the charm_metadata.

The charms are formatted per the normal charm_details method by casting to a Charm object and then running through _format_charm.

QA:

http://charmworld:2464/api/3/bundle/~bac/wiki/wiki

Should output a new large suite of json for each charm in the bundle. It should also show the list of files so that the front end can implement a hasIcon as per the way gui handles charms. It also allows searching for the README file (README, readme.md, etc) just as we do with the charms in the Gui.

[Benji York (benji)]

The branch looks good. I saw a few little things that deserve your
attention.

Lines 44 and 85 of the diff: The name "data" is somewhat
information-free, maybe bundle_data would be more informative.

Line 45 of the diff: The docstring summery might be better rendered as
"Construct a query that specifies a charm from data about that charm."

Line 47 of the diff: The bulleted lists in the docstring have
inconsistent indentation.

Lines 64 and 77 of the diff: Please capitalize the sentence in the
comment.

Line 71 of the diff: I would be paranoid and pass "/trunk" to the
.endswith() call so a branch name like "who-is-that-in-the-trunk" won't
be matched.

Line 196 of the diff: It would be slightly better to use .assertIsNone().

Line 357 of the diff: There is an extra space after the colon. I think
lint will pick that up, so you should run lint to see if there is more.

Line 205 of the diff: The TestBundleLoadingCharms test case could use a
test that shows what a request with too-little information to find a
charm will produce.

review: Approve

[Richard Harding (rharding)]

> Lines 44 and 85 of the diff: The name "data" is somewhat
> information-free, maybe bundle_data would be more informative.

Updated. It's actually the services bit from the bundle so went with bundle_charm_info.

> Line 45 of the diff: The docstring summery might be better rendered as
> "Construct a query that specifies a charm from data about that charm."

Updated

> Line 47 of the diff: The bulleted lists in the docstring have
> inconsistent indentation.

Same

> Lines 64 and 77 of the diff: Please capitalize the sentence in the
> comment.

Thanks

> Line 71 of the diff: I would be paranoid and pass "/trunk" to the
> .endswith() call so a branch name like "who-is-that-in-the-trunk" won't
> be matched.

Makes sense.

> Line 196 of the diff: It would be slightly better to use .assertIsNone().

Updated

> Line 357 of the diff: There is an extra space after the colon. I think
> lint will pick that up, so you should run lint to see if there is more.

Thanks, also had the extra line setting the maxdiff to none that lint caught and removed.

> Line 205 of the diff: The TestBundleLoadingCharms test case could use a
> test that shows what a request with too-little information to find a
> charm will produce.

Thanks! This misbehaved and the test caught that and got it adjusted.

[Juju Gui Bot (juju-gui-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
http://162.213.35.27:8080/job/charmworld-autoland-lxc/15/
Executed test runs:

review: Needs Fixing (continuous-integration)

[Richard Harding (rharding)]

Updated and sync'd with trunk. Will try to land again.

[Juju Gui Bot (juju-gui-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
http://162.213.35.27:8080/job/charmworld-autoland-lxc/16/
Executed test runs:

review: Needs Fixing (continuous-integration)

[Juju Gui Bot (juju-gui-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
http://162.213.35.27:8080/job/charmworld-autoland-lxc/17/
Executed test runs:

review: Needs Fixing (continuous-integration)

[Juju Gui Bot (juju-gui-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
http://162.213.35.27:8080/job/charmworld-autoland-lxc/18/
Executed test runs:

review: Needs Fixing (continuous-integration)

[Juju Gui Bot (juju-gui-bot)]

review: Approve (continuous-integration)

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-10-01 00:38:01 +0000
+++ charmworld/models.py	2013-10-07 14:35:40 +0000
@@ -1288,7 +1288,7 @@
 class Bundle:
     """A model to represent bundles of charms."""
 
-    _COMMON_REPRESENTATION = {
+    _MONGO_FIELDS = {
         'owner': '',
         'basket_name': None,
         'basket_revision': None,
@@ -1305,7 +1305,7 @@
             'inheriting from other bundles is not supported, the bundle '
             'must be preprocessed to incorporate any inheritance')
         self._raw_representation = data
-        self._representation = dict(self._COMMON_REPRESENTATION)
+        self._representation = dict(self._MONGO_FIELDS)
         self._representation.update(data)
 
     @classmethod
@@ -1333,7 +1333,7 @@
                 and self.permanent_url == other.permanent_url)
 
     def __getattr__(self, name):
-        if name in self._COMMON_REPRESENTATION:
+        if name in self._MONGO_FIELDS:
             return self._representation[name]
         else:
             raise AttributeError
@@ -1406,7 +1406,7 @@
         """
         yield 'id', self.id
         yield 'permanent_url', self.permanent_url
-        for key in self._COMMON_REPRESENTATION:
+        for key in self._MONGO_FIELDS:
             yield key, self.__getattr__(key)
 
     @property
@@ -1487,6 +1487,69 @@
         sort, max_scan, as_class, all_revisions, **kwargs))
 
 
+def _build_charm_query_from_bundle_info(bundle_charm_info, series):
+    """Construct a query for a charm based on the info available in the bundle
+
+    Bundles may provide:
+        - branch
+        - charm (name)
+            - If so, the series is required to determine which charm with that
+              name. This also assumes a promulgated charm.
+        - charm (store url)
+    """
+    charm_query = None
+    branch, rev = None, None
+    charm_branch = bundle_charm_info.get('branch')
+    store_url = bundle_charm_info.get(
+        'charm',
+        bundle_charm_info.get('charm_url', None))
+
+    if (store_url and store_url.startswith('cs:')):
+        charm_query = {'store_url': store_url}
+    elif charm_branch:
+        branch, sep, revision = charm_branch.partition('@')
+
+        # Strip the lp:charms to match the fields in the db.
+        if branch.startswith('lp:'):
+            branch = re.sub('^lp:', '', branch)
+        # If the branch url doesn't start with an owner assume it's a
+        # ~charmers branch
+        if branch[0] != '~':
+            branch = '~charmers/' + branch
+        if not branch.endswith('/trunk'):
+            branch = branch + '/trunk'
+        charm_query = {u'branch_spec': str(branch)}
+        if (revision):
+            charm_query['store_data.revision'] = int(revision)
+    elif store_url and series:
+        # The last resort, if there's a series in the bundle config, and we
+        # have a charm name, we can attempt to load the latest version of the
+        # charm with that address.
+        address = 'cs:' + '/'.join([series, store_url])
+        charm_query = {u'address': address}
+    return charm_query
+
+
+def resolve_charm_from_bundle_info(db, bundle_charm_info, series):
+    """Find the charm that the bundle is looking for.
+
+    Bundles may provide charm info in several ways. Attempt to find the best
+    charm we can based on that information.
+
+    """
+    charm_query = _build_charm_query_from_bundle_info(
+        bundle_charm_info, series)
+
+    if charm_query:
+        # Order all results by revision so that we grab the latest match
+        # as we might not always have a revision to work with.
+        sort = [('store_data.revision', pymongo.DESCENDING)]
+        results = db.charms.find(charm_query, sort=sort).limit(1)
+        if results.count() > 0:
+            return results[0]
+    return None
+
+
 def _find_charms(collection, spec=None, fields=None, skip=0, limit=0,
                  timeout=True, snapshot=False, tailable=False, sort=None,
                  max_scan=None, as_class=None, all_revisions=False, **kwargs):

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-10-03 17:13:56 +0000
+++ charmworld/testing/factory.py	2013-10-07 14:35:40 +0000
@@ -256,7 +256,7 @@
         charm = dict(payload)
     add_store_data(charm, promulgated, charm_error, store_revision)
     charm.update({
-        '_id': construct_charm_id(charm),
+        '_id': construct_charm_id(charm, revision),
         'address': get_address(charm, promulgated),
         "branch_dir": os.path.join(branch_root, charm['name']),
         "categories": categories,

=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py	2013-09-10 14:45:25 +0000
+++ charmworld/tests/test_models.py	2013-10-07 14:35:40 +0000
@@ -27,6 +27,7 @@
 
 from charmworld.models import (
     acquire_session_secret,
+    _build_charm_query_from_bundle_info,
     Bundle,
     Charm,
     CharmFile,
@@ -40,6 +41,7 @@
     options_to_storage,
     QAData,
     QADataSource,
+    resolve_charm_from_bundle_info,
     slurp_files,
     storage_to_options,
     store_bundles,
@@ -1644,7 +1646,7 @@
         bundle_data = {}
         bundle = Bundle(bundle_data)
         self.assertIs(bundle_data, bundle._raw_representation)
-        self.assertEqual(bundle._COMMON_REPRESENTATION, bundle._representation)
+        self.assertEqual(bundle._MONGO_FIELDS, bundle._representation)
 
     def test_init_with_minimal_data(self):
         bundle_data = {
@@ -1852,6 +1854,51 @@
             '~bac/sample_bundle/1',
             bundle.basket_id)
 
+    def test_charm_resolving_from_bundle_branch(self):
+        service_data = {
+            'charm': 'mediawiki',
+            'branch': 'lp:charms/precise/mediawiki'
+        }
+        query = _build_charm_query_from_bundle_info(service_data, None)
+        self.assertEqual(
+            '~charmers/charms/precise/mediawiki/trunk',
+            query['branch_spec'])
+        self.assertNotIn('store_data.revision', query)
+
+        # And supports a version.
+        service_data['branch'] = 'lp:charms/precise/mediawiki@7'
+        query = _build_charm_query_from_bundle_info(service_data, None)
+        self.assertEqual(
+            '~charmers/charms/precise/mediawiki/trunk',
+            query['branch_spec'])
+        self.assertEqual(7, query['store_data.revision'])
+
+    def test_charm_resolving_from_bundle_charm_csurl(self):
+        service_data = {
+            'charm': 'cs:precise/mediawiki-7'
+        }
+        query = _build_charm_query_from_bundle_info(service_data, None)
+        self.assertEqual(
+            'cs:precise/mediawiki-7',
+            query['store_url'])
+
+    def test_charm_resolving_from_bundle_charm_and_series(self):
+        service_data = {
+            'charm': 'mediawiki'
+        }
+        series = 'precise'
+        query = _build_charm_query_from_bundle_info(service_data, series)
+        self.assertEqual(
+            'cs:precise/mediawiki',
+            query['address'])
+
+    def test_charm_resolving_from_bundle_empty_missing_info(self):
+        service_data = {
+            'charm': 'mediawiki'
+        }
+        query = _build_charm_query_from_bundle_info(service_data, None)
+        self.assertIsNone(query)
+
 
 class TestMakeBundleDoc(TestCase):
 
@@ -1953,6 +2000,76 @@
         self.assertEqual(expected, filelist)
 
 
+class TestBundleLoadingCharms(MongoTestBase):
+
+    def setUp(self):
+        """Create 3 of the same charms at different revisions"""
+        super(TestBundleLoadingCharms, self).setUp()
+        charm_args = {
+            'name': 'wikipedia',
+            'series': 'precise',
+            'owner': 'charmers',
+            'promulgated': True
+        }
+        charm_args['revision'] = 1
+        charm_args['store_revision'] = 1
+        factory.makeCharm(self.db, **charm_args)
+        charm_args['revision'] = 2
+        charm_args['store_revision'] = 2
+        factory.makeCharm(self.db, **charm_args)
+        charm_args['revision'] = 3
+        charm_args['store_revision'] = 3
+        factory.makeCharm(self.db, **charm_args)
+
+    def test_resolve_by_store_url(self):
+        data = {
+            'charm': 'cs:precise/wikipedia-1'
+        }
+        charm = resolve_charm_from_bundle_info(self.db, data, None)
+        self.assertIsNotNone(charm)
+        self.assertEqual('~charmers/precise/wikipedia/1', charm['_id'])
+        self.assertEqual('wikipedia', charm['name'])
+        self.assertEqual(1, charm['store_data']['revision'])
+        self.assertEqual(1, charm['revision'])
+
+    def test_resolve_by_name_and_series(self):
+        data = {
+            'charm': 'wikipedia'
+        }
+        charm = resolve_charm_from_bundle_info(self.db, data, 'precise')
+        self.assertIsNotNone(charm)
+        self.assertEqual('wikipedia', charm['name'])
+        self.assertEqual(3, charm['store_data']['revision'])
+        self.assertEqual(3, charm['revision'])
+
+    def test_resolve_by_branch(self):
+        data = {
+            'branch': 'lp:charms/precise/wikipedia'
+        }
+        charm = resolve_charm_from_bundle_info(self.db, data, 'precise')
+        self.assertIsNotNone(charm)
+        self.assertEqual('wikipedia', charm['name'])
+        self.assertEqual(3, charm['store_data']['revision'])
+        self.assertEqual(3, charm['revision'])
+
+    def test_resolve_by_branch_with_revision(self):
+        data = {
+            'branch': 'lp:charms/precise/wikipedia@2'
+        }
+        charm = resolve_charm_from_bundle_info(self.db, data, 'precise')
+        self.assertIsNotNone(charm)
+        self.assertEqual('wikipedia', charm['name'])
+        self.assertEqual(2, charm['store_data']['revision'])
+        self.assertEqual(2, charm['revision'])
+
+    def test_not_enough_info(self):
+        data = {
+            'name': 'wikipedia'
+        }
+        charm = resolve_charm_from_bundle_info(self.db, data, None)
+        self.assertIsNone(charm)
+
+
 class TestFeaturedSource(MongoTestBase):
 
     def test_set_and_get_work(self):

=== modified file 'charmworld/views/api.py'
--- charmworld/views/api.py	2013-10-03 17:13:56 +0000
+++ charmworld/views/api.py	2013-10-07 14:35:40 +0000
@@ -29,6 +29,7 @@
     FeaturedSource,
     getfs,
     QADataSource,
+    resolve_charm_from_bundle_info,
 )
 from charmworld.search import (
     BUNDLE,
@@ -427,6 +428,25 @@
                 query, sort=[('store_data.revision', pymongo.DESCENDING)])
         return charm_id, trailing, charm
 
+    def _build_bundle_metadata(self, bundle, db):
+        bundle_dict = dict(bundle)
+        # Add the list of files in the bundle.
+        bundle_dict['files'] = bundle.get_file_list(db)
+        bundle_dict['charm_metadata'] = {}
+        # Now load the charm information we require for the services in the
+        # bundle.
+        for service, data in bundle.data['services'].iteritems():
+            charm = resolve_charm_from_bundle_info(
+                db,
+                data,
+                bundle_dict.get('series')
+            )
+            if charm:
+                formatted = self._format_charm(Charm(charm))
+                bundle_dict['charm_metadata'][service] = formatted
+
+        return bundle_dict
+
     def _find_bundle(self, path):
         try:
             bundle_id, trailing, bundle_bits = self._parse_bundle_id(path)
@@ -463,6 +483,7 @@
         if charm_data is None:
             return json_response(
                 404, {'type': 'no_such_charm', 'charm_id': charm_id})
+
         charm = Charm(charm_data)
         if trailing is None:
             return self._charm_details(charm_data)
@@ -503,6 +524,7 @@
             result = {'type': 'no_such_bundle', 'bundle_id': '/'.join(path)}
             return json_response(status, result)
         if not trailing:
+            bundle = self._build_bundle_metadata(bundle, self.request.db)
             return json_response(200, bundle)
         elif trailing and trailing[0] == self.ICON_TRAILING:
             return self._bundle_file(bundle, filename)

=== modified file 'charmworld/views/tests/test_api.py'
--- charmworld/views/tests/test_api.py	2013-10-03 17:13:56 +0000
+++ charmworld/views/tests/test_api.py	2013-10-07 14:35:40 +0000
@@ -779,7 +779,20 @@
         self.assertEqual(200, response.status_code)
 
     def test_results_match(self):
-        self.makeBundle(name='bat', owner='bac', basket_with_rev='byobu/4')
+        # Make the charm that we'll use as a service.
+        _id, charm = factory.makeCharm(
+            self.db,
+            description=''
+        )
+        services = {
+            u'charm': {
+                u'branch': charm['branch_spec'],
+                u'charm': charm['name']
+            }
+        }
+        self.makeBundle(
+            name='bat', owner='bac',
+            basket_with_rev='byobu/4', services=services)
         response = self.get_response('bundle', '~bac/byobu/4/bat')
         self.assertEqual(200, response.status_code)
         self.assertEqual(
@@ -788,12 +801,21 @@
                 u'basket_name': u'byobu',
                 u'basket_revision': 4,
                 u'branch_deleted': False,
+                u'charm_metadata': {
+                    u'charm': self.api_class._format_charm(Charm(charm))
+                },
                 u'data': {
                     u'series': u'precise',
-                    u'services': {},
+                    u'services': {
+                        u'charm': {
+                            u'branch': charm['branch_spec'],
+                            u'charm': charm['name']
+                        }
+                    },
                     u'relations': {},
                 },
                 u'description': u'',
+                u'files': [],
                 u'id': u'~bac/byobu/4/bat',
                 u'name': u'bat',
                 u'owner': u'bac',