Merge lp:~benji/charmworld/remove-charm into lp:charmworld

[Benji York]

Add a script to remove a charm

https://codereview.appspot.com/29210043/

R=bac

Add a script to remove a charm

https://codereview.appspot.com/29210043/

[Benji York (benji)]

Reviewers: mp+195839_code.launchpad.net,

Message:
Please take a look.

Description:
Add a script to remove a charm

https://code.launchpad.net/~benji/charmworld/remove-charm/+merge/195839

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/29210043/

Affected files (+218, -1 lines):
   A [revision details]
   M charmworld/models.py
   A charmworld/scripts/remove_charm.py
   A charmworld/scripts/tests/test_remove_charm.py
   M charmworld/search.py
   M charmworld/testing/factory.py
   M setup.py

[Brad Crittenden (bac)]

LGTM. Cannot do QA at this time.

https://codereview.appspot.com/29210043/diff/1/charmworld/scripts/tests/test_remove_charm.py
File charmworld/scripts/tests/test_remove_charm.py (right):

https://codereview.appspot.com/29210043/diff/1/charmworld/scripts/tests/test_remove_charm.py#newcode53
charmworld/scripts/tests/test_remove_charm.py:53: # A non-zero exit
status is set to indicate the error condition.
positive, non-zero

https://codereview.appspot.com/29210043/diff/1/charmworld/scripts/tests/test_remove_charm.py#newcode74
charmworld/scripts/tests/test_remove_charm.py:74: # Silence is a virtue
for sucessfully executing programs.
I love the editorial content.

https://codereview.appspot.com/29210043/diff/1/charmworld/search.py
File charmworld/search.py (right):

https://codereview.appspot.com/29210043/diff/1/charmworld/search.py#newcode102
charmworld/search.py:102: _BAD_CHARS = u'~()/[]^{}'
It is very odd that this caused the test failure you saw. The whole
point is a search with one of these characters would cause an exception.
  I wonder if you have a different version of ES installed that handles
it differently.

I'm confused why this doesn't cause the test you had trouble with
earlier to *always* fail.

https://codereview.appspot.com/29210043/

[Richard Harding (rharding)]

Search QA is ok. Search change just made it so that ~charmers works vs
produces an unparseable error. The ~ is dropped from the actual search
and it goes on with just 'charmers'.

https://codereview.appspot.com/29210043/

[Benji York (benji)]

Thanks for the review and QA guys!

https://codereview.appspot.com/29210043/diff/1/charmworld/scripts/tests/test_remove_charm.py
File charmworld/scripts/tests/test_remove_charm.py (right):

https://codereview.appspot.com/29210043/diff/1/charmworld/scripts/tests/test_remove_charm.py#newcode53
charmworld/scripts/tests/test_remove_charm.py:53: # A non-zero exit
status is set to indicate the error condition.
On 2013/11/19 19:05:48, bac wrote:
> positive, non-zero

Since POSIX requires it to be an "unsigned decimal integer", all
non-zero values are positive. ;)

https://codereview.appspot.com/29210043/

[lp:~benji/charmworld/remove-charm]

468. By Benji York on 2013-11-19

remove silly, ineffective argument

[Benji York (benji)]

Brad reviewed this.

review: Approve

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

review: Approve (continuous-integration)

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-11-13 19:54:55 +0000
+++ charmworld/models.py	2013-11-19 20:26:17 +0000
@@ -228,6 +228,7 @@
 
 
 def construct_charm_id(charm_data, use_revision=True):
+    """Build the ID of a charm from a mapping of that charm's attributes."""
     elements = [charm_data['owner'], charm_data['series'], charm_data['name']]
     if use_revision:
         elements.append('%d' % charm_data['store_data']['revision'])
@@ -1288,6 +1289,18 @@
     def create_collection(self):
         self.collection.database.create_collection(self.collection.name)
 
+    @staticmethod
+    def make_all_revisions_query(charm_data):
+        return {
+            'owner': charm_data['owner'],
+            'series': charm_data['series'],
+            'name': charm_data['name'],
+        }
+
+    def find_all_revisions(self, charm_data):
+        query = self.make_all_revisions_query(charm_data)
+        return list(self.collection.find(query))
+
 
 def get_basket_info(revno, **data):
     owner_segment = '~%s' % data['owner']

