Merge lp:~rharding/python-jujuclient/adjust_constraint_handling into lp:~hazmat/python-jujuclient/trunk

[Richard Harding]

Adjust constraint handling to drop unknown ones.

- Adjust tests and test runner along with README updates.

Adjust constraint handling to drop unknown ones.

- Setup a whitelist for constraints for now until we get better messaging from the client through the deployer setup.
- Update the parse_constraints helper to drop constraints that aren't valid and perform the type casting as it was.
- Update with a new tests directory and update setup.py to be able to run python setup.py test
- Update README with some beginner hacking information
- Move the function tests to a different filename and +x it so that nose would skip it during normal test runs. It's meant to be run manually with an environment
- add a .bzrignore file

[Benji York (benji)]

Line 49 of the diff has the last two characters on the line transposed ("test.s").

Since we don't really use the type of the constraints unless it is "int", perhaps a boolean instead of "int"/"str" would be more direct.

On line 178 and line 212 of the diff you could use mock.assert_called_once_with(...) instead.

[lp:~rharding/python-jujuclient/adjust_constraint_handling]

20. By Richard Harding on 2013-11-06

Update per review, remember to +x the test_functional file

21. By Richard Harding on 2013-11-06

Update per review correctly this time

[Richard Harding (rharding)]

> Line 49 of the diff has the last two characters on the line transposed
> ("test.s").
>
> Since we don't really use the type of the constraints unless it is "int",
> perhaps a boolean instead of "int"/"str" would be more direct.
>
> On line 178 and line 212 of the diff you could use
> mock.assert_called_once_with(...) instead.

Thanks for the review and comments. Updated.

[Kapil Thangavelu (hazmat)]

Whitelisting and handling exceptions should be done at the application level not at the client level which is policy free. Ie this implementation already misses several supported constraints, and new ones come out all the time. It also silently strips invalid constraints with no reporting back.

review: Disapprove

[Kapil Thangavelu (hazmat)]

talked with rick a bit more about the reason for this on irc which is transient fix for gui. capturing for posterity.

<hazmat> rick_h_, is there any expectation or error handling for deploys in gui?
<rick_h_> hazmat: no, because it's async we fire the deploy request off and errors don't get back right now. At least in our debugging why come bundles were failing to work yesterday
<hazmat> rick_h_, that sounds like a separate / additional issue with the guiserver deployer middleware.
<rick_h_> hazmat: +1 the guiserver bit int he deployer needs to communicate errors back to the gui server in the charm
<rick_h_> hazmat: right now, in env/gui.py it calls deploy and walks away
<hazmat> rick_h_, and a common way to communicate errors is exceptions.. so three separate issues rich exception passthrough in deployer, and handle exceptions at gui server, and ... implement error handling in the gui for bundles
<rick_h_> hazmat: yes, that's the fix we want to do. To move forward today the band-aid fix came up. If it's not ok, then we'll do something else. However, trying to get to release today/tomorrow.
<hazmat> rick_h_, the closest place to gui that's reasonable to do a hack is the guiserver deployer middleware
<rick_h_> hazmat: rgr, will move the band-aid there for now then and we'll be fixing coms/deployer as a library as follow ups post-release

Unmerged revisions

21. By Richard Harding on 2013-11-06

Update per review correctly this time

20. By Richard Harding on 2013-11-06

Update per review, remember to +x the test_functional file

19. By Richard Harding on 2013-11-06

Adjust test running, add constraint checking

=== added file '.bzrignore'
--- .bzrignore	1970-01-01 00:00:00 +0000
+++ .bzrignore	2013-11-06 16:53:14 +0000
@@ -0,0 +1,16 @@
+./bin/
+./build/
+./data/
+./develop-eggs/
+./download-cache/
+./docs/_build
+./eggs/
+./include/
+./lib/
+./local/
+./node_modules/
+./parts/
+.coverage
+.noseids
+*.egg
+*.egg-info/

=== modified file 'README.rst'
--- README.rst	2013-07-12 13:50:05 +0000
+++ README.rst	2013-11-06 16:53:14 +0000
@@ -1,5 +1,5 @@
 Juju Client
------------
+============
 
 A simple synchronous python client for the juju-core/gojuju websocket api.
 
@@ -31,3 +31,23 @@
    for change_set in watcher:
        print change_set
 
+
+
+Hacking
+---------
+
+There are two sets of tests. The first are the main software tests in the
+``tests`` directory. These are run via the test command.
+
+::
+
+    python setup.py test
+
+Note: The above command will install test dependencies. Be aware that it's ok
+to get an initial download of packages the first time you run test.s
+
+
+There is also a functional set of tests in the ``test_functional.py`` file.
+These are run manually and require a working environment setup. They are
+executable so that the test runner will ignore then during normal test runs.
+Please check both sets of tests when verifying your changes are good to go.

=== modified file 'jujuclient.py'
--- jujuclient.py	2013-10-29 01:00:08 +0000
+++ jujuclient.py	2013-11-06 16:53:14 +0000
@@ -228,6 +228,13 @@
 
 class Environment(RPC):
 
