Merge lp:~frankban/juju-quickstart/changeset-from-jujubundlelib into lp:juju-quickstart

[Francesco Banconi]

Juju bundlelib validation in quickstart.

Validate bundles using jujubundlelib.
Also add to the bundle model the ability
to return its change set.

https://codereview.appspot.com/232890043/

[Francesco Banconi (frankban)]

Reviewers: mp+257248_code.launchpad.net,

Message:
Please take a look.

Description:
Juju bundlelib validation in quickstart.

Validate bundles using jujubundlelib.
Also add to the bundle model the ability
to return its change set.

https://code.launchpad.net/~frankban/juju-quickstart/changeset-from-jujubundlelib/+merge/257248

(do not edit description out of merge proposal)

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

Affected files (+188, -61 lines):
   A [revision details]
   M quickstart/models/bundles.py
   M quickstart/tests/helpers.py
   M quickstart/tests/models/test_bundles.py
   M tox.ini

[lp:~frankban/juju-quickstart/changeset-from-jujubundlelib]

132. By Francesco Banconi on 2015-04-23

done done

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/232890043/

[Jeff Pihach (hatch)]

LGTM!

https://codereview.appspot.com/232890043/diff/20001/quickstart/models/bundles.py
File quickstart/models/bundles.py (right):

https://codereview.appspot.com/232890043/diff/20001/quickstart/models/bundles.py#newcode389
quickstart/models/bundles.py:389: if settings.JUJU_GUI_SERVICE_NAME in
data['services'].keys():
I'm wondering if we shouldn't be a little bit more proactive here by
throwing a warning to the user but then continuing on ignoring that they
supplied their own GUI instance. We could possibly even try to apply
their supplied GUI config.

https://codereview.appspot.com/232890043/

[Francesco Banconi (frankban)]

On 2015/04/23 14:02:41, jeff.pihach wrote:
> LGTM!

https://codereview.appspot.com/232890043/diff/20001/quickstart/models/bundles.py
> File quickstart/models/bundles.py (right):

https://codereview.appspot.com/232890043/diff/20001/quickstart/models/bundles.py#newcode389
> quickstart/models/bundles.py:389: if settings.JUJU_GUI_SERVICE_NAME in
> data['services'].keys():
> I'm wondering if we shouldn't be a little bit more proactive here by
throwing a
> warning to the user but then continuing on ignoring that they supplied
their own
> GUI instance. We could possibly even try to apply their supplied GUI
config.

Thanks for the review!
This is a good suggestion. Something like this can be implemented later,
but it's not trivial in the current approach which makes use of the
deployer in the backend.

https://codereview.appspot.com/232890043/

[Madison Scott-Clary (makyo)]

LGTM - great step forward

https://codereview.appspot.com/232890043/

[Francesco Banconi (frankban)]

Thanks for the reviews!

https://codereview.appspot.com/232890043/

[Francesco Banconi (frankban)]

*** Submitted:

Juju bundlelib validation in quickstart.

Validate bundles using jujubundlelib.
Also add to the bundle model the ability
to return its change set.

R=jeff.pihach, matthew.scott
CC=
https://codereview.appspot.com/232890043

https://codereview.appspot.com/232890043/

=== modified file 'quickstart/models/bundles.py'
--- quickstart/models/bundles.py	2015-04-21 10:22:46 +0000
+++ quickstart/models/bundles.py	2015-04-23 12:16:34 +0000
@@ -46,7 +46,11 @@
 import os
 import re
 
