Merge lp:~james-w/pkgme-service/review-click into lp:pkgme-service

[James Westby]

Split out the logic of packaging an app from the mechanics of handling callbacks.

We want to re-use the callback logic for reviewing click packages, so split up
the existing code to make that separation clearer.

Hi,

This is the first of my stack of branches to add reviewing of click
packages to pkgme-service.

It splits the logic of running some code and sending callback/errbacks to
sca from the logic of packaging an application. We'll then be able to glue
in the click review code as an alternative codepath while re-using the
callback code.

Thanks,

James

[James Westby (james-w)]

This is largely a moving of code, with a couple of changes such as moving the
json.dumps in to the submit_to_myapps function.

Thanks,

James

[Ricardo Kirkner (ricardokirkner)]

LGTM

review: Approve

[lp:~james-w/pkgme-service/review-click]

143. By James Westby on 2013-10-09

[r=ricardokirkner] Split out the logic of packaging an app from the mechanics of handling callbacks.

We want to re-use the callback logic for reviewing click packages, so split up
the existing code to make that separation clearer.

=== modified file 'src/djpkgme/client.py'
--- src/djpkgme/client.py	2012-09-14 13:59:48 +0000
+++ src/djpkgme/client.py	2013-10-09 20:40:35 +0000
@@ -41,13 +41,13 @@
     """Called when any error happens during package build."""
     body = get_response_dict(None, metadata)
     body['error'] = error_data
-    submit_to_myapps(metadata['errback_url'], json.dumps(body),
-            logger=logger)
-
-
-def submit_to_myapps(url, json_body, logger=None):
-    """Put ``json_body`` to MyApps at ``url``."""
+    submit_to_myapps(metadata['errback_url'], body, logger=logger)
+
+
+def submit_to_myapps(url, body, logger=None):
+    """Put ``body`` json-encoded to MyApps at ``url``."""
     GOOD_RESPONSES = (200,)
+    json_body = json.dumps(body)
 
     headers = {}
     if url.startswith(settings.MYAPPS_BASE_URL):
@@ -66,11 +66,6 @@
         raise CallbackError(url, 'PUT', response.status, content)
 
 
-def submit_pkgme_info(packaged_app_url, metadata, logger=None):
-    body = json.dumps(get_response_dict(packaged_app_url, metadata))
-    return submit_to_myapps(metadata['callback_url'], body, logger=logger)
-
-
 def get_response_dict(packaged_app_url, metadata):
     """Return a json-serializable guess set for the myapps callback."""
 

=== modified file 'src/djpkgme/tasks.py'
--- src/djpkgme/tasks.py	2012-11-13 14:51:44 +0000
+++ src/djpkgme/tasks.py	2013-10-09 20:40:35 +0000
@@ -26,8 +26,9 @@
 
 from .client import (
     CallbackError,
+    get_response_dict,
     submit_error,
-    submit_pkgme_info,
+    submit_to_myapps,
     )
 from .osutils import (
     download_file,
@@ -164,15 +165,116 @@
         PkgmeError,
     )
 
-    # The name of the directory that we store the downloaded file in.
-    DOWNLOAD_DIRECTORY_NAME = 'download'
-
     REQUIRED_FIELDS = [
         'callback_url',
         'package_url',
         ]
 
