Merge lp:~bac/charms/precise/juju-gui/constraint-parsing-deux into lp:~juju-gui/charms/precise/juju-gui/trunk

[Brad Crittenden]

Constraint parsing from charmworldlib.

The constraint parsing code has been moved to charmworldlib so it has been
deleted here.

Add dependency on charmworldlib and include it in deps.

https://codereview.appspot.com/71230043/

[Brad Crittenden (bac)]

Reviewers: mp+209327_code.launchpad.net,

Message:
Please take a look.

Description:
Constraint parsing from charmworldlib.

The constraint parsing code has been moved to charmworldlib so it has
been
deleted here.

Add dependency on charmworldlib and include it in deps.

https://code.launchpad.net/~bac/charms/precise/juju-gui/constraint-parsing-deux/+merge/209327

(do not edit description out of merge proposal)

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

Affected files (+32, -102 lines):
   M HACKING.md
   A [revision details]
   A deps/charmworldlib-0.3.0.tar.gz
   M server-requirements.pip
   M server/guiserver/bundles/utils.py
   M server/guiserver/tests/bundles/test_utils.py
   M tests/20-functional.test

[Francesco Banconi (frankban)]

Nice branch Brad, thank you!
LGTM with minor changes.

https://codereview.appspot.com/71230043/diff/1/HACKING.md
File HACKING.md (right):

https://codereview.appspot.com/71230043/diff/1/HACKING.md#newcode72
HACKING.md:72: Note that the test will not work using an LXC
environment. The test
Thank you for this! To be more explicit, we could say "Note that
functional tests will not work...".

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils.py
File server/guiserver/bundles/utils.py (left):

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils.py#oldcode40
server/guiserver/bundles/utils.py:40: # Define a sequence of allowed
constraints to be used in the process of
I am very happy that now we have a shared lib at least for this
constraints stuff.

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils.py
File server/guiserver/bundles/utils.py (right):

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils.py#newcode34
server/guiserver/bundles/utils.py:34: from charmworldlib.utils import
parse_constraints
Please keep those in alphabetical order.

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils.py#newcode160
server/guiserver/bundles/utils.py:160: - the bundle includes unsupported
constraints.
I see that parse_constraints raises a ValueError if constraints are not
valid. Maybe it's worth mentioning here too, or below, e.g.
- the bundle includes unsupported or invalid constraints.

We also might want to mention somewhere (perhaps in the requirements
file) that when updating the charmworldlib tgz, we should ensure
parse_constraints still only raises ValueErrors.

https://codereview.appspot.com/71230043/

[lp:~bac/charms/precise/juju-gui/constraint-parsing-deux]

176. By Brad Crittenden on 2014-03-05

Documentation changes per review.

[Brad Crittenden (bac)]

*** Submitted:

Constraint parsing from charmworldlib.

The constraint parsing code has been moved to charmworldlib so it has
been
deleted here.

Add dependency on charmworldlib and include it in deps.

R=frankban
CC=
https://codereview.appspot.com/71230043

https://codereview.appspot.com/71230043/diff/1/HACKING.md
File HACKING.md (right):

https://codereview.appspot.com/71230043/diff/1/HACKING.md#newcode72
HACKING.md:72: Note that the test will not work using an LXC
environment. The test
On 2014/03/05 13:07:54, frankban wrote:
> Thank you for this! To be more explicit, we could say "Note that
functional
> tests will not work...".

Done.

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils.py
File server/guiserver/bundles/utils.py (left):

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils.py#oldcode40
server/guiserver/bundles/utils.py:40: # Define a sequence of allowed
constraints to be used in the process of
On 2014/03/05 13:07:54, frankban wrote:
> I am very happy that now we have a shared lib at least for this
constraints
> stuff.

Done.

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils.py
File server/guiserver/bundles/utils.py (right):

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils.py#newcode34
server/guiserver/bundles/utils.py:34: from charmworldlib.utils import
parse_constraints
On 2014/03/05 13:07:54, frankban wrote:
> Please keep those in alphabetical order.

Done.

https://codereview.appspot.com/71230043/diff/1/server/guiserver/bundles/utils.py#newcode160
server/guiserver/bundles/utils.py:160: - the bundle includes unsupported
constraints.
On 2014/03/05 13:07:54, frankban wrote:
> I see that parse_constraints raises a ValueError if constraints are
not valid.
> Maybe it's worth mentioning here too, or below, e.g.
> - the bundle includes unsupported or invalid constraints.