-from jujubundlelib import references
+from jujubundlelib import (
+    changeset,
+    references,
+    validation,
+)
 
 from quickstart import (
     charmstore,
@@ -83,6 +87,13 @@
     def __repr__(self):
         return b'<Bundle: {}>'.format(bytes(self))
 
+    def get_changeset(self):
+        """Return the collections of changes required to deploy this bundle.
+
+        The change set is returned as a generator object.
+        """
+        return changeset.parse(self.data)
+
     def serialize(self):
         """Serialize the bundle data as a YAML encoded string."""
         return serializers.yaml_dump(self.data)
@@ -277,12 +288,6 @@
 def parse_yaml(content):
     """Parse and validate the given bundle content as a YAML encoded string.
 
-    Note that the bundle validation performed by Juju Quickstart is weak by
-    design: it just checks that the content looks like a bundle YAML. Contents
-    provided by the charm store are already known as valid. For other sources,
-    a more cogent validation is done down in the stack, when the content is
-    sent to the GUI server and then to the Juju deployer.
-
     Return the resulting YAML decoded dictionary.
     Raise a ValueError if:
         - the bundle YAML contents are not parsable by YAML;
@@ -372,29 +377,16 @@
 def validate(data):
     """Validate the given YAML decoded bundle data.
 
-    Note that the bundle validation performed by Juju Quickstart is weak by
-    design: it just checks that the content looks like a bundle YAML. Contents
-    provided by the charm store are already known as valid. For other sources,
-    a more cogent validation is done down in the stack, when the content is
-    sent to the GUI server and then to the Juju deployer.
-
     Raise a ValueError if:
         - the YAML contents are not properly structured;
         - the bundle does not include services.
     """
-    _ensure_is_dict(data)
-    # Retrieve the bundle services.
-    try:
-        services = data['services'].keys()
-    except (AttributeError, KeyError, TypeError):
-        content = serializers.yaml_dump(data).strip()
-        msg = 'unable to retrieve bundle services: {}'.format(content)
+    errors = validation.validate(data)
+    if errors:
+        msg = 'invalid bundle data:\n  {}'.format('\n  '.join(errors))
         raise ValueError(msg.encode('utf-8'))
-    # Ensure at least one service is defined in the bundle.
-    if not services:
-        raise ValueError(b'no services found in the bundle')
-    # Check that the Juju GUI charm is not included as a service.
-    if settings.JUJU_GUI_SERVICE_NAME in services:
+    # Check that the Juju GUI charm is not included in the bundle as a service.
+    if settings.JUJU_GUI_SERVICE_NAME in data['services'].keys():
         raise ValueError(
             b'the provided bundle contains an instance of juju-gui. Juju '
             b'Quickstart will install the latest version of the Juju GUI '

=== modified file 'quickstart/tests/helpers.py'
--- quickstart/tests/helpers.py	2015-03-09 18:20:47 +0000
+++ quickstart/tests/helpers.py	2015-04-23 12:16:34 +0000
@@ -18,7 +18,9 @@
 
 from __future__ import unicode_literals
 
+import collections
 from contextlib import contextmanager
+import copy
 import os
 import shutil
 import socket
@@ -47,11 +49,33 @@
 class BundleFileTestsMixin(object):
     """Shared methods for testing Juju bundle files."""
 
-    bundle_data = {'services': {'wordpress': {}, 'mysql': {}}}
+    bundle_data = {
+        'services': {
+            'wordpress': {
+                'charm': 'cs:trusty/wordpress-42',
+                'num_units': 1,
+            },
+            'mysql': {
+                'charm': 'cs:vivid/mysql-47',
+                'num_units': 0,
+            },
+        },
+    }
     bundle_content = yaml.safe_dump(bundle_data)
     legacy_bundle_data = {
-        'bundle1': {'services': {'wordpress': {}, 'mysql': {}}},
-        'bundle2': {'services': {'django': {}, 'nodejs': {}}},
+        'bundle1': bundle_data,
+        'bundle2': {
+            'services': {
+                'django': {
+                    'charm': 'cs:vivid/django-42',
+                    'num_units': 2,
+                },
+                'nodejs': {
+                    'charm': 'nodejs',
+                    'num_units': 1,
+                },
+            },
+        },
     }
     legacy_bundle_content = yaml.safe_dump(legacy_bundle_data)
 
@@ -269,6 +293,21 @@
     return {'environments': environments}
 
 
+def make_ordered_bundle(bundle):
+    """Given a bundle, return a new one with the same data sorted.
+
+    The provided bundle must be an an instance of
+    "quickstart.models.bundles.Bundle".
+    The resulting bundle can be used to test its corresponding change set in a
+    deterministic way.
+    """
+    data = copy.deepcopy(bundle.data)
+    for key, value in data.items():
+        if isinstance(value, collections.Mapping):
+            data[key] = collections.OrderedDict(sorted(value.items()))
+    return bundle.__class__(data, reference=bundle.reference)
+
+
 # Mock the builtin print function.
 mock_print = mock.patch('__builtin__.print')
 

=== modified file 'quickstart/tests/models/test_bundles.py'
--- quickstart/tests/models/test_bundles.py	2015-04-21 10:22:46 +0000
+++ quickstart/tests/models/test_bundles.py	2015-04-23 12:16:34 +0000
@@ -61,17 +61,58 @@
         bundle = bundles.Bundle(self.bundle_data)
         self.assertEqual('<Bundle: bundle>', repr(bundle))
 
+    def test_get_changeset(self):
+        # The collection of changes corresponding to the bundle is correctly
+        # returned.
+        expected_changeset = (
+            {'id': 'addCharm-0',
+             'method': 'addCharm',
+             'args': ['cs:vivid/mysql-47'],
+             'requires': []},
+            {'id': 'addService-1',
+             'method': 'deploy',
+             'args': ['cs:vivid/mysql-47', 'mysql', {}],
+             'requires': ['addCharm-0']},
+            {'id': 'addCharm-2',
+             'method': 'addCharm',
+             'args': ['cs:trusty/wordpress-42'],
+             'requires': []},
+            {'id': 'addService-3',
+             'method': 'deploy',
+             'args': ['cs:trusty/wordpress-42', 'wordpress', {}],
+             'requires': ['addCharm-2']},
+            {'id': 'addUnit-4',
+             'method': 'addUnit',
+             'args': ['$addService-3', 1, None],
+             'requires': ['addService-3']},
+        )
+        bundle = helpers.make_ordered_bundle(self.bundle)
+        self.assertEqual(expected_changeset, tuple(bundle.get_changeset()))
+
     def test_serialization(self):
         # The bundle data is correctly serialized into a YAML encoded string.
-        content = self.bundle.serialize()
-        self.assertEqual('services:\n  mysql: {}\n  wordpress: {}\n', content)
+        expected_content = (
+            'services:\n'
+            '  mysql:\n'
+            '    charm: cs:vivid/mysql-47\n'
+            '    num_units: 0\n'
+            '  wordpress:\n'
+            '    charm: cs:trusty/wordpress-42\n'
+            '    num_units: 1\n')
+        self.assertEqual(expected_content, self.bundle.serialize())
 
     def test_legacy_serialization(self):
         # The bundle data can be serialized for legacy API version 3.
-        content = self.bundle.serialize_legacy()
-        self.assertEqual(
-            'bundle:\n  services:\n    mysql: {}\n    wordpress: {}\n',
-            content)
+        expected_content = (
+            'bundle:\n'
+            '  services:\n'
+            '    mysql:\n'
+            '      charm: cs:vivid/mysql-47\n'
+            '      num_units: 0\n'
+            '    wordpress:\n'
+            '      charm: cs:trusty/wordpress-42\n'
+            '      num_units: 1\n')
+        self.assertEqual(expected_content, self.bundle.serialize_legacy())
 
     def test_services(self):
         # Bundle services can be easily retrieved.
@@ -167,8 +208,10 @@
     def test_charmworld_bundle_invalid_content(self):
         # A ValueError is raised if the content associated with the given
         # charmworld URL is not valid.
+        expected_error = (
+            'invalid bundle data:\n  bundle does not appear to be a bundle')
         with self.patch_urlread(contents='exterminate!'):
-            with self.assert_value_error('invalid YAML content: exterminate!'):
+            with self.assert_value_error(expected_error):
                 bundles.from_source('bundle:mediawiki/single')
 
     def test_charmworld_bundle_connection_error(self):
@@ -307,8 +350,10 @@
     def test_jujucharms_bundle_invalid_content(self):
         # A ValueError is raised if the content associated with the given
         # jujucharms.com URL are not valid.
+        expected_error = (
+            'invalid bundle data:\n  bundle does not appear to be a bundle')
         with self.patch_urlread(contents='exterminate!'):
-            with self.assert_value_error('invalid YAML content: exterminate!'):
+            with self.assert_value_error(expected_error):
                 bundles.from_source('wordpress-scalable')
 
     def test_jujucharms_bundle_connection_error(self):
@@ -348,7 +393,12 @@
         # a legacy version 3 bundle.
         # In this case, the resulting bundle does not have a reference.
         legacy_bundle_data = {
-            'bundle': {'services': {'wordpress': {}, 'mysql': {}}},
+            'bundle': {
+                'services': {
+                    'wordpress': {'charm': 'wordpress', 'num_units': 1},
+                    'mysql': {'charm': 'mysql', 'num_units': 1},
+                }
+            },
         }
         path = self.make_bundle_file(legacy_bundle_data)
         bundle = bundles.from_source(path)
@@ -407,7 +457,8 @@
         # A ValueError is raised if a local file contains an invalid legacy
         # version 3 content.
         path = self.make_bundle_file({'bundle': '42'})
-        expected_error = "invalid YAML content: 42"
+        expected_error = (
+            'invalid bundle data:\n  bundle does not appear to be a bundle')
         with self.assert_value_error(expected_error):
             bundles.from_source(path, 'bundle')
 
@@ -503,46 +554,62 @@
 
     def test_yaml_invalid_type(self):
         # A ValueError is raised if the bundle content is not well formed.
-        with self.assert_value_error('invalid YAML content: a-string'):
+        expected_error = (
+            'invalid bundle data:\n  bundle does not appear to be a bundle')
+        with self.assert_value_error(expected_error):
             bundles.parse_yaml('a-string')
 
     def test_yaml_invalid_bundle_data(self):
         # A ValueError is raised if the bundle content is not well formed.
         contents = yaml.safe_dump('not valid')
-        with self.assert_value_error('invalid YAML content: not valid'):
+        expected_error = (
+            'invalid bundle data:\n  bundle does not appear to be a bundle')
+        with self.assert_value_error(expected_error):
             bundles.parse_yaml(contents)
 
     def test_yaml_no_services(self):
         # A ValueError is raised if the bundle does not include services.
         contents = yaml.safe_dump({})
-        with self.assert_value_error('unable to retrieve bundle services: {}'):
+        expected_error = (
+            'invalid bundle data:\n  bundle does not define any services')
+        with self.assert_value_error(expected_error):
             bundles.parse_yaml(contents)
 
     def test_yaml_none_bundle_services(self):
         # A ValueError is raised if services are None.
         contents = yaml.safe_dump({'services': None})
-        expected = 'unable to retrieve bundle services: services: null'
-        with self.assert_value_error(expected):
+        expected_error = (
+            'invalid bundle data:\n  bundle does not define any services')
+        with self.assert_value_error(expected_error):
             bundles.parse_yaml(contents)
 
     def test_yaml_invalid_bundle_services_type(self):
         # A ValueError is raised if services have an invalid type.
         contents = yaml.safe_dump({'services': 42})
-        expected = 'unable to retrieve bundle services: services: 42'
-        with self.assert_value_error(expected):
+        expected_error = (
+            'invalid bundle data:\n'
+            '  services spec does not appear to be well-formed')
+        with self.assert_value_error(expected_error):
             bundles.parse_yaml(contents)
 
     def test_no_services(self):
         # A ValueError is raised if the specified bundle does not contain
         # services.
         contents = yaml.safe_dump({'services': {}})
-        with self.assert_value_error('no services found in the bundle'):
+        expected_error = (
+            'invalid bundle data:\n  bundle does not define any services')
+        with self.assert_value_error(expected_error):
             bundles.parse_yaml(contents)
 
     def test_yaml_gui_in_services(self):
         # A ValueError is raised if the bundle contains juju-gui.
         contents = yaml.safe_dump({
-            'services': {settings.JUJU_GUI_SERVICE_NAME: {}},
+            'services': {
+                settings.JUJU_GUI_SERVICE_NAME: {
+                    'charm': 'juju-gui',
+                    'num_units': 1,
+                },
+            },
         })
         expected_error = (
             'the provided bundle contains an instance of juju-gui. Juju '
@@ -586,25 +653,32 @@
 
     def test_yaml_no_services(self):
         # A ValueError is raised if the bundle does not include services.
-        with self.assert_value_error('unable to retrieve bundle services: {}'):
+        expected_error = (
+            'invalid bundle data:\n  bundle does not define any services')
+        with self.assert_value_error(expected_error):
             bundles.validate({})
 
     def test_yaml_none_bundle_services(self):
         # A ValueError is raised if services are None.
-        expected = 'unable to retrieve bundle services: services: null'
-        with self.assert_value_error(expected):
+        expected_error = (
+            'invalid bundle data:\n  bundle does not define any services')
+        with self.assert_value_error(expected_error):
             bundles.validate({'services': None})
 
     def test_yaml_invalid_bundle_services_type(self):
         # A ValueError is raised if services have an invalid type.
-        expected = 'unable to retrieve bundle services: services: 42'
-        with self.assert_value_error(expected):
+        expected_error = (
+            'invalid bundle data:\n'
+            '  services spec does not appear to be well-formed')
+        with self.assert_value_error(expected_error):
             bundles.validate({'services': 42})
 
     def test_no_services(self):
         # A ValueError is raised if the specified bundle does not contain
         # services.
-        with self.assert_value_error('no services found in the bundle'):
+        expected_error = (
+            'invalid bundle data:\n  bundle does not define any services')
+        with self.assert_value_error(expected_error):
             bundles.validate({'services': {}})
 
     def test_yaml_gui_in_services(self):
@@ -613,10 +687,30 @@
             'the provided bundle contains an instance of juju-gui. Juju '
             'Quickstart will install the latest version of the Juju GUI '
             'automatically; please remove juju-gui from the bundle')
-        with self.assert_value_error(expected_error):
-            bundles.validate({
-                'services': {settings.JUJU_GUI_SERVICE_NAME: {}},
-            })
+        data = {
+            'services': {
+                settings.JUJU_GUI_SERVICE_NAME: {
+                    'charm': 'juju-gui',
+                    'num_units': 1,
+                },
+            },
+        }
+        with self.assert_value_error(expected_error):
+            bundles.validate(data)
+
+    def test_multiple_errors(self):
+        # A ValueError with multiple bundle errors is properly reported.
+        expected_error = (
+            'invalid bundle data:\n'
+            '  no charm specified for service django\n'
+            '  num_units for service django must be an integer\n'
+            '  machine 1 not referred to by a placement directive')
+        data = {
+            'services': {'django': {}},
+            'machines': {'1': {}},
+        }
+        with self.assert_value_error(expected_error):
+            bundles.validate(data)
 
     def test_success(self):
         # The function succeeds when a valid bundle content is provided.

=== modified file 'tox.ini'
--- tox.ini	2015-04-21 10:13:12 +0000
+++ tox.ini	2015-04-23 12:16:34 +0000
@@ -72,7 +72,7 @@
     # See https://launchpad.net/~juju/+archive/ubuntu/stable.
     websocket-client==0.18.0
     jujuclient==0.50.1
-    jujubundlelib==0.1.1
+    jujubundlelib==0.1.3
     urwid==1.2.1
     # The distribution PyYAML requirement is used in this case.
 
@@ -82,7 +82,7 @@
     # Ubuntu 14.04 (trusty) distro dependencies.
     websocket-client==0.12.0
     jujuclient==0.17.5
-    jujubundlelib==0.1.1
+    jujubundlelib==0.1.3
     PyYAML==3.10
     urwid==1.1.1
 
@@ -92,7 +92,7 @@
     # Ubuntu 14.10 (utopic) distro dependencies.
     websocket-client==0.12.0
     jujuclient==0.17.5
-    jujubundlelib==0.1.1
+    jujubundlelib==0.1.3
     PyYAML==3.11
     urwid==1.2.1
 
@@ -102,7 +102,7 @@
     # Ubuntu 15.04 (vivid) distro dependencies.
     websocket-client==0.18.0
     jujuclient==0.18.5
-    jujubundlelib==0.1.1
+    jujubundlelib==0.1.3
     PyYAML==3.11
     urwid==1.2.1