Merge lp:~abentley/charmworld/config-storage into lp:~juju-jitsu/charmworld/trunk

[Aaron Bentley]

Change the internal representation of config options, so that the user-supplied option names are used as values, not as keys.

In this update, the upgrade is performed as an exodus, rather than a fast migration. Also, some unused code is deleted.

Note: This branch must not land until a corresponding update to the charm is in place.

[Curtis Hovey (sinzui)]

We talked on IRC to clarify that some charms are not in a sane state and migration needs to handle cases where config is not a dict with a single item of key, sub-dict. We might want migrate to log in the future. We know from the 007 migration problem seen with production's bogus charms that the juju-log is capturing stdout.

review: Approve (code)

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2013-07-01 14:45:02 +0000
+++ charmworld/jobs/ingest.py	2013-07-05 14:17:29 +0000
@@ -32,11 +32,14 @@
     get_branch_info,
     parse_date,
 )
-from charmworld.models import getconnection
-from charmworld.models import getdb
-from charmworld.models import getfs
-from charmworld.models import CharmFileSet
-from charmworld.models import CharmSource
+from charmworld.models import (
+    CharmFileSet,
+    CharmSource,
+    getconnection,
+    getdb,
+    getfs,
+    options_to_storage,
+)
 from charmworld.search import (
     ElasticSearchClient,
     SearchServiceNotAvailable,
@@ -491,7 +494,11 @@
         config_raw = cfile.read()
 
         try:
-            config = quote_yaml(yaml.load(config_raw))
+            config_yaml = yaml.load(config_raw)
+            if 'options' in config_yaml:
+                config_yaml['options'] = options_to_storage(
+                    config_yaml['options'])
+            config = quote_yaml(config_yaml)
         except Exception, exc:
             raise IngestError(
                 'Invalid charm config yaml. %s: %s' % (

=== modified file 'charmworld/jobs/tests/test_scan.py'
--- charmworld/jobs/tests/test_scan.py	2013-06-24 15:45:29 +0000
+++ charmworld/jobs/tests/test_scan.py	2013-07-05 14:17:29 +0000
@@ -34,8 +34,40 @@
         # 'foo.bar' is replaced with 'foo%2Ebar'.
         self.assertFalse('dotted.name' in self.charm)
         self.assertEqual('foo', self.charm['dotted%2Ename'])
-        self.assertFalse('foo.bar' in self.charm['config']['options'])
-        self.assertEqual('baz', self.charm['config']['options']['foo%2Ebar'])
+
+    def test_options_format(self):
+        del self.charm['config']
+        scan_charm(self.charm, self.db, self.fs, self.log)
+        expected = [
+            {
+                'name': 'source',
+                'default': 'lp:charmworld',
+                'type': 'string',
+                'description': 'The bzr branch to pull the charmworld source'
+                               ' from.'
+            },
+            {
+                'name': 'revno',
+                'default': -1,
+                'type': 'int',
+                'description': 'The revno of the bzr branch to use. -1 for'
+                               ' current tip.'
+            },
+            {
+                'name': 'execute-ingest-every',
+                'default': 15,
+                'type': 'int',
+                'description': 'The amount of time (in minutes) that the'
+                               ' ingest tasks for charmworld are executed'
+            },
+            {
+                'name': 'error-email',
+                'default': '',
+                'type': 'string',
+                'description': 'Address to mail errors to.'
+            }
+        ]
+        self.assertItemsEqual(expected, self.charm['config']['options'])
 
     def test_short_interface_specification(self):
         # Charms can specify the interfaces they require or provide

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-07-05 14:17:29 +0000
+++ charmworld/models.py	2013-07-05 14:17:29 +0000
@@ -544,7 +544,11 @@
 
         The dict is parsed from the config_raw property's YAML.
         """
-        return self._representation['config']
+        config = self._representation['config']
+        if config is not None and 'options' in config:
+            config = dict(config)
+            config['options'] = storage_to_options(config['options'])
+        return config
 
     @property
     def config_raw(self):
@@ -557,7 +561,7 @@
     @property
     def options(self):
         """The charm's configuration options."""
-        config = self._representation['config']
+        config = self.config
         if config is not None and 'options' in config:
             return config['options']
         else:
@@ -992,7 +996,11 @@
 
     def save(self, charm):
         self.collection.save(charm)
-        self.index_client.index_charm(charm)
+        try:
+            self.index_client.index_charm(charm)
+        except:
+            self.index_client.delete_charm(charm['_id'])
+            raise
 
     def get_store_charms(self):
         return self.collection.find({'store_url': {'$exists': True}})
@@ -1078,3 +1086,33 @@
             logger.exception('Unable to index charm.')
         else:
             logger.info('Syncing %s to Elasticsearch' % charm_id)
+
+
+def options_to_storage(options):
+    """Convert options, as retrieved from config.yaml, into a list.
+
+    This format avoids user-defined keys, which are problematic in
+    elasticsearch.
+    """
+    storage = []
+    for key, value in options.items():
+        option = {'name': key}
+        option.update(value)
+        storage.append(option)
+    return storage
+
+
+def storage_to_options(storage):
+    """Convert options, as retrieved from mongodb, into a dict.
+
+    This format has the same structure as config.yaml.
+    """
+    options = {}
+    for option in storage:
+        new_option = dict(option)
+        del new_option['name']
+        value = options.setdefault(option['name'], new_option)
+        if value is not new_option:
+            raise ValueError('Option name "%s" occurs multiple times.' %
+                             option['name'])
+    return options

=== modified file 'charmworld/search.py'
--- charmworld/search.py	2013-07-05 14:17:29 +0000
+++ charmworld/search.py	2013-07-05 14:17:29 +0000
@@ -30,7 +30,7 @@
     'name': 10,
     'summary': 5,
     'description': 3,
-    'config.options.*.description': None,
+    'config.options.description': None,
     'relations': None,
 }
 
@@ -154,13 +154,24 @@
         """
         exact_index = ['categories', 'name', 'owner', 'i_provides',
                        'i_requires', 'series', 'store_url']
+        charm_properties = dict(
+            (name, {'type': 'string', 'index': 'not_analyzed'})
+            for name in exact_index)
+        charm_properties['config'] = {
+            'properties': {
+                'options': {
+                    'properties': {
+                        'default': {'type': 'string'},
+                        'description': {'type': 'string'},
+                    }
+                }
+            }
+        }
+
         with translate_error():
             self._client.put_mapping(self.index_name, 'charm', {
                 'charm': {
-                    'properties': dict(
-                        (name, {'type': 'string', 'index': 'not_analyzed'})
-                        for name in exact_index
-                    )
+                    'properties': charm_properties,
                 }
             })
 
@@ -174,6 +185,10 @@
             self._client.index(self.index_name, 'charm', charm, charm_id,
                                refresh=True)
 
+    def delete_charm(self, charm_id):
+        with translate_error():
+            self._client.delete(self.index_name, 'charm', charm_id)
+
     def index_charms(self, charms):
         if len(charms) == 0:
             return

=== modified file 'charmworld/testing/data/sample_charm/config.yaml'
--- charmworld/testing/data/sample_charm/config.yaml	2013-02-07 17:26:59 +0000
+++ charmworld/testing/data/sample_charm/config.yaml	2013-07-05 14:17:29 +0000
@@ -15,4 +15,3 @@
     type: string
     default: ""
     description: "Address to mail errors to."
-  foo.bar: baz

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-07-01 14:45:02 +0000
+++ charmworld/testing/factory.py	2013-07-05 14:17:29 +0000
@@ -24,8 +24,9 @@
     process_charm,
 )
 from charmworld.models import (
+    CharmFileSet,
     getfs,
-    CharmFileSet,
+    options_to_storage,
 )
 
 
@@ -185,6 +186,7 @@
                 'description': 'The interval between script runs.'
             },
         }
+    options = options_to_storage(options)
     if files is None:
         files = {
             'readme': {

=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py	2013-07-01 14:45:02 +0000
+++ charmworld/tests/test_models.py	2013-07-05 14:17:29 +0000
@@ -14,6 +14,10 @@
 from os.path import join
 
 from mock import patch
+from pyelasticsearch.exceptions import (
+    ElasticHttpError,
+    ElasticHttpNotFoundError,
+)
 
 from charmworld.models import (
     acquire_session_secret,
@@ -22,7 +26,9 @@
     CharmFileSet,
     CharmSource,
     find_charms,
+    options_to_storage,
     QAData,
+    storage_to_options,
     sync_index,
     UserMgr,
 )
@@ -437,11 +443,10 @@
 
     def test_config(self):
         # The config property is a dict of charm options.
-        config = {'options': {'mode': 'fast'}}
-        charm_data = factory.get_charm_json()
-        charm_data['config'] = config
+        config = {'options': {'key': {'default': 'value', 'type': 'string'}}}
+        charm_data = factory.get_charm_json(options=config['options'])
         charm = Charm(charm_data)
-        self.assertIs(config, charm.config)
+        self.assertEqual(config, charm.config)
         # The default is a dict with a single key named 'options' set to
         # an empty dict.
         charm = Charm({})
@@ -460,12 +465,10 @@
 
     def test_options(self):
         # The options property is a dict from the charm's config.
-        options = {'mode': 'fast'}
-        config = {'options': options}
-        charm_data = factory.get_charm_json()
-        charm_data['config'] = config
+        options = {'key': {'default': 'value', 'type': 'string'}}
+        charm_data = factory.get_charm_json(options=options)
         charm = Charm(charm_data)
-        self.assertIs(options, charm.options)
+        self.assertEqual(options, charm.options)
         # The default is a dict.
         charm = Charm({})
         self.assertEqual({}, charm.options)
@@ -1040,3 +1043,58 @@
             messages = [r.getMessage() for r in handler.buffer]
             self.assertIn('Unable to index charm.', messages)
         self.assertEqual(1, len(self.index_client.api_search()))
+
+    def test_save_deletes_on_error(self):
+        self.use_index_client()
+        source = CharmSource.from_request(self)
+        source.save({'_id': 'a', 'b': {}})
+        with self.assertRaises(ElasticHttpError):
+            source.save({'_id': 'a', 'b': 'c'})
+        with self.assertRaises(ElasticHttpNotFoundError):
+            self.index_client.get('a')
+
+
+class TestOptionsStorage(TestCase):
+
+    yaml = {
+        'foo': {
+            'default': 'bar',
+            'type': 'string',
+            'description': 'Hello.'
+        },
+        'baz': {
+            'default': 42,
+            'type': 'int',
+            'description': 'Hello.'
+        },
+    }
+    storage = [{
+        'name': 'baz',
+        'default': 42,
+        'type': 'int',
+        'description': 'Hello.'
+    }, {
+        'name': 'foo',
+        'default': 'bar',
+        'type': 'string',
+        'description': 'Hello.'
+    }]
+
+    def test_options_to_storage(self):
+        self.assertItemsEqual(self.storage, options_to_storage(self.yaml))
+
+    def test_storage_to_options(self):
+        self.assertEqual(self.yaml, storage_to_options(self.storage))
+
+    def test_storage_to_options_duplicate_key(self):
+        with self.assertRaises(ValueError) as exc_context:
+            storage_to_options([{'name': 'same'}, {'name': 'same'}])
+        self.assertEqual('Option name "same" occurs multiple times.',
+                         str(exc_context.exception))
+
+    def test_roundtrip(self):
+        self.assertEqual(
+            self.yaml, storage_to_options(options_to_storage(self.yaml)))
+        self.assertItemsEqual(
+            self.storage,
+            options_to_storage(storage_to_options(self.storage)))

=== modified file 'charmworld/tests/test_search.py'
--- charmworld/tests/test_search.py	2013-07-01 14:45:02 +0000
+++ charmworld/tests/test_search.py	2013-07-05 14:17:29 +0000
@@ -173,7 +173,7 @@
             elif key in ('provides', 'requires'):
                 query = query.values()[0].values()[0]
             elif key == 'config':
-                query = query.values()[0].values()[0]['description']
+                query = query.values()[0][0]['description']
             elif key == 'store_data':
                 query = query['digest']
             elif key == 'files':

=== modified file 'charmworld/views/charms.py'
--- charmworld/views/charms.py	2013-07-03 21:33:20 +0000
+++ charmworld/views/charms.py	2013-07-05 14:17:29 +0000
@@ -165,7 +165,10 @@
 
 
 def _json_charm(charm):
+    charm_config = charm.config
     charm = dict(charm._representation)
+    # Reformat config into old internal representation.
+    charm['config'] = charm_config
     charm_keys = set(charm.keys())
     for k in SANITIZE.intersection(charm_keys):
         del charm[k]

=== modified file 'charmworld/views/tests/test_charms.py'
--- charmworld/views/tests/test_charms.py	2013-07-03 21:33:20 +0000
+++ charmworld/views/tests/test_charms.py	2013-07-05 14:17:29 +0000
@@ -1,6 +1,7 @@
 # Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from datetime import date
 from logging import getLogger
 
 from charmworld.jobs.ingest import (
@@ -16,7 +17,6 @@
 from charmworld.testing import ViewTestBase
 from charmworld.testing import WebTestBase
 from charmworld.utils import quote_key
-from charmworld.views import SANITIZE
 from charmworld.views.api import API2
 from charmworld.views.charms import (
     _charm_view,
@@ -35,7 +35,65 @@
     quality_edit,
     series_collection,
 )
-import json
+
+
+DISTRO_CHARM_DATA = {
+    'name': 'foo',
+    'series': 'precise',
+    'owner': 'bar',
+    'maintainer': 'John Doe <[e-mail]>',
+    'summary': 'Remote terminal classroom over https',
+    'description': 'foobar',
+    'store_url': 'cs:precise/foo-1',
+    'bzr_branch': '',
+    'promulgated': True,
+    'branch_deleted': False,
+    'categories': [],
+    'is_featured': False,
+    'subordinate': False,
+    'requires': {'database': {'interface': 'mongodb'}},
+    'provides': {'website': {'interface': 'https'}},
+    'config': {
+        'options': {
+            'script-interval': {
+                'default': 5,
+                'type': 'int',
+                'description': 'The interval between script runs.'
+            }
+        }
+    },
+    'ensemble': 'formula',
+    'downloads': 0,
+    'downloads_in_past_30_days': 0,
+    'files': {
+        'readme': {
+            'subdir': '',
+            'filename': 'README',
+        },
+        'install': {
+            'subdir': 'hooks',
+            'filename': 'install',
+        },
+    },
+    'tests': {},
+    'test_results': {},
+    'proof': {
+        'w': [
+            ' no README file',
+            ' (install:31) - hook accesses EC2 metadata service'
+            ' directly'
+        ]
+    },
+    'date_created': '2012-01-01',
+    'error': '',
+    'qa': None,
+    'last_change': {
+        'committer': u'John Doe <[e-mail]>',
+        'message': u'maintainer',
+        'revno': 12,
+        'created': 1343082725.06
+    }
+}
 
 
 class TestCharms(ViewTestBase):
@@ -368,17 +426,15 @@
         self.enable_routes()
         ignore, charm = factory.makeCharm(
             self.db, name='foo', owner='bar', series='precise',
-            promulgated=True)
+            description='foobar', promulgated=True,
+            date_created=date(2012, 1, 1))
         request = self.getRequest()
         request.matchdict = {
             'charm': 'foo',
             'series': 'precise',
         }
-        charm_data = dict(Charm(charm)._representation)
-        for key in SANITIZE.intersection(charm_data.keys()):
-            del charm_data[key]
         response = distro_charm_json(request)
-        self.assertEqual(charm_data, json.loads(response.body))
+        self.assertEqual(DISTRO_CHARM_DATA, response.json_body)
 
     def test_valid_others(self):
         charm = factory.makeCharm(self.db, files={})[1]

=== added file 'migrations/versions/010_remap_options.py'
--- migrations/versions/010_remap_options.py	1970-01-01 00:00:00 +0000
+++ migrations/versions/010_remap_options.py	2013-07-05 14:17:29 +0000
@@ -0,0 +1,17 @@
+# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from charmworld.models import (
+    options_to_storage,
+)
+
+
+def exodus_update(source, target):
+    for charm in source.collection.find():
+        config = charm.get('config')
+        if config is not None and 'options' in config:
+            config['options'] = options_to_storage(config['options'])
+        try:
+            target.save(charm)
+        except Exception as e:
+            print e

=== modified file 'migrations/versions/tests/test_migrations.py'
--- migrations/versions/tests/test_migrations.py	2013-06-11 19:18:47 +0000
+++ migrations/versions/tests/test_migrations.py	2013-07-05 14:17:29 +0000
@@ -1,34 +1,95 @@
 # Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from pyelasticsearch.exceptions import (
+    ElasticHttpNotFoundError,
+)
 from charmworld.models import CharmSource
 from charmworld.testing import MongoTestBase
 from migrations.migrate import Versions
 
 
-def run_migration(db, index_client, module_name):
-    versions = Versions('migrations/versions/')
-    versions.run_migration(db, index_client, module_name)
-
-
-class TestMigration004(MongoTestBase):
+class MigrationTestBase(MongoTestBase):
+
+    def setUp(self):
+        super(MigrationTestBase, self).setUp()
+        self.versions = Versions('migrations/versions/')
+
+
+class TestMigration004(MigrationTestBase):
 
     def test_migration(self):
         self.use_index_client()
         self.db.create_collection('charm-errors')
-        run_migration(self.db, self.index_client, '004_remove_charm_errors.py')
+        self.versions.run_migration(self.db, self.index_client,
+                                    '004_remove_charm_errors.py')
         self.assertNotIn('charm-errors', self.db.collection_names())
 
 
-class TestMigration008(MongoTestBase):
+class TestMigration008(MigrationTestBase):
 
     def test_migration(self):
         self.use_index_client()
         source = CharmSource.from_request(self)
         source.save({'_id': 'a', 'icon': 'asdf', 'asdf': 'asdf'})
         source.save({'_id': 'b', 'icon': 'asdf', 'asdf': 'asdf'})
-        run_migration(self.db, self.index_client, '008_delete_icon_field.py')
-        for charm in source._get_all('a'):
-            self.assertNotIn('icon', charm)
-        for charm in source._get_all('b'):
-            self.assertNotIn('icon', charm)
+        self.versions.run_migration(self.db, self.index_client,
+                                    '008_delete_icon_field.py')
+        for charm in source._get_all('a'):
+            self.assertNotIn('icon', charm)
+        for charm in source._get_all('b'):
+            self.assertNotIn('icon', charm)
+
+
+class TestMigration010(MigrationTestBase):
+
+    def test_migration(self):
+        self.use_index_client()
+        source = CharmSource.from_request(self)
+        source.save({
+            '_id': 'a',
+            'config': None,
+        })
+        source.save({
+            '_id': 'b',
+            'config': {
+                'options': {},
+            },
+        })
+        source.save({
+            '_id': 'c',
+            'config': {
+                'options': {'foo': {'default': 'bar'}},
+            },
+        })
+        source.save({'_id': 'd'})
+        # This is not acceptable to Elasticsearch, because config is a list.
+        self.db.charms.save({
+            '_id': 'e',
+            'config': ['a'],
+        })
+        self.versions.get_exodus('010_remap_options.py')(source, source)
+        for charm in source._get_all('a'):
+            self.assertEqual({
+            '_id': 'a',
+            'config': None,
+        }, charm)
+        for charm in source._get_all('b'):
+            self.assertEqual({
+                '_id': 'b',
+                'config': {'options': []},
+        }, charm)
+        for charm in source._get_all('c'):
+            self.assertEqual({
+                '_id': 'c',
+                'config': {'options': [{
+                    'name': 'foo',
+                    'default': 'bar',
+                }]}
+            }, charm)
+        for charm in source._get_all('d'):
+            self.assertEqual({'_id': 'd'}, charm)
+        with self.assertRaises(ElasticHttpNotFoundError):
+            self.index_client.get('e')
+        self.assertEqual({'_id': 'e', 'config': ['a']},
+                         self.db.charms.find_one({'_id': 'e'}))