> We also might want to mention somewhere (perhaps in the requirements
file) that
> when updating the charmworldlib tgz, we should ensure
parse_constraints still
> only raises ValueErrors.

I made the first change. The reminder seems like a good idea but would
not scale. I mean we have lots of dependencies so making notes about
double checking exceptions seems unworkable.

https://codereview.appspot.com/71230043/

=== modified file 'HACKING.md'
--- HACKING.md	2013-11-19 16:31:00 +0000
+++ HACKING.md	2014-03-05 14:55:46 +0000
@@ -69,6 +69,10 @@
 your `~/.juju/environments.yaml`, that will be bootstrapped before running the
 tests and destroyed at the end of the test run.
 
+Note that the functional tests will not work using an LXC environment.  The
+test co-locates the juju-gui on the bootstrap node, which is not possible in
+LXC.
+
 Please read further for additional details.
 
 ### Unit Tests ###

=== added file 'deps/charmworldlib-0.3.0.tar.gz'
Binary files deps/charmworldlib-0.3.0.tar.gz	1970-01-01 00:00:00 +0000 and deps/charmworldlib-0.3.0.tar.gz	2014-03-05 14:55:46 +0000 differ
=== modified file 'server-requirements.pip'
--- server-requirements.pip	2014-02-20 13:08:59 +0000
+++ server-requirements.pip	2014-03-05 14:55:46 +0000
@@ -28,3 +28,4 @@
 websocket-client==0.12.0
 jujuclient==0.15
 juju-deployer==0.3.2
+charmworldlib==0.3.0

=== modified file 'server/guiserver/bundles/utils.py'
--- server/guiserver/bundles/utils.py	2014-02-12 19:11:20 +0000
+++ server/guiserver/bundles/utils.py	2014-03-05 14:55:46 +0000
@@ -29,6 +29,7 @@
 )
 from tornado.httpclient import AsyncHTTPClient
 
+from charmworldlib.utils import parse_constraints
 from guiserver.watchers import AsyncWatcher
 from jujuclient import EnvError
 
@@ -37,23 +38,6 @@
 STARTED = 'started'
 CANCELLED = 'cancelled'
 COMPLETED = 'completed'
-# Define a sequence of allowed constraints to be used in the process of
-# preparing the bundle object. See the _prepare_constraints function below.
-ALLOWED_CONSTRAINTS = (
-    'arch',
-    'container',
-    'cpu-cores',
-    'cpu-power',
-    'mem',
-    'root-disk',
-    # XXX: BradCrittenden 2014-02-12:
-    # tags are supported by MaaS only so they are not currently implemented.
-    # It is unclear whether the GUI should support them or not so they are
-    # being left out for now.
-    # Also, tags are a comma-separated, which would clash with the currently
-    # broken constraint parsing in the GUI.
-    # 'tags',
-)
 
 
 def create_change(deployment_id, status, queue=None, error=None):
@@ -162,28 +146,6 @@
         logging.info('deployment {} completed'.format(deployment_id))
 
 
-def _prepare_constraints(constraints):
-    """Validate and prepare the given service constraints.
-
-    If constraints are passed as a string, convert them to be a dict.
-
-    Return the validated constraints dict.
-    Raise a ValueError if unsupported constraints are present.
-    """
-    if not isinstance(constraints, collections.Mapping):
-        try:
-            constraints = dict(i.split('=') for i in constraints.split(','))
-        except ValueError:
-            # A ValueError is raised if constraints are invalid, e.g. "cpu=,".
-            raise ValueError('invalid constraints: {}'.format(constraints))
-    unsupported = set(constraints).difference(ALLOWED_CONSTRAINTS)
-    if unsupported:
-        msg = 'unsupported constraints: {}'.format(
-            ', '.join(sorted(unsupported)))
-        raise ValueError(msg)
-    return constraints
-
-
 def prepare_bundle(bundle):
     """Validate and prepare the bundle.
 
@@ -195,7 +157,7 @@
     Raise a ValueError if:
         - the bundle is not well structured;
         - the bundle does not include services;
-        - the bundle includes unsupported constraints.
+        - the bundle includes unsupported or invalid constraints.
     """
     # XXX frankban 2013-11-07: is the GUI Server in charge of validating the
     # bundles? For now, the weak checks below should be enough.
@@ -213,7 +175,7 @@
                 del service_data['constraints']
             else:
                 # Otherwise sanitize the value.
-                service_data['constraints'] = _prepare_constraints(constraints)
+                service_data['constraints'] = parse_constraints(constraints)
 
 
 def require_authenticated_user(view):