=== added file 'charmworld/scripts/remove_charm.py'
--- charmworld/scripts/remove_charm.py	1970-01-01 00:00:00 +0000
+++ charmworld/scripts/remove_charm.py	2013-11-19 20:26:17 +0000
@@ -0,0 +1,71 @@
+# Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import print_function
+
+__metaclass__ = type
+
+import sys
+from pyelasticsearch.exceptions import ElasticHttpNotFoundError
+from charmworld.utils import get_ini
+from charmworld.models import (
+    CharmSource,
+    FeaturedSource,
+    getconnection,
+    getdb,
+)
+from charmworld.search import ElasticSearchClient
+
+
+CHARM_ID_MESSAGE = (
+    'Please provide a charm ID, including revision. E.g., '
+    '~charmers/precise/apache2/14')
+
+
+def main(argv, stderr=sys.stderr, db=None, index_client=None):
+    """The main function for the server start timestamp removal script."""
+    if len(argv) != 2:
+        print(
+            'Please provide the ID of the charm you wish to remove.',
+            file=stderr)
+        return 1
+
+    charm_id = argv[1]
+
+    if db is None:
+        settings = get_ini()
+        connection = getconnection(settings)
+        db = getdb(connection, settings.get('mongo.database'))
+
+    # If the specified charm does not exist, tell the user.
+    charm_query = {'_id': charm_id}
+    charm_data = db.charms.find_one(charm_query)
+    if charm_data is None:
+        print('Unknown charm ID: %r' % charm_id, file=stderr)
+        print(CHARM_ID_MESSAGE, file=stderr)
+        return 1
+
+    if index_client is None:
+        index_client = ElasticSearchClient.from_settings(settings)
+
+    # The charm needs to be removed from the search engine.
+    try:
+        index_client.delete_charm(charm_data)
+    except ElasticHttpNotFoundError:
+        # It is fine if the charm was not indexed.
+        pass
+
+    # If the charm is featured, we want to unfeature it.
+    source = FeaturedSource.from_db(db)
+    source.clear_featured(charm_data, 'charm')
+
+    # Finally, remove all the revisions of the charm document from the
+    # database.
+    charm_query = CharmSource.make_all_revisions_query(charm_data)
+    db.charms.remove(charm_query)
+    return 0
+
+
+def entry_point():
+    """Script entry point for removing a server's start up timestamp."""
+    sys.exit(main(sys.argv))

