Merge lp:~makyo/charms/trusty/juju-gui/guiserver-v4bundle into lp:charms/trusty/juju-gui

[Madison Scott-Clary]

Adds support for v4 (basketless) bundles as used by the charmstore. This retains legacy v3 functionality for the time being.

[Francesco Banconi (frankban)]

Hi Madison,

could you please re-propose this against the development branch?
It's lp:~juju-gui/charms/trusty/juju-gui/trunk

Thank you.

[lp:~makyo/charms/trusty/juju-gui/guiserver-v4bundle]

113. By Madison Scott-Clary on 2015-02-12

Reduce code repetition by adding a version to the params

114. By Madison Scott-Clary on 2015-02-13

Reduce repetition in views

115. By Madison Scott-Clary on 2015-02-17

Update docs to refer to v4 bundles

=== modified file 'server/guiserver/bundles/__init__.py'
--- server/guiserver/bundles/__init__.py	2013-11-15 12:41:59 +0000
+++ server/guiserver/bundles/__init__.py	2015-02-17 17:42:45 +0000
@@ -30,7 +30,9 @@
       The following arguments are passed to the validate and import_bundle
       interface methods:
         - user: a guiserver.auth.User instance, representing a logged in user;
-        - name: a string representing the name of the bundle to be imported;
+        - name: a string representing the name of the bundle to be imported; in
+          the case of v3 bundles, this will be the name of the bundle within
+          the basket, in v4 bundles, this will be the bundle ID;
         - bundle: a YAML decoded object representing the bundle contents.
       The watch and next interface methods are used to retrieve information
       about the status of the currently started/scheduled deployments.
@@ -87,7 +89,7 @@
 Importing a bundle.
 -------------------
 
-A deployment request looks like the following:
+A v3 deployment request looks like the following:
 
     {
         'RequestId': 1,
@@ -100,12 +102,26 @@
         },
     }
 
+A v4 deployment request looks like the following:
+
+    {
+        'RequestId': 1,
+        'Type': 'Deployer',
+        'Request': 'Import',
+        'Params': {
+            'Version': 4,
+            'YAML': 'bundle',
+            'BundleID': 'id'
+        },
+    }
+
 In the request parameters above, the YAML field stores the YAML encoded
-contents representing one or more bundles, and the Name field is the name of
-the specific bundle (included in YAML) that must be deployed. The Name
-parameter is optional in the case YAML includes only one bundle.  The BundleID
-is optional and is used for incrementing the deployment counter in
-Charmworld.
+contents representing one (or more, in the case of v3 baskets) bundles, and the
+Name field is the name of the specific bundle (included in YAML) that must be
+deployed. The Name parameter is optional in the case YAML includes only one
+bundle, or in the case of v4 bundles.  The BundleID is optional for v3 bundles
+and required for v4 bundles, and is used for incrementing the deployment
+counter in Charmworld.
 
 After receiving a deployment request, the DeployMiddleware sends a response
 indicating whether or not the request has been accepted. This response is sent

=== modified file 'server/guiserver/bundles/base.py'
--- server/guiserver/bundles/base.py	2014-12-17 13:19:51 +0000
+++ server/guiserver/bundles/base.py	2015-02-17 17:42:45 +0000
@@ -97,7 +97,7 @@
         self.importer_options = blocking.get_default_guiserver_options()
 
     @gen.coroutine
-    def validate(self, user, name, bundle):
+    def validate(self, user, bundle):
         """Validate the deployment bundle.
 
         The validation is executed in a separate process using the
@@ -105,7 +105,6 @@
 
         Three arguments are provided:
           - user: the current authenticated user;
-          - name: then name of the bundle to be imported;
           - bundle: a YAML decoded object representing the bundle contents.
 
         Return a Future whose result is a string representing an error or None
@@ -272,6 +271,7 @@
         self._deployer = deployer
         self._write_response = write_response
         self.routes = {
+            # Default import route
             'Import': views.import_bundle,
             'Watch': views.watch,
             'Next': views.next,
@@ -291,8 +291,9 @@
     def process_request(self, data):
         """Process a deployment request."""
         request_id = data['RequestId']
+        params = data.get('Params', {})
         view = self.routes[data['Request']]
-        request = ObjectDict(params=data.get('Params', {}), user=self._user)
+        request = ObjectDict(params=params, user=self._user)
         response = yield view(request, self._deployer)
         response['RequestId'] = request_id
         self._write_response(response)

=== modified file 'server/guiserver/bundles/views.py'
--- server/guiserver/bundles/views.py	2013-11-19 14:56:29 +0000
+++ server/guiserver/bundles/views.py	2015-02-17 17:42:45 +0000
@@ -76,12 +76,15 @@
     """Parse the request data and return a (name, bundle, bundle_id) tuple.
 
     In the tuple:
-      - name is the name of the bundle to be imported;
+      - name is the name of the bundle to be imported; this is required for old
+        style bundles, not for v4 bundles
       - bundle is the YAML decoded bundle object.
-      - bundle_id is the permanent id of the bundle of the form
+      - bundle_id is the permanent id of the bundle, in v3 of the form
         ~user/basketname/version/bundlename, e.g.
         ~jorge/mediawiki/3/mediawiki-simple.  The bundle_id is optional and
         will be None if not given.
+        In v4, the bundle ID is in the form ~user/bundlename, e.g.
+        ~jorge/mediawiki-simple and is required.
 
     Raise a ValueError if data represents an invalid request.
     """
@@ -92,6 +95,11 @@
         bundles = yaml.safe_load(contents)
     except Exception as err:
         raise ValueError('invalid YAML contents: {}'.format(err))
+    if params.get('Version') == 4:
+        bundle_id = params.get('BundleID')
+        return bundle_id, bundles, bundle_id
+
+    # This is an old-style bundle.
     name = params.get('Name')
     if name is None:
         # The Name is optional if the YAML contents contain only one bundle.
@@ -116,7 +124,12 @@
     assigned to the bundle deployment.
 
     Request: 'Import'.
-    Parameters example: {'Name': 'bundle-name', 'YAML': 'bundles'}.
+    Parameters example: {
+        'Name': 'bundle-name',
+        'YAML': 'bundles',
+        'Version': 4,
+        'BundleID': '~user/bundle-name',
+    }.
     """
     # Validate the request parameters.
     try:
@@ -130,7 +143,7 @@
         error = 'invalid request: invalid bundle {}: {}'.format(name, err)
         raise response(error=error)
     # Validate the bundle against the current state of the Juju environment.
-    err = yield deployer.validate(request.user, name, bundle)
+    err = yield deployer.validate(request.user, bundle)
     if err is not None:
         raise response(error='invalid request: {}'.format(err))
     # Add the bundle deployment to the Deployer queue.

=== modified file 'server/guiserver/tests/bundles/test_base.py'
--- server/guiserver/tests/bundles/test_base.py	2014-12-17 13:19:51 +0000
+++ server/guiserver/tests/bundles/test_base.py	2015-02-17 17:42:45 +0000
@@ -86,7 +86,7 @@
         # None is returned if the validation succeeds.
         deployer = self.make_deployer()
         with self.patch_validate():
-            result = yield deployer.validate(self.user, 'bundle', self.bundle)
+            result = yield deployer.validate(self.user, self.bundle)
         self.assertIsNone(result)
 
     @gen_test
@@ -95,7 +95,7 @@
         deployer = self.make_deployer()
         error = ValueError('validation error')
         with self.patch_validate(side_effect=error):
-            result = yield deployer.validate(self.user, 'bundle', self.bundle)
+            result = yield deployer.validate(self.user, self.bundle)
         self.assertEqual(str(error), result)
 
     @gen_test
@@ -103,7 +103,7 @@
         # The validation is executed in a separate process.
         deployer = self.make_deployer()
         with self.patch_validate() as mock_validate:
-            yield deployer.validate(self.user, 'bundle', self.bundle)
+            yield deployer.validate(self.user, self.bundle)
         mock_validate.assert_called_once_with(
             self.apiurl, self.user.username, self.user.password, self.bundle)
         mock_validate.assert_called_in_a_separate_process()
@@ -112,7 +112,7 @@
     def test_unsupported_api_version(self):
         # An error message is returned the API version is not supported.
         deployer = self.make_deployer(apiversion='not-supported')