+    constraint_whitelist = {
+        'arch': {'type': 'str'},
+        'cpu-cores': {'type': 'int'},
+        'cpu-power': {'type': 'int'},
+        'mem': {'type': 'int'},
+    }
+
     def __init__(self, endpoint, conn=None):
         self.endpoint = endpoint
         self._watches = []
@@ -326,10 +333,28 @@
         return r
 
     def _prepare_constraints(self, constraints):
-        for k in ['cpu-cores', 'cpu-power', 'mem']:
-            if constraints.get(k):
-                constraints[k] = int(constraints[k])
-        return constraints
+        """Help format constraints per the juju-core expectations
+
+        - Cast int values for constraints that must be of type int
+
+        XXX: Rick Harding 11/6/13
+        Remove constraints we don't expect. Follow ups should throw a proper
+        exception or return a proper error that users of the client can use
+        to provide feedback to end users.
+        """
+        clean_constraints = {}
+        for name, value in constraints.iteritems():
+            if name not in self.constraint_whitelist:
+                continue
+
+            # If it exists, check what type it needs to be so that it's
+            # accepted by the Go backend.
+            if self.constraint_whitelist[name]['type'] == 'int':
+                clean_constraints[name] = int(constraints[name])
+            else:
+                clean_constraints[name] = constraints[name]
+
+        return clean_constraints
 
     # Relations
     def add_relation(self, endpoint_a, endpoint_b):

=== modified file 'setup.py'
--- setup.py	2013-10-29 01:00:08 +0000
+++ setup.py	2013-11-06 16:53:14 +0000
@@ -6,12 +6,14 @@
 
 setup(
     name="jujuclient",
-    version="0.13",
+    version="0.14",
     description="A juju-core/gojuju simple synchronous python api client.",
     author="Kapil Thangavelu",
     author_email="[e-mail]",
-    url="http://juju.ubuntu.com",
+    url="https://launchpad.net/python-jujuclient",
     install_requires=["websocket-client"],
+    tests_require=["nose==1.3", "mock==1.0.1"],
+    test_suite="nose.collector",
     classifiers=[
         "Development Status :: 2 - Pre-Alpha",
         "Programming Language :: Python",

=== renamed file 'test_jujuclient.py' => 'test_functional.py' (properties changed: -x to +x)
=== added directory 'tests'
=== added file 'tests/__init__.py'
=== added file 'tests/test_environment.py'
--- tests/test_environment.py	1970-01-01 00:00:00 +0000
+++ tests/test_environment.py	2013-11-06 16:53:14 +0000
@@ -0,0 +1,76 @@
+from mock import patch
+from unittest import TestCase
+
+from jujuclient import Environment
+
+
+class TestEnvironment(TestCase):
+
+    def test_non_whitelist_constraints_drop(self):
+        """Any constraint not in the whitelist should be dropped."""
+        # The endpoint and connection are required for the Environment, but
+        # the constraints code doesn't deal with it. Just fake it till you
+        # make it.
+        endpoint = None
+        connection = {}
+        test_constraints = {
+            'cpu': '3',
+            'power': 10,
+            'cpu-power': 1000,
+            'mem': '8000',
+            'arch': 'i386',
+        }
+
+        called_with = {
+            "Type": "Client",
+            "Request": "ServiceDeploy",
+            "Params": {
+                "ServiceName": 'test-service',
+                "CharmURL": 'url',
+                "NumUnits": 1,
+                "Config": {},
+                "Constraints": {
+                    'arch': 'i386',
+                    'cpu-power': 1000,
+                    'mem': 8000
+                }
+            }
+        }
+
+        with patch('jujuclient.Environment._rpc') as mock_rpc:
+            env = Environment(endpoint, connection)
+            env.deploy('test-service', 'url', constraints=test_constraints)
+            mock_rpc.assert_called_once_with(called_with)
+
+    def test_constraints_cast_to_proper_type(self):
+        """Some constraints must be an int value for Go to accept them."""
+        endpoint = None
+        connection = {}
+        test_constraints = {
+            'cpu-cores': '4',
+            'cpu-power': '1000',
+            'mem': '8000',
+            'arch': 'i386',
+        }
+
+        called_with = {
+            "Type": "Client",
+            "Request": "ServiceDeploy",
+            "Params": {
+                "ServiceName": 'test-service',
+                "CharmURL": 'url',
+                "NumUnits": 1,
+                "Config": {},
+                "Constraints": {
+                    'arch': 'i386',
+                    'cpu-cores': 4,
+                    'cpu-power': 1000,
+                    'mem': 8000
+                }
+            }
+        }
+
+        with patch('jujuclient.Environment._rpc') as mock_rpc:
+            env = Environment(endpoint, connection)
+            env.deploy('test-service', 'url', constraints=test_constraints)
+            mock_rpc.assert_called_once_with(called_with)