=== added file 'charmworld/scripts/tests/test_remove_charm.py'
--- charmworld/scripts/tests/test_remove_charm.py	1970-01-01 00:00:00 +0000
+++ charmworld/scripts/tests/test_remove_charm.py	2013-11-19 20:26:17 +0000
@@ -0,0 +1,119 @@
+# Copyright 2012, 2013, Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE.txt).
+
+from cStringIO import StringIO
+import os
+
+from charmworld.models import (
+    Charm,
+    CharmSource,
+    construct_charm_id,
+    FeaturedSource,
+)
+from charmworld.scripts import remove_charm
+from charmworld.testing import MongoTestBase
+from charmworld.testing.factory import make_charm
+from charmworld.utils import project_root
+
+
+class RemoveCharmScriptTests(MongoTestBase):
+
+    def setUp(self):
+        super(RemoveCharmScriptTests, self).setUp()
+        # This is a lot of work just to create a new revision of a charm.
+        older_charm = make_charm(self.db)
+        newer_charm_data = older_charm._representation
+        newer_charm_data['store_data']['revision'] += 1
+        newer_charm_data['_id'] = construct_charm_id(newer_charm_data)
+        self.db.charms.insert(newer_charm_data)
+        self.charm = Charm(newer_charm_data)
+        # As a convienience we will create a stderr stand-in here.
+        self.stderr = StringIO()
+        # Be paranoid about the charms actually existing in the database.
+        assert self.db.charms.find_one({'_id': older_charm._id})
+        assert self.db.charms.find_one({'_id': self.charm._id})
+
+    def is_indexed(self, id_):
+        return self.index_client.get(id_) is not None
+
+    def test_entry_point_exists(self):
+        # The entry point is wrapped in a script by setuptools.  We want to be
+        # sure the entrypoint actually exists.
+        self.assertIn(
+            'remove-charm = charmworld.scripts.remove_charm:entry_point\n',
+            open(os.path.join(project_root, 'setup.py')).read())
+
+    def test_script_chides_you_for_not_providing_an_id(self):
+        # If no charm ID is provided on the command line the script prints a
+        # helpful message.
+        exit_status = remove_charm.main([], self.stderr)
+        self.assertEqual(
+            self.stderr.getvalue(),
+            'Please provide the ID of the charm you wish to remove.\n')
+        # A non-zero exit status is set to indicate the error condition.
+        self.assertGreater(exit_status, 0)
+
+    def test_script_reports_unkown_charm_ids(self):
+        # The script will generate an error message if the given charm ID does
+        # not exist in the DB.
+        exit_status = remove_charm.main(['', 'not a charm ID'], self.stderr)
+        self.assertIn(
+            "Unknown charm ID: 'not a charm ID'\n",
+            self.stderr.getvalue())
+        # The user is also given a hint as to how the ID is structured.
+        self.assertIn(remove_charm.CHARM_ID_MESSAGE, self.stderr.getvalue())
+        # A non-zero exit status is set to indicate the error condition.
+        self.assertGreater(exit_status, 0)
+
+    def test_script_removes_charm_from_mongodb(self):
+        # The script removes the charm with the provided ID from the database.
+        # ...we can invoke the script to delete it.
+        exit_status = remove_charm.main(['', self.charm._id], self.stderr)
+        # Lo and behold, the charm is actually gone.
+        self.assertFalse(self.db.charms.find_one({'_id': self.charm._id}))
+        # Silence is a virtue for sucessfully executing programs.
+        self.assertEqual(self.stderr.getvalue(), '')
+        # The exit status signals success.
+        self.assertEqual(exit_status, 0)
+
+    def test_script_removes_charm_from_featuerd(self):
+        # If a charm is featured, its featured status is revoked by the script.
+        source = FeaturedSource.from_db(self.db)
+        charm_data = self.charm._representation
+        source.set_featured(charm_data, 'charm')
+        exit_status = remove_charm.main(['', self.charm._id], self.stderr)
+        self.assertFalse(source.is_featured(charm_data, 'charm'))
+        # The exit status signals success.
+        self.assertEqual(exit_status, 0)
+
+    def test_script_removes_charm_from_elastic_search(self):
+        # The charm needs to be indexed so the script can remove it.
+        self.use_index_client()
+        charm_data = self.charm._representation
+        self.index_client.index_charm(charm_data)
+        # Ensure the charm actually got indexed.
+        assert self.is_indexed(charm_data['_id'])
+        # Removing a charm should also remove it from the search engine.
+        exit_status = remove_charm.main(
+            ['', self.charm._id], self.stderr, index_client=self.index_client)
+        # The script removed the charm from the index.
+        self.assertFalse(self.is_indexed(charm_data['_id']))
+        # The exit status signals success.
+        self.assertEqual(exit_status, 0)
+
+    def test_old_revisions_of_the_charm_are_removed_too(self):
+        # Requesting the removal of a charm results in all revisions of that
+        # charm being removed.
+        charm_data = self.charm._representation
+        # This can't be the best way to find all the revisions of a particular
+        # charm.
+        source = CharmSource(self.db, index_client='fake')
+        # There should exist more than one revision of the charm in the
+        # database.
+        assert len(source.find_all_revisions(charm_data)) > 1
+        # Run the script.
+        exit_status = remove_charm.main(['', self.charm._id], self.stderr)
+        # The script removed all the charm revisions from the database.
+        self.assertEqual(len(source.find_all_revisions(charm_data)), 0)
+        # The exit status signals success.
+        self.assertEqual(exit_status, 0)

=== modified file 'charmworld/search.py'
--- charmworld/search.py	2013-11-15 20:00:35 +0000
+++ charmworld/search.py	2013-11-19 20:26:17 +0000
@@ -99,7 +99,7 @@
                                         e)
 
 
-_BAD_CHARS = u'()/[]^{}'
+_BAD_CHARS = u'~()/[]^{}'
 _BAD_CHAR_TABLE = [
     unichr(c) if unichr(c) not in _BAD_CHARS else u' ' for c in range(128)]
 _BAD_CHAR_TABLE = u''.join(_BAD_CHAR_TABLE)

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-11-08 14:59:51 +0000
+++ charmworld/testing/factory.py	2013-11-19 20:26:17 +0000
@@ -30,6 +30,7 @@
 )
 from charmworld.models import (
     Bundle,
+    Charm,
     CharmFileSet,
     construct_charm_id,
     getfs,
@@ -317,6 +318,16 @@
     return record_id, charm
 
 
+def make_charm(db, **kwargs):
+    """Create a new charm and add it to the database.
+
+    This function returns a charm model.
+    """
+    charm_data = get_charm_json(**kwargs)
+    db.charms.insert(charm_data)
+    return Charm(charm_data)
+
+
 def get_bundle_data(name=None, owner=None, basket_with_rev=None,
                     series='precise', title='', description='',
                     services=dict(), relations=dict(), promulgated=False,

=== modified file 'setup.py'
--- setup.py	2013-11-15 16:58:32 +0000
+++ setup.py	2013-11-19 20:26:17 +0000
@@ -41,6 +41,7 @@
       sync-index = charmworld.models:sync_index
       remove-start-time-entry = \
         charmworld.scripts.remove_server_start_time:entry_point
+      remove-charm = charmworld.scripts.remove_charm:entry_point
       [beaker.backends]
       mongodb = mongodb_beaker:MongoDBNamespaceManager
       """,