-        result = yield deployer.validate(self.user, 'bundle', self.bundle)
+        result = yield deployer.validate(self.user, self.bundle)
         self.assertEqual('unsupported API version: not-supported', result)
 
     def test_import_bundle_scheduling(self):
@@ -422,7 +422,7 @@
         self.deployment = base.DeployMiddleware(
             self.user, self.deployer, self.responses.append)
 
-    def test_deployment_requested(self):
+    def test_deployment_requested_v3(self):
         # True is returned if the incoming data is a deployment request.
         requests = (
             self.make_deployment_request('Import'),
@@ -434,7 +434,19 @@
             requested = self.deployment.requested(request)
             self.assertTrue(requested, request)
 
-    def test_deployment_not_requested(self):
+    def test_deployment_requested_v4(self):
+        # True is returned if the incoming data is a deployment request.
+        requests = (
+            self.make_deployment_request('Import', version=4),
+            self.make_deployment_request('Watch', version=4),
+            self.make_deployment_request('Next', version=4),
+            self.make_deployment_request('Status', version=4),
+        )
+        for request in requests:
+            requested = self.deployment.requested(request)
+            self.assertTrue(requested, request)
+
+    def test_deployment_not_requested_v3(self):
         # False is returned if the incoming data is not a deployment request.
         # Params are not validated by DeployMiddleware.requested.
         params = {'Name': 'mybundle', 'YAML': 'foo: bar'}
@@ -474,8 +486,48 @@
             requested = self.deployment.requested(request)
             self.assertFalse(requested, request)
 
+    def test_deployment_not_requested_v4(self):
+        # False is returned if the incoming data is not a deployment request.
+        # Params are not validated by DeployMiddleware.requested.
+        params = {'Name': 'mybundle', 'YAML': 'foo: bar', 'Version': 4}
+        requests = (
+            # Empty request.
+            {},
+            # Invalid type field.
+            {
+                'RequestId': 1,
+                'Type': 'INVALID',
+                'Request': 'Import',
+                'Params': params,
+            },
+            # Invalid request field.
+            {
+                'RequestId': 2,
+                'Type': 'Deployer',
+                'Request': 'INVALID',
+                'Params': params,
+            },
+            # Missing request id field.
+            {
+                'INVALID': 3,
+                'Type': 'Deployer',
+                'Request': 'Import',
+                'Params': params,
+            },
+            # Field names are case sensitive.
+            {
+                'RequestId': 4,
+                'type': 'Deployer',
+                'request': 'Import',
+                'Params': params,
+            },
+        )
+        for request in requests:
+            requested = self.deployment.requested(request)
+            self.assertFalse(requested, request)
+
     @gen_test
-    def test_process_request(self):
+    def test_process_request_v3(self):
         # A deployment request is correctly processed.
         deployment_request = self.make_deployment_request('Import')
 
@@ -495,3 +547,25 @@
         self.assertEqual(1, len(self.responses))
         response = self.responses[0]
         self.assertEqual({'RequestId': 42, 'Response': 'ok'}, response)
+
+    @gen_test
+    def test_process_request_v4(self):
+        # A deployment request is correctly processed.
+        deployment_request = self.make_deployment_request('Import', version=4)
+
+        @gen.coroutine
+        def view(request, deployer):
+            # Ensure the view is called with the expected arguments.
+            self.assertEqual(deployment_request['Params'], request.params)
+            self.assertIs(self.user, request.user)
+            self.assertIs(self.deployer, deployer)
+            return {'Response': 'ok'}
+
+        # Patch the routes so that the customized view defined above is called
+        # when an import request is processed.
+        self.deployment.routes['Import'] = view
+        yield self.deployment.process_request(deployment_request)
+        # Ensure the response has been correctly sent.
+        self.assertEqual(1, len(self.responses))
+        response = self.responses[0]
+        self.assertEqual({'RequestId': 42, 'Response': 'ok'}, response)

=== modified file 'server/guiserver/tests/bundles/test_views.py'
--- server/guiserver/tests/bundles/test_views.py	2013-11-19 15:57:08 +0000
+++ server/guiserver/tests/bundles/test_views.py	2015-02-17 17:42:45 +0000
@@ -84,7 +84,7 @@
         self.assertEqual(0, len(self.deployer.mock_calls))
 
 
-class TestImportBundle(
+class TestImportBundleV3(
         ViewsTestMixin, helpers.BundlesTestMixin, LogTrapTestCase,
         AsyncTestCase):
 
@@ -189,7 +189,7 @@
         self.assertEqual(expected_response, response)
         # The Deployer validate method has been called.
         self.deployer.validate.assert_called_once_with(
-            request.user, 'mybundle', {'services': {}})
+            request.user, {'services': {}})
 
     @gen_test
     def test_success(self):
@@ -204,7 +204,7 @@
         expected_response = {'Response': {'DeploymentId': 42}}
         self.assertEqual(expected_response, response)
         # Ensure the Deployer methods have been correctly called.
-        args = (request.user, 'mybundle', {'services': {}})
+        args = (request.user, {'services': {}})
         self.deployer.validate.assert_called_once_with(*args)
         args = (request.user, 'mybundle', {'services': {}}, None)
         self.deployer.import_bundle.assert_called_once_with(*args)
@@ -258,13 +258,149 @@
         # Execute the view.
         yield self.view(request, self.deployer)
         # Ensure the Deployer methods have been correctly called.
-        args = (request.user, 'mybundle', {'services': {}})
+        args = (request.user, {'services': {}})
         self.deployer.validate.assert_called_once_with(*args)
         args = (request.user, 'mybundle', {'services': {}},
                 '~jorge/wiki/3/smallwiki')
         self.deployer.import_bundle.assert_called_once_with(*args)
 
 
+class TestImportBundleV4(
+        ViewsTestMixin, helpers.BundlesTestMixin, LogTrapTestCase,
+        AsyncTestCase):
+
+    def get_view(self):
+        return views.import_bundle
+
+    @gen_test
+    def test_invalid_yaml(self):
+        # An error response is returned if an invalid YAML encoded string is
+        # passed.
+        params = {'Name': 'bundle-name', 'Version': 4, 'YAML': 42}
+        request = self.make_view_request(params=params)
+        response = yield self.view(request, self.deployer)
+        expected_response = {
+            'Response': {},
+            'Error': 'invalid request: invalid YAML contents: '
+                     "'int' object has no attribute 'read'",
+        }
+        self.assertEqual(expected_response, response)
+        # The Deployer methods have not been called.
+        self.assertEqual(0, len(self.deployer.mock_calls))
+
+    @gen_test
+    def test_invalid_bundle(self):
+        # An error response is returned if the bundle is not well formed.
+        params = {'YAML': 'not valid', 'Version': 4, 'BundleID': 'foo'}
+        request = self.make_view_request(params=params)
+        response = yield self.view(request, self.deployer)
+        expected_response = {
+            'Response': {},
+            'Error': 'invalid request: invalid bundle foo: '
+                     'the bundle data is not well formed',
+        }
+        self.assertEqual(expected_response, response)
+        # The Deployer methods have not been called.
+        self.assertEqual(0, len(self.deployer.mock_calls))
+
+    @gen_test
+    def test_invalid_bundle_constraints(self):
+        # An error response is returned if the bundle includes services with
+        # unsupported constraints.
+        params = {
+            'YAML': 'services: {django: {constraints: invalid=1}}',
+            'Version': 4,
+            'BundleID': 'foo'
+        }
+        request = self.make_view_request(params=params)
+        response = yield self.view(request, self.deployer)
+        expected_response = {
+            'Response': {},
+            'Error': 'invalid request: invalid bundle foo: '
+                     'unsupported constraints: invalid',
+        }
+        self.assertEqual(expected_response, response)
+        # The Deployer methods have not been called.
+        self.assertEqual(0, len(self.deployer.mock_calls))
+
+    @gen_test
+    def test_undeployable_bundle(self):
+        # An error response is returned if the bundle cannot be imported in the
+        # current Juju environment.
+        params = {'Version': 4, 'YAML': 'services: {}'}
+        request = self.make_view_request(params=params)
+        # Simulate an error returned by the Deployer validate method.
+        self.deployer.validate.return_value = self.make_future('an error')
+        # Execute the view.
+        response = yield self.view(request, self.deployer)
+        expected_response = {
+            'Response': {},
+            'Error': 'invalid request: an error',
+        }
+        self.assertEqual(expected_response, response)
+        # The Deployer validate method has been called.
+        self.deployer.validate.assert_called_once_with(
+            request.user, {'services': {}})
+
+    @gen_test
+    def test_success(self):
+        # The response includes the deployment identifier.
+        params = {'BundleID': 'foo', 'Version': 4, 'YAML': 'services: {}'}
+        request = self.make_view_request(params=params)
+        # Set up the Deployer mock.
+        self.deployer.validate.return_value = self.make_future(None)
+        self.deployer.import_bundle.return_value = 42
+        # Execute the view.
+        response = yield self.view(request, self.deployer)
+        expected_response = {'Response': {'DeploymentId': 42}}
+        self.assertEqual(expected_response, response)
+        # Ensure the Deployer methods have been correctly called.
+        args = (request.user, {'services': {}})
+        self.deployer.validate.assert_called_once_with(*args)
+        args = (request.user, 'foo', {'services': {}}, 'foo')
+        self.deployer.import_bundle.assert_called_once_with(*args)
+
+    @gen_test
+    def test_logging(self):
+        # The beginning of the bundle import process is properly logged.
+        params = {'BundleID': 'foo', 'Version': 4, 'YAML': 'services: {}'}
+        request = self.make_view_request(params=params)
+        # Set up the Deployer mock.
+        self.deployer.validate.return_value = self.make_future(None)
+        self.deployer.import_bundle.return_value = 42
+        # Execute the view.
+        expected_log = "import_bundle: scheduling 'foo' deployment"
+        with ExpectLog('', expected_log, required=True):
+            yield self.view(request, self.deployer)
+
+    # The following tests exercise views._validate_import_params directly.
+    def test_id_provided(self):
+        params = {'YAML': 'services: {}',
+                  'Version': 4,
+                  'BundleID': '~jorge/wiki'}
+        results = views._validate_import_params(params)
+        expected = ('~jorge/wiki', {'services': {}}, '~jorge/wiki')
+        self.assertEqual(expected, results)
+
+    @gen_test
+    def test_id_passed_to_deployer(self):
+        params = {'YAML': 'services: {}',
+                  'Version': 4,
+                  'BundleID': '~jorge/wiki/3/smallwiki'}
+        request = self.make_view_request(params=params)
+        # Set up the Deployer mock.
+        self.deployer.validate.return_value = self.make_future(None)
+        self.deployer.import_bundle.return_value = 42
+        # Execute the view.
+        yield self.view(request, self.deployer)
+        # Ensure the Deployer methods have been correctly called.
+        args = (request.user, {'services': {}})
+        self.deployer.validate.assert_called_once_with(*args)
+        args = (request.user, '~jorge/wiki/3/smallwiki', {'services': {}},
+                '~jorge/wiki/3/smallwiki')
+        self.deployer.import_bundle.assert_called_once_with(*args)
+
+
 class TestWatch(
         ViewsTestMixin, helpers.BundlesTestMixin, LogTrapTestCase,
         AsyncTestCase):

=== modified file 'server/guiserver/tests/helpers.py'
--- server/guiserver/tests/helpers.py	2014-01-28 15:48:59 +0000
+++ server/guiserver/tests/helpers.py	2015-02-17 17:42:45 +0000
@@ -143,19 +143,26 @@
         return mock.Mock(params=params, user=user)
 
     def make_deployment_request(
-            self, request, request_id=42, params=None, encoded=False):
+            self, request, request_id=42, params=None, encoded=False,
+            version=None):
         """Create and return a deployment request message.
 
         If encoded is set to True, the returned message will be JSON encoded.
         """
+        if version is None:
+            bundle = 'bundle: {services: {}}'
+        else:
+            bundle = 'services: {}'
         defaults = {
-            'Import': {'Name': 'bundle', 'YAML': 'bundle: {services: {}}'},
+            'Import': {'Name': 'bundle', 'YAML': bundle},
             'Watch': {'DeploymentId': 0},
             'Next': {'WatcherId': 0},
             'Status': {},
         }
         if params is None:
             params = defaults[request]
+        if version is not None:
+            params['Version'] = version
         data = {
             'RequestId': request_id,
             'Type': 'Deployer',