-    def build_package(self, metadata, overrides):
+    def _submit_error(self, metadata, error_data):
+        if metadata.get('errback_url', None):
+            self.get_logger().error(
+                "Submitting error info to %s" % (metadata['errback_url'],))
+            try:
+                submit_error(metadata, error_data, logger=self.get_logger())
+            except CallbackError, e:
+                # XXX: Should generate an OOPS report and log the OOPS number.
+                # Obviously we can't send to the client.
+                self.get_logger().error(
+                    "Error while trying to submit error to client: %s\n%s"
+                    % (str(e), error_data))
+        else:
+            raise
+
+    def _validate_metadata(self, metadata):
+        """Make sure 'metadata' has all the fields we need."""
+        missing_fields = []
+        for field in self.REQUIRED_FIELDS:
+            if field not in metadata:
+                missing_fields.append(field)
+        if missing_fields:
+            raise MissingFieldsError(metadata, missing_fields)
+
+    def _log_failure_exception(self, metadata, ftype):
+        self.get_logger().info("%s (%s) failed with %s exception: %s" % (
+            ftype,
+            metadata.get('myapps_id', '<unknown myapps id>'),
+            metadata.get('package_url', '<unknown package url>'),
+            traceback.format_exc(),
+            ))
+
+    def run(self, metadata, overrides):
+        """Build a source package.
+
+        Requires 'metadata' to have:
+         * callback_url
+         * package_url
+
+        Uses these if they exist:
+         * errback_url
+         * myapps_id
+
+        Assumes that the backend can determine a non-empty value for
+        'package_name'.
+        """
+        self._validate_metadata(metadata)
+        self.get_logger().info(
+            "Building package for %s: %s" % (
+                metadata.get('myapps_id', '<unknown myapps id>'),
+                metadata.get('package_url', '<unknown package url>'),
+                ))
+        job = PackageAppJob()
+        return self.run_with_error_handling(job, metadata, overrides)
+
+    def run_with_error_handling(self, job, metadata, overrides):
+        try:
+            generated_info = job.run(metadata, overrides, self.get_logger())
+        except self.EXPECTED_EXCEPTIONS, e:
+            # pkgme couldn't make the package.  If the user were running
+            # 'pkgme' on the command line they would get a simple,
+            # user-friendly error.  Just send that, and not the whole
+            # traceback.
+            self._log_failure_exception(metadata, "user")
+            self._submit_error(metadata, str(e))
+        except FATAL_EXCEPTIONS:
+            # Someone hit Ctrl-C or something like that. Just keep raising.
+            raise
+        except:
+            # The exception is something unexpected, and thus represents a bug
+            # in the system.
+            #
+            # XXX: Should generate an OOPS and report that to the client.  For
+            # now, send the error across without the traceback.  This is an
+            # interim measure to reduce our reliance on timeouts for doing
+            # system testing.
+            self._log_failure_exception(metadata, "internal")
+            exc_type, exc_obj, tb = sys.exc_info()
+            self._submit_error(
+                metadata, exc_info_as_dict((exc_type, exc_obj, None)))
+        else:
+            self.get_logger().info(
+                "Submitting success info to %r: %s"
+                % (metadata['callback_url'], generated_info))
+            try:
+                callback_body = job.transform_results(
+                    generated_info, metadata)
+                submit_to_myapps(metadata['callback_url'], callback_body,
+                    logger=self.get_logger())
+            except CallbackError:
+                self._submit_error(metadata, exc_info_as_dict(sys.exc_info()))
+
+tasks.register(BuildPackageTask)
+
+
+class PackageAppJob(object):
+
+    # The name of the directory that we store the downloaded file in.
+    DOWNLOAD_DIRECTORY_NAME = 'download'
+
+    def transform_results(self, package_path, metadata):
+        package_url = get_url_for_file(package_path)
+        return get_response_dict(package_url, metadata)
+
+    def run(self, metadata, overrides, logger):
         """Build packaging for 'metadata'.
 
         :return: A path on disk to a tarball containing the built source
@@ -201,7 +303,6 @@
         # devportal-metadata.json) are tarred up and stored in a preconfigured
         # location: PKGME_OUTPUT_DIRECTORY.
         working_dirs = []
-        logger = self.get_logger()
         with TempDir() as temp_dir:
             output_dir = temp_dir.path
             download_dir = os.path.join(
@@ -256,97 +357,6 @@
             return make_tarball(
                 output_dir, settings.PKGME_OUTPUT_DIRECTORY, package_name)
 
-    def _submit_error(self, metadata, error_data):
-        if metadata.get('errback_url', None):
-            self.get_logger().error(
-                "Submitting error info to %s" % (metadata['errback_url'],))
-            try:
-                submit_error(metadata, error_data, logger=self.get_logger())
-            except CallbackError, e:
-                # XXX: Should generate an OOPS report and log the OOPS number.
-                # Obviously we can't send to the client.
-                self.get_logger().error(
-                    "Error while trying to submit error to client: %s\n%s"
-                    % (str(e), error_data))
-        else:
-            raise
-
-    def _validate_metadata(self, metadata):
-        """Make sure 'metadata' has all the fields we need."""
-        missing_fields = []
-        for field in self.REQUIRED_FIELDS:
-            if field not in metadata:
-                missing_fields.append(field)
-        if missing_fields:
-            raise MissingFieldsError(metadata, missing_fields)
-
-    def _log_failure_exception(self, metadata, ftype):
-        self.get_logger().info("%s (%s) failed with %s exception: %s" % (
-            ftype,
-            metadata.get('myapps_id', '<unknown myapps id>'),
-            metadata.get('package_url', '<unknown package url>'),
-            traceback.format_exc(),
-            ))
-
-    def run(self, metadata, overrides):
-        """Build a source package.
-
-        Requires 'metadata' to have:
-         * callback_url
-         * package_url
-
-        Uses these if they exist:
-         * errback_url
-         * myapps_id
-
-        Assumes that the backend can determine a non-empty value for
-        'package_name'.
-        """
-        self._validate_metadata(metadata)
-        self.get_logger().info(
-            "Building package for %s: %s" % (
-                metadata.get('myapps_id', '<unknown myapps id>'),
-                metadata.get('package_url', '<unknown package url>'),
-                ))
-        try:
-            packaged_app_path = self.build_package(metadata, overrides)
-        except self.EXPECTED_EXCEPTIONS, e:
-            # pkgme couldn't make the package.  If the user were running
-            # 'pkgme' on the command line they would get a simple,
-            # user-friendly error.  Just send that, and not the whole
-            # traceback.
-            self._log_failure_exception(metadata, "user")
-            self._submit_error(metadata, str(e))
-        except FATAL_EXCEPTIONS:
-            # Someone hit Ctrl-C or something like that. Just keep raising.
-            raise
-        except:
-            # The exception is something unexpected, and thus represents a bug
-            # in the system.
-            #
-            # XXX: Should generate an OOPS and report that to the client.  For
-            # now, send the error across without the traceback.  This is an
-            # interim measure to reduce our reliance on timeouts for doing
-            # system testing.
-            self._log_failure_exception(metadata, "internal")
-            exc_type, exc_obj, tb = sys.exc_info()
-            self._submit_error(
-                metadata, exc_info_as_dict((exc_type, exc_obj, None)))
-        else:
-            packaged_app_url = get_url_for_file(packaged_app_path)
-            self.get_logger().info(
-                "Submitting success info to %r: %s"
-                % (metadata['callback_url'], packaged_app_url))
-            try:
-                submit_pkgme_info(packaged_app_url, metadata,
-                    logger=self.get_logger())
-            except CallbackError:
-                self._submit_error(metadata, exc_info_as_dict(sys.exc_info()))
-            else:
-                return packaged_app_url
-
-tasks.register(BuildPackageTask)
-
 
 class CauseOopsTask(Task):
     def run(self):

=== modified file 'src/djpkgme/tests/test_handlers.py'
--- src/djpkgme/tests/test_handlers.py	2012-03-12 14:10:27 +0000
+++ src/djpkgme/tests/test_handlers.py	2013-10-09 20:40:35 +0000
@@ -46,7 +46,7 @@
 
     # Patch httplib2 so the handler doesn't try to PUT to callback_url
     @patch('httplib2.Http.request', return_value=(FakeResponse(200, 'OK'), ''))
-    @patch('djpkgme.tasks.BuildPackageTask.build_package')
+    @patch('djpkgme.tasks.PackageAppJob.run')
     def test_no_overrides_success(self, mock_build_package, mock_request):
         request = Mock()
         mock_build_package.return_value = 'foo'
@@ -57,7 +57,7 @@
 
     # Patch httplib2 so the handler doesn't try to PUT to callback_url
     @patch('httplib2.Http.request', return_value=(FakeResponse(200, 'OK'), ''))
-    @patch('djpkgme.tasks.BuildPackageTask.build_package')
+    @patch('djpkgme.tasks.PackageAppJob.run')
     def test_overrides_success(self, mock_build_package, mock_request):
         request = Mock()
         mock_build_package.return_value = 'foo'

=== modified file 'src/djpkgme/tests/test_tasks.py'
--- src/djpkgme/tests/test_tasks.py	2012-11-15 15:34:28 +0000
+++ src/djpkgme/tests/test_tasks.py	2013-10-09 20:40:35 +0000
@@ -2,6 +2,7 @@
 # http://ask.github.com/celery/cookbook/unit-testing.html
 
 import json
+import logging
 import os
 import sys
 import tarfile
@@ -32,6 +33,7 @@
     BuildPackageTask,
     CallbackError,
     get_url_for_file,
+    PackageAppJob,
     prepare_icons,
     prepare_metadata,
     translate_dict,
@@ -225,7 +227,7 @@
 class BuildPackageTaskTestCase(TestCaseWithFactory):
 
     @patch('httplib2.Http.request', return_value=(FakeResponse(200, 'OK'), ''))
-    @patch('djpkgme.tasks.BuildPackageTask.build_package')
+    @patch('djpkgme.tasks.PackageAppJob.run')
     def test_build_package_is_called(self, mock_build_package, mock_request):
         mock_build_package.return_value = "foo"
 
@@ -234,12 +236,11 @@
 
         result = BuildPackageTask.delay(metadata, overrides)
 
-        self.assertEqual(get_url_for_file('foo'), result.get())
         self.assertTrue(result.successful())
         self.assertEqual(1, mock_build_package.call_count)
 
     @patch('httplib2.Http.request', return_value=(FakeResponse(200, 'OK'), ''))
-    @patch('djpkgme.tasks.BuildPackageTask.build_package')
+    @patch('djpkgme.tasks.PackageAppJob.run')
     def test_result_is_submitted(self, mock_build_package, mock_request):
         mock_build_package.return_value = "foo"
 
@@ -260,8 +261,8 @@
         self.assertEqual(expected, json.loads(kwargs['body']))
 
     @patch('djpkgme.tasks.submit_error')
-    @patch('djpkgme.tasks.submit_pkgme_info')
-    @patch('djpkgme.tasks.BuildPackageTask.build_package')
+    @patch('djpkgme.tasks.submit_to_myapps')
+    @patch('djpkgme.tasks.PackageAppJob.run')
     def test_cannot_send_callback(self, mock_build_package, mock_submit,
                                   mock_error):
         # If we get a negative response to our callback, then we try to send
@@ -285,7 +286,7 @@
 
     @patch('djpkgme.tasks.BuildPackageTask.get_logger')
     @patch('djpkgme.tasks.submit_error')
-    @patch('djpkgme.tasks.BuildPackageTask.build_package')
+    @patch('djpkgme.tasks.PackageAppJob.run')
     def test_cannot_send_errback(self, mock_build_package, mock_error,
                                  mock_get_logger):
         # If we get a negative response to our errback, then we just log the
@@ -310,25 +311,24 @@
         self.patch(task.request, 'loglevel', UNREASONABLY_HIGH_LEVEL)
 
     @patch('djpkgme.client.submit_to_myapps')
-    @patch('djpkgme.tasks.BuildPackageTask.build_package')
+    @patch('djpkgme.tasks.PackageAppJob.run')
     def test_failed_build_package(self, mock_build_package, mock_submit):
         metadata = self.factory.make_metadata()
         mock_build_package.side_effect = PkgmeError('some failure')
         task = BuildPackageTask()
         self._suppress_logging(task)
         task.run(metadata, {})
-        [((url, json_body), kwargs)] = mock_submit.call_args_list
+        [((url, body), kwargs)] = mock_submit.call_args_list
         self.assertEqual(metadata['errback_url'], url)
         del kwargs['logger']
         self.assertEqual({}, kwargs)
-        body = json.loads(json_body)
         error = body.pop('error')
         self.assertEqual(
             [metadata['errback_url'],
              get_response_dict(None, metadata), 'some failure', {}],
             [url, body, error, kwargs])
 
-    @patch('djpkgme.tasks.BuildPackageTask.build_package')
+    @patch('djpkgme.tasks.PackageAppJob.run')
     def test_failed_build_package_no_errback_url(self, mock_build_package):
         # If no errback URL is provided then just raise.
         metadata = self.factory.make_metadata()
@@ -338,7 +338,7 @@
         self.assertRaises(PkgmeError, task.run, metadata, {})
 
     @patch('djpkgme.client.submit_to_myapps')
-    @patch('djpkgme.tasks.BuildPackageTask.build_package')
+    @patch('djpkgme.tasks.PackageAppJob.run')
     def test_failed_build_package_unexpected(self, mock_build_package,
                                              mock_submit):
         metadata = self.factory.make_metadata()
@@ -347,11 +347,10 @@
         task = BuildPackageTask()
         self._suppress_logging(task)
         task.run(metadata, {})
-        [((url, json_body), kwargs)] = mock_submit.call_args_list
+        [((url, body), kwargs)] = mock_submit.call_args_list
         self.assertEqual(metadata['errback_url'], url)
         del kwargs['logger']
         self.assertEqual({}, kwargs)
-        body = json.loads(json_body)
         error = body.pop('error')
         self.assertThat(
             [url, body, error, kwargs],
@@ -363,7 +362,7 @@
                          'traceback': 'RuntimeError: some failure\n'}),
                  Equals({})]))
 
-    @patch('djpkgme.tasks.BuildPackageTask.build_package')
+    @patch('djpkgme.tasks.PackageAppJob.run')
     def test_failed_build_package_unexpected_no_errback_url(
         self, mock_build_package):
         # If no errback URL is provided then just raise.
@@ -373,7 +372,7 @@
         task = BuildPackageTask()
         self.assertRaises(RuntimeError, task.run, metadata, {})
 
-    @patch('djpkgme.tasks.BuildPackageTask.build_package')
+    @patch('djpkgme.tasks.PackageAppJob.run')
     def test_failed_build_package_fatal(self, mock_build_package):
         # If keyboard interrupt or something like that is raised, then
         # re-raise, even if errback_url is set.
@@ -383,11 +382,11 @@
         self.assertRaises(KeyboardInterrupt, task.run, metadata, {})
 
 
-class TestBuildPackageTaskIntegration(TestCaseWithFactory):
+class TestPackageAppJobIntegration(TestCaseWithFactory):
     """Tests that actually build the package."""
 
     def run_task(self, metadata, file_path):
-        task = BuildPackageTask()
+        job = PackageAppJob()
         def download_file(url, output_dir):
             # Create a tarball and stick it in 'output_dir'.
             proper_location = os.path.join(
@@ -398,7 +397,8 @@
         self.useFixture(LibdepFixture(DEPENDENCIES_FOR_TEST_DATA))
         temp_dir = self.useFixture(TempDir()).path
         self.patch(settings, 'PKGME_OUTPUT_DIRECTORY', temp_dir)
-        return task.build_package(metadata, {})
+        logger = logging.getLogger()
+        return job.run(metadata, {}, logger)
 
     def test_writes_packaging_for_tarball(self):
         # BuildPackageTask.build_package runs pkgme to build the package and