Merge lp:~abentley/charmworld/remove-api-0 into lp:~juju-jitsu/charmworld/trunk

[Aaron Bentley]

Remove API 0

Remove API 0 and introduce a migration to remove icons from charm data.

Since the charm formatting changes, the migration must also update elasticsearch. This is done using reindex, which gains a charms parameter so that mongodb can be used as a data source, and now handles the case where the index does not already exist.

Much test refactoring was done, since all current APIs (i.e. API1) run all
tests now.

copy_index was broken, because it invoked index_charms on itself, not the pending copy. This is now fixed. It is also renamed to "create_replacement", to better-describe its purpose.

unlimited_search is updated to raise IndexMissing if the search errors out with an IndexMissingException.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

[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/routes.py'
--- charmworld/routes.py	2013-04-29 18:21:35 +0000
+++ charmworld/routes.py	2013-06-04 16:12:32 +0000
@@ -80,9 +80,6 @@
     config.add_route('auth_callback', '/auth_callback')
     config.add_route('logout', '/logout')
 
-    # API
-    config.add_route('api_0', '/api/0/{endpoint}{remainder:.*}')
-    config.add_view('charmworld.views.api.API0', route_name='api_0')
     config.add_route('api_1', '/api/1/{endpoint}{remainder:.*}')
     config.add_view('charmworld.views.api.API1', route_name='api_1')
 

=== modified file 'charmworld/search.py'
--- charmworld/search.py	2013-05-29 12:36:35 +0000
+++ charmworld/search.py	2013-06-04 16:12:32 +0000
@@ -233,7 +233,8 @@
         kwargs = {'index': self.index_name}
         if limit is not None:
             kwargs['size'] = limit
-        hits = self._client.search(dsl, **kwargs)
+        with translate_error():
+            hits = self._client.search(dsl, **kwargs)
         if limit is None and len(hits['hits']['hits']) < hits['hits']['total']:
             kwargs['size'] = hits['hits']['total']
             hits = self._client.search(dsl, **kwargs)
@@ -348,40 +349,56 @@
             name = '%s-%d' % (self.index_name, random.randint(1, 99999))
         return self.__class__(self._client, name)
 
-    def copy_index(self, name=None):
-        """Create a copy of this index with the specified name.
+    def create_replacement(self, name=None, charms=None):
+        """Create a replacement for this index.
 
         The mapping will not be this index's mapping, it will be the current
         mapping (e.g. put_mapping).
 
+        :param name: If not supplied, a name based on this index's name will
+            be used.
+        :param charms: A list of charms.  If None, the charms in the current
+            index will be used.
         :return: The ElasticSearchClient for the supplied mapping.
         """
         copy = self.get_aliasable_client(name)
         copy.create_index()
         try:
             copy.put_mapping()
-            self.index_charms(self.api_search(valid_charms_only=False))
+            if charms is None:
+                try:
+                    charms = self.api_search(valid_charms_only=False)
+                except IndexMissing:
+                    charms = []
+            copy.index_charms(charms)
             return copy
         except:
             copy.delete_index()
             raise
 
 
-def reindex(index_client):
+def reindex(index_client, charms=None):
     """Reindex documents with the current mapping.
 
     This works by creating a new index and then aliasing it to the old index.
     If the old index was already an alias, this is a simple swap.  If not, the
     old index is deleted.
+
+    :param charms: A list of charms to use.  If None, the charms in the index
+        will be used.
     """
-    new_index = index_client.copy_index()
+    new_index = index_client.create_replacement(charms=charms)
     try:
         aliased = index_client.get_aliased()
         if aliased == []:
-            index_client.delete_index()
+            try:
+                index_client.delete_index()
+            except IndexMissing:
+                pass
         index_client.update_aliased(new_index.index_name, aliased)
     except:
         new_index.delete_index()
+        raise
     if aliased != []:
         for name in aliased:
             ElasticSearchClient(index_client._client, name).delete_index()

=== modified file 'charmworld/tests/test_search.py'
--- charmworld/tests/test_search.py	2013-05-29 12:36:35 +0000
+++ charmworld/tests/test_search.py	2013-06-04 16:12:32 +0000
@@ -444,12 +444,29 @@
         alias.update_aliased('bar', alias.get_aliased())
         self.assertEqual(['bar'], alias.get_aliased())
 
-    def test_copy_index(self):
-        self.index_client.index_charm({'_id': 'a', 'name': 'foo'})
-        copy = self.index_client.copy_index('index-copy')
-        self.addCleanup(copy.delete_index)
-        copy.wait_for_startup()
-        self.assertIn('series', copy.get_mapping()['charm']['properties'])
+    def test_create_replacement(self):
+        self.index_client.index_charm({'_id': 'a', 'name': 'foo'})
+        copy = self.index_client.create_replacement('index-copy')
+        self.addCleanup(copy.delete_index)
+        copy.wait_for_startup()
+        self.assertIn('series', copy.get_mapping()['charm']['properties'])
+        self.assertEqual({'_id': 'a', 'name': 'foo'}, copy.get('a'))
+
+    def test_create_replacement_charms(self):
+        self.index_client.index_charm({'_id': 'a', 'name': 'foo'})
+        copy = self.index_client.create_replacement(
+            charms=[{'_id': 'a', 'name': 'bar'}])
+        self.addCleanup(copy.delete_index)
+        copy.wait_for_startup()
+        self.assertIn('series', copy.get_mapping()['charm']['properties'])
+        self.assertEqual({'_id': 'a', 'name': 'bar'}, copy.get('a'))
+
+    def test_create_replacement_misssing(self):
+        client = ElasticSearchClient.from_settings(get_ini(), 'temp-index')
+        # This should not raise an exception, even though the index has not
+        # been created.
+        copy = client.create_replacement()
+        self.addCleanup(copy.delete_index)
 
     def test_put_mapping_incompatible_mapping_error(self):
         # The error we get from an incompatible mapping is IncompatibleMapping
@@ -600,3 +617,10 @@
         mapping = alias.get_mapping()
         self.assertEqual('not_analyzed',
                          mapping['charm']['properties']['name']['index'])
+
+    def test_reindexed_no_client_charms(self):
+        client = ElasticSearchClient.from_settings(get_ini())
+        # This should not raise an exception, even though the index does not
+        # exist.
+        new_client = reindex(client, charms=[])
+        new_client.delete_index()

=== modified file 'charmworld/views/api.py'
--- charmworld/views/api.py	2013-06-03 16:18:58 +0000
+++ charmworld/views/api.py	2013-06-04 16:12:32 +0000
@@ -412,33 +412,3 @@
                 'popular': self._charm_results(popular),
             }
         }
-
-
-class API0(API1):
-    """Implementation of API 0.
-
-    All methods whose names do not begin with an underscore are exposed.
-    Methods may return a webob Response, which is returned directly, or a
-    json-serializable value, which will be returned as a json HTTP response.
-    """
-
-    @classmethod
-    def _charm_result(cls, charm, requires, provides):
-        return cls._format_charm(charm)
-
-    def _charm_results(self, charms):
-        return [self._charm_result(charm, {}, {}) for charm in charms]
-
-    def _charms(self, limit=None, name=None, series=None, owner=None,
-                provides=None, requires=None, type_=None, provider=None,
-                scope=None, category=None, categories=None, text=None):
-        return super(API0, self)._charms(
-            limit, name, series, owner, provides, requires, type_, provider,
-            scope, categories, text)
-
-    @staticmethod
-    def _format_charm(charm):
-        output = API1._format_charm(charm)
-        if 'icon' in charm:
-            output['icon'] = charm['icon']
-        return output

=== modified file 'charmworld/views/tests/test_api.py'
--- charmworld/views/tests/test_api.py	2013-06-03 16:18:58 +0000
+++ charmworld/views/tests/test_api.py	2013-06-04 16:12:32 +0000
@@ -15,7 +15,6 @@
 )
 from charmworld.models import CharmSource
 from charmworld.views.api import (
-    API0,
     API1,
     json_response,
 )
@@ -73,6 +72,30 @@
         return self.api_class(request)()
 
 
+class API1Mixin(APITestBase):
+
+    api_class = API1
+
+    @staticmethod
+    def weightless(charm):
+        """Remove weight from charm because we cannot predict its value."""
+        charm = dict(charm)
+        del charm['weight']
+        return charm
+
+    @classmethod
+    def weightless_relation(cls, relation):
+        new_relation = {}
+        for interface, charms in relation.items():
+            charms = [cls.weightless(charm) for charm in charms]
+            new_relation[interface] = charms
+        return new_relation
+
+    @classmethod
+    def related_weightless(cls, charm):
+        return cls.weightless(cls.api_class._format_related(charm, 0))
+
+
 class TestAPICharms:
 
     def setUp(self):
@@ -259,19 +282,20 @@
         self.assertIn('new', data['result'])
         self.assertIn('featured', data['result'])
         self.assertEqual(['pop%d' % num for num in range(14, 4, -1)],
-                         [self.get_name(r) for r in data['result']['popular']])
+                         [r['charm']['name']
+                          for r in data['result']['popular']])
         self.assertEqual(self.api_class._charm_result(last_popular_charm, {},
                                                       {}),
                          data['result']['popular'][0])
         self.assertEqual(['new%d' % num for num in range(14, 4, -1)],
-                         [self.get_name(r) for r in data['result']['new']])
+                         [r['charm']['name'] for r in data['result']['new']])
         self.assertEqual(self.api_class._charm_result(last_new_charm, {}, {}),
                          data['result']['new'][0])
         self.assertItemsEqual(
             ['featured%d' % num for num in range(14, -1, -1)],
-            [self.get_name(r) for r in data['result']['featured']])
+            [r['charm']['name'] for r in data['result']['featured']])
         featured14, = [charm for charm in data['result']['featured'] if
-                       self.get_name(charm) == 'featured14']
+                       charm['charm']['name'] == 'featured14']
         self.assertEqual(
             self.api_class._charm_result(last_featured_charm, {}, {}),
             featured14)
@@ -281,40 +305,6 @@
         response = self.get_response('charms', categories='asdf')
         self.assertEqual(200, response.status_code)
 
-
-class API1Mixin:
-
-    api_class = API1
-
-    @staticmethod
-    def weightless(charm):
-        """Remove weight from charm because we cannot predict its value."""
-        charm = dict(charm)
-        del charm['weight']
-        return charm
-
-    @classmethod
-    def weightless_relation(cls, relation):
-        new_relation = {}
-        for interface, charms in relation.items():
-            charms = [cls.weightless(charm) for charm in charms]
-            new_relation[interface] = charms
-        return new_relation
-
-    @classmethod
-    def related_weightless(cls, charm):
-        return cls.weightless(cls.api_class._format_related(charm, 0))
-
-
-class TestAPI1Charms(TestAPICharms, APITestBase, API1Mixin):
-
-    def setUp(self):
-        super(TestAPI1Charms, self).setUp()
-
-    @staticmethod
-    def get_name(charm):
-        return charm['charm']['name']
-
     def test_charms(self):
         """Charms endpoint produces expected result."""
         one_changes = [factory.makeChange(created=datetime(1999, 12, 31),
@@ -585,190 +575,8 @@
         )
 
 
-class TestAPI0Charms(TestAPICharms, APITestBase):
-
-    api_class = API0
-
-    @staticmethod
-    def get_name(charm):
-        return charm['name']
-
-    def test_charms(self):
-        """Charms endpoint produces expected result."""
-        one_changes = [factory.makeChange(created=datetime(1999, 12, 31),
-                                          revno=37, message='Hello')]
-        one_id, one = self.makeCharm(name='name1', changes=one_changes,
-                                     date_created=datetime(1997, 10, 29,
-                                                           12, 10, 9),
-                                     promulgated=True)
-        self.index_client.index_charm(one)
-        maintainer2 = '[e-mail] (J. Random Hacker)'
-        provides2 = {'cookie-factory': {
-            'interface': 'imap',
-        }}
-        requires2 = {'postal-service': {
-            'interface': 'envelope',
-        }}
-        options2 = {
-            'foobar': {
-                'type': 'str',
-                'default': 'baz',
-                'description': 'quxx',
-            },
-        }
-        files2 = {
-            'readme': {
-                'filename': 'README.md',
-                'subdir': '',
-            },
-            'config-changed': {
-                'filename': 'config-changed',
-                'subdir': 'hooks',
-            },
-        }
-        two_changes = [factory.makeChange(created=datetime(1998, 11, 30),
-                                          revno=32, message='Hi',
-                                          authors=['Baz <[e-mail]>'])]
-        two_id, two = self.makeCharm(owner='jrandom', revision=3,
-                                     series='warty', summary='Charm two',
-                                     name='name2', revno=13,
-                                     maintainer=maintainer2,
-                                     commit_message='message2',
-                                     provides=provides2,
-                                     requires=requires2, options=options2,
-                                     files=files2, subordinate=True,
-                                     changes=two_changes,
-                                     date_created=datetime(1996, 9, 28,
-                                                           11, 9, 8))
-        response = self.get_response('charms')
-        self.assertEqual(200, response.status_code)
-        self.assertEqual('application/json', response.content_type)
-        result = response.json_body['result']
-        self.assertEqual(2, len(result))
-        one_url = make_store_url(1, get_address(one, short=True))
-        self.assertEqual({'owner': 'charmers',
-                          'maintainer': {
-                              'name': 'John Doe',
-                              'email': '[e-mail]',
-                          },
-                          'id': one_url[3:],
-                          'categories': [],
-                          'date_created': '1997-10-29T12:10:09',
-                          'name': 'name1',
-                          'url': one_url,
-                          'description': one['description'],
-                          'revision': 1,
-                          'summary': "Remote terminal classroom over https",
-                          'distro_series': 'precise',
-                          'is_approved': True,
-                          'rating_numerator': 0,
-                          'rating_denominator': 0,
-                          'code_source': {
-                              'type': 'bzr',
-                              'location':
-                                  'lp:~charmers/charms/precise/name1/trunk',
-                              'revision': '12',
-                              'last_log': 'maintainer',
-                              'bugs_link':
-                                  'https://bugs.launchpad.net/charms/'
-                                  '+source/name1',
-                              'revisions': [{
-                                  'revno': 37,
-                                  'message': 'Hello',
-                                  'date': '1999-12-31T00:00:00Z',
-                                  'authors': [
-                                      {
-                                          'name': 'Foo',
-                                          'email': '[e-mail]'
-                                      },
-                                      {
-                                          'name': 'Bar',
-                                          'email': '[e-mail]'
-                                      },
-                                  ],
-                              }],
-                          },
-                          'relations': {
-                              'provides': {
-                                  'website': {
-                                      'interface': 'https',
-                                  },
-                              },
-                              'requires': {
-                                  'database': {
-                                      'interface': 'mongodb',
-                                  },
-                              },
-                          },
-                          'options': {
-                              'script-interval': {
-                                  'type': 'int',
-                                  'default': 5,
-                                  'description':
-                                  'The interval between script runs.'
-                              },
-                          },
-                          'files': ['README', 'hooks/install'],
-                          'tested_providers': {},
-                          'downloads_in_past_30_days': 0,
-                          'is_subordinate': False,
-                          }, result[0])
-        two_url = make_store_url(1, get_address(two, short=False))
-        self.assertEqual({'owner': 'jrandom',
-                          'maintainer': {
-                              'name': 'J. Random Hacker',
-                              'email': '[e-mail]',
-                          },
-                          'id': two_url[3:],
-                          'categories': [],
-                          'date_created': '1996-09-28T11:09:08',
-                          'name': 'name2',
-                          'url': two_url,
-                          'description': two['description'],
-                          'revision': 3,
-                          'summary': 'Charm two',
-                          'distro_series': 'warty',
-                          'is_approved': False,
-                          'rating_numerator': 0,
-                          'rating_denominator': 0,
-                          'code_source': {
-                              'type': 'bzr',
-                              'location':
-                                  'lp:~jrandom/charms/warty/name2/trunk',
-                              'revision': '13',
-                              'revisions': [{
-                                  'revno': 32,
-                                  'message': 'Hi',
-                                  'date': '1998-11-30T00:00:00Z',
-                                  'authors': [
-                                      {
-                                          'name': 'Baz',
-                                          'email': '[e-mail]'
-                                      },
-                                  ],
-                              }],
-                              'last_log': 'message2',
-                              'bugs_link':
-                              'https://bugs.launchpad.net/charms/+source/name2'
-                          },
-                          'relations': {
-                              'provides': provides2,
-                              'requires': requires2,
-
-                          },
-                          'options': options2,
-                          'files': ['README.md', 'hooks/config-changed'],
-                          'tested_providers': {},
-                          'downloads_in_past_30_days': 0,
-                          'is_subordinate': True,
-                          }, result[1])
-        self.assertNotEqual(one_id, two_id)
-        self.assertNotEqual(one_url, two_url)
-
-    def test_charms_accepts_category(self):
-        """Category is accepted (but ignored)."""
-        response = self.get_response('charms', category='asdf')
-        self.assertEqual(200, response.status_code)
+class TestAPI1Charms(TestAPICharms, API1Mixin):
+    """Test the AP1 implementation of charms endpoint."""
 
 
 class TestAPI:
@@ -1151,9 +959,6 @@
         # Only the headers matter for a CORS preflight response.
         self.assertEqual(0, response.content_length)
 
-
-class TestAPI1(APITestBase, TestAPI, API1Mixin):
-
     def test_charm_result_includes_charm_and_metadata(self):
         charm = factory.get_charm_json(promulgated=False)
         result = self.api_class._charm_result(charm, {}, {})
@@ -1283,29 +1088,5 @@
         self.assertNotIn('icon', formatted)
 
 
-class TestAPI0(APITestBase, TestAPI):
-
-    api_class = API0
-
-    @staticmethod
-    def get_id(charm):
-        return charm['id']
-
-    def test_charm_result_excludes_charm_and_metadata(self):
-        charm = factory.get_charm_json(promulgated=False)
-        result = self.api_class._charm_result(charm, {}, {})
-        self.assertNotIn('metadata', result)
-        self.assertNotIn('charm', result)
-
-    def test_format_charm_adds_icon(self):
-        """If a charm provides an icon, it is added to the data returned
-        by the API.
-        """
-        charmid, charm = factory.makeCharm(self.db)
-        # The test data does not contain an icon.
-        self.assertFalse('icon' in charm)
-        formatted = self.api_class._format_charm(charm)
-        self.assertFalse('icon' in formatted)
-        charm['icon'] = 'fake'
-        formatted = self.api_class._format_charm(charm)
-        self.assertTrue('icon' in formatted)
+class TestAPI1(TestAPI, API1Mixin):
+    """Test API 1."""

=== modified file 'migrations/migrate.py'
--- migrations/migrate.py	2013-02-12 11:36:14 +0000
+++ migrations/migrate.py	2013-06-04 16:12:32 +0000
@@ -17,6 +17,7 @@
 
 from charmworld.models import getconnection
 from charmworld.models import getdb
+from charmworld.search import ElasticSearchClient
 from charmworld.utils import get_ini
 
 SCRIPT_TEMPLATE = """
@@ -152,7 +153,13 @@
         """:returns: Latest version in Collection"""
         return self.version_indexes[-1] if len(self.version_indexes) > 0 else 0
 
-    def upgrade(self, datastore, init):
+    def run_migration(self, db, index_client, module_name):
+        module = imp.load_source(
+            module_name.strip('.py'),
+            join(self.versions_dir, module_name))
+        getattr(module, 'upgrade')(db, index_client)
+
+    def upgrade(self, datastore, index_client, init):
         """Run `upgrade` methods for required version files.
 
         :param datastore: An instance of DataStore
@@ -176,10 +183,7 @@
             # Let's get processing.
             next_version = current_version + 1
             module_name = self.versions[next_version]
-            module = imp.load_source(
-                module_name.strip('.py'),
-                join(self.versions_dir, module_name))
-            getattr(module, 'upgrade')(datastore.db)
+            self.run_migration(datastore.db, index_client, module_name)
             current_version = next_version
             datastore.update_version(current_version)
         return current_version
@@ -252,8 +256,9 @@
     def upgrade(cls, ini, args):
         """Upgrade the data store to the latest available migration."""
         ds = cls.get_datastore(ini)
+        index_client = ElasticSearchClient.from_settings(ini)
         migrations = Versions(ini['migrations'])
-        new_version = migrations.upgrade(ds, args.init)
+        new_version = migrations.upgrade(ds, index_client, args.init)
 
         if new_version is None:
             print 'There are no new migrations to apply'

=== modified file 'migrations/test_migrate.py'
--- migrations/test_migrate.py	2013-02-12 18:57:02 +0000
+++ migrations/test_migrate.py	2013-06-04 16:12:32 +0000
@@ -6,10 +6,10 @@
 from os.path import exists
 import pymongo
 from tempfile import mkdtemp
-from unittest import TestCase
 
 from charmworld.models import getconnection
 from charmworld.models import getdb
+from charmworld.testing import TestCase
 from charmworld.testing.factory import random_string
 from charmworld.utils import get_ini
 
@@ -155,16 +155,19 @@
 
     def test_upgrade_wo_migrations(self):
         """Upgrade runs silently if no migrations are created yet."""
+        self.use_index_client()
         self.ds.version_datastore()
         tmpdir = mkdtemp()
         versions = Versions(tmpdir)
-        self.assertEqual(versions.upgrade(self.ds, False), None)
+        self.assertEqual(versions.upgrade(self.ds, self.index_client, False),
+                         None)
 
     def test_upgrade_succeeds(self):
         """upgrade will run the methods and we get a version of 4."""
         self.ds.version_datastore()
+        self.use_index_client()
         # We should get 4 versions done. The upgrade returns 4
-        last_version = self.version.upgrade(self.ds, False)
+        last_version = self.version.upgrade(self.ds, self.index_client, False)
         self.assertEqual(4, last_version)
 
         # and we can query the db for the revision of 4
@@ -172,7 +175,8 @@
 
     def test_upgrade_succeeds_from_unversioned(self):
         """Forcing init no an unversioned db will upgrade successfully."""
-        last_version = self.version.upgrade(self.ds, True)
+        self.use_index_client()
+        last_version = self.version.upgrade(self.ds, self.index_client, True)
         self.assertEqual(4, last_version)
 
         # and we can query the db for the revision of 4

=== modified file 'migrations/versions/001_load_qa_questions.py'
--- migrations/versions/001_load_qa_questions.py	2013-02-12 11:36:14 +0000
+++ migrations/versions/001_load_qa_questions.py	2013-06-04 16:12:32 +0000
@@ -9,7 +9,7 @@
 """
 
 
-def upgrade(db):
+def upgrade(db, index_client):
     """Complete this function with work to be done for the migration/update.
 
     db is the pymongo db instance for our datastore. Charms are in db.charms

=== modified file 'migrations/versions/002_Escape_dots_dollar_signs_percent_signs_in_mongo_keys.py'
--- migrations/versions/002_Escape_dots_dollar_signs_percent_signs_in_mongo_keys.py	2013-02-12 11:36:14 +0000
+++ migrations/versions/002_Escape_dots_dollar_signs_percent_signs_in_mongo_keys.py	2013-06-04 16:12:32 +0000
@@ -5,7 +5,7 @@
 
 from charmworld.utils import quote_yaml
 
-def upgrade(db):
+def upgrade(db, index_client):
     """Iterate all charm data and replace all dictionary keys containing
     a '.', '$' or '%' with an escaped variant.
     """

=== modified file 'migrations/versions/003_remove_provider_qa_questions.py'
--- migrations/versions/003_remove_provider_qa_questions.py	2013-04-23 19:23:17 +0000
+++ migrations/versions/003_remove_provider_qa_questions.py	2013-06-04 16:12:32 +0000
@@ -21,7 +21,7 @@
 ]
 
 
-def upgrade(db):
+def upgrade(db, index_client):
     """Update each category of questions from new_category_data.
 
     This update elaborates on the 001_* migration. Subsequent qa form

=== modified file 'migrations/versions/004_remove_charm_errors.py'
--- migrations/versions/004_remove_charm_errors.py	2013-05-27 19:02:19 +0000
+++ migrations/versions/004_remove_charm_errors.py	2013-06-04 16:12:32 +0000
@@ -1,2 +1,4 @@
-def upgrade(db):
+# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+def upgrade(db, index_client):
     db.drop_collection('charm-errors')

=== added file 'migrations/versions/005_delete_icon_field.py'
--- migrations/versions/005_delete_icon_field.py	1970-01-01 00:00:00 +0000
+++ migrations/versions/005_delete_icon_field.py	2013-06-04 16:12:32 +0000
@@ -0,0 +1,8 @@
+# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+from charmworld.search import reindex
+
+
+def upgrade(db, index_client):
+    db.charms.update({}, {'$unset': {'icon':''}})
+    reindex(index_client, list(db.charms.find({})))

=== modified file 'migrations/versions/tests/test_migrations.py'
--- migrations/versions/tests/test_migrations.py	2013-05-27 19:02:19 +0000
+++ migrations/versions/tests/test_migrations.py	2013-06-04 16:12:32 +0000
@@ -2,8 +2,15 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import imp
+from charmworld.models import CharmSource
 from charmworld.testing import MongoTestBase
 from charmworld.testing.factory import random_string
+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 TestMigration002(MongoTestBase):
@@ -53,10 +60,18 @@
 class TestMigration004(MongoTestBase):
 
     def test_migration(self):
+        self.use_index_client()
         self.db.create_collection('charm-errors')
-        module = imp.load_source(
-            '004_remove_charm_errors',
-            'migrations/versions/'
-            '004_remove_charm_errors.py')
-        module.upgrade(self.db)
+        run_migration(self.db, self.index_client, '004_remove_charm_errors.py')
         self.assertNotIn('charm-errors', self.db.collection_names())
+
+
+class TestMigration005(MongoTestBase):
+
+    def test_migration(self):
+        self.use_index_client()
+        source = CharmSource.from_request(self)
+        source.save({'_id': 'a', 'icon': 'asdf', 'asdf': 'asdf'})
+        run_migration(self.db, self.index_client, '005_delete_icon_field.py')
+        for charm in source._get_all('a'):
+            self.assertNotIn('icon', charm)