=== modified file 'server/guiserver/tests/bundles/test_utils.py'
--- server/guiserver/tests/bundles/test_utils.py	2014-02-12 13:55:47 +0000
+++ server/guiserver/tests/bundles/test_utils.py	2014-03-05 14:55:46 +0000
@@ -269,70 +269,30 @@
         self.assertTrue(watcher.closed)
 
 
-class TestPrepareConstraints(unittest.TestCase):
-
-    def test_valid_constraints(self):
-        # Valid constraints are returned as they are.
-        constraints = {
-            'arch': 'i386',
-            'cpu-cores': 4,
-            'cpu-power': 2,
-            'mem': 2000,
-            'root-disk': '1G',
-            'container': 'lxc',
-        }
-        self.assertEqual(constraints, utils._prepare_constraints(constraints))
-
-    def test_valid_constraints_subset(self):
-        # A subset of valid constraints is returned as it is.
-        constraints = {'cpu-cores': '4', 'cpu-power': 2}
-        self.assertEqual(constraints, utils._prepare_constraints(constraints))
-
-    def test_invalid_constraints(self):
-        # A ValueError is raised if unsupported constraints are found.
-        with self.assertRaises(ValueError) as context_manager:
-            utils._prepare_constraints({'arch': 'i386', 'not-valid': 'bang!'})
-        self.assertEqual(
-            'unsupported constraints: not-valid',
-            str(context_manager.exception))
-
-    def test_string_constraints(self):
-        # String constraints are converted to a dict.
-        constraints = 'arch=i386,cpu-cores=4,cpu-power=2,mem=2000'
-        expected = {
-            'arch': 'i386',
-            'cpu-cores': '4',
-            'cpu-power': '2',
-            'mem': '2000',
-        }
-        self.assertEqual(expected, utils._prepare_constraints(constraints))
-
-    def test_string_constraints_subset(self):
-        # A subset of string constraints is converted to a dict.
-        constraints = 'cpu-cores=4,mem=2000'
-        expected = {'cpu-cores': '4', 'mem': '2000'}
-        self.assertEqual(expected, utils._prepare_constraints(constraints))
-
-    def test_unsupported_string_constraints(self):
-        # A ValueError is raised if unsupported string constraints are found.
-        with self.assertRaises(ValueError) as context_manager:
-            utils._prepare_constraints('cpu-cores=4,invalid1=1,invalid2=2')
-        self.assertEqual(
-            'unsupported constraints: invalid1, invalid2',
-            str(context_manager.exception))
-
-    def test_invalid_string_constraints(self):
-        # A ValueError is raised if unsupported string constraints are found.
-        with self.assertRaises(ValueError) as context_manager:
-            utils._prepare_constraints('arch=,cpu-cores=,')
-        self.assertEqual(
-            'invalid constraints: arch=,cpu-cores=,',
-            str(context_manager.exception))
-
-
 class TestPrepareBundle(unittest.TestCase):
 
-    def test_constraints_conversion(self):
+    def test_constraints_conversion_space_separated(self):
+        # Service constraints stored as strings are converted to a dict.
+        bundle = {
+            'services': {
+                'django': {'constraints': 'arch=i386 cpu-cores=4 mem=2000'},
+            },
+        }
+        expected = {
+            'services': {
+                'django': {
+                    'constraints': {
+                        'arch': 'i386',
+                        'cpu-cores': '4',
+                        'mem': '2000',
+                    },
+                },
+            },
+        }
+        utils.prepare_bundle(bundle)
+        self.assertEqual(expected, bundle)
+
+    def test_constraints_conversion_comma_separated(self):
         # Service constraints stored as strings are converted to a dict.
         bundle = {
             'services': {

=== modified file 'tests/20-functional.test'
--- tests/20-functional.test	2014-01-16 20:32:55 +0000
+++ tests/20-functional.test	2014-03-05 14:55:46 +0000
@@ -86,6 +86,7 @@
         # Create a Selenium browser instance.
         selenium = self.selenium = Firefox()
         self.addCleanup(selenium.quit)
+        self.service_name = None
         super(DeployTestMixin, self).setUp()
 
     def assertEnvironmentIsConnected(self):
@@ -170,7 +171,8 @@
 class TestDeployOptions(DeployTestMixin, unittest.TestCase):
 
     def tearDown(self):
-        juju_destroy_service(self.service_name)
+        if self.service_name is not None:
+            juju_destroy_service(self.service_name)
 
     def test_stable_release(self):
         # Ensure the stable Juju GUI release is correctly set up.