Merge lp:~niedbalski/python-jujuclient/lp-1456332 into lp:python-jujuclient

[Jorge Niedbalski]

This patch fixes LP: #1456332

[Tim Van Steenburgh (tvansteenburgh)]

One minor quibble: it's not clear what happens if you call Run without providing any machines, services, or units. Is that an error from Juju, or is it the equivalent of calling RunOnAllMachines? It would be great to clarify this in the run() docstring.

Otherwise looks good, pending a successful test run.

review: Needs Fixing

[Felipe Reyes (freyes)]

Hi @niedbalski, I was reviewing your patch and looks fine to me, but I would like to ask you to add two more tests, to test when units are passed and another one for service, at the moment you're only testing with machines.

Best,

review: Needs Fixing

[Jorge Niedbalski (niedbalski)]

Thanks guys for taking the time for reviewing.

@Tim,

I think that this is indeed a Juju-core validation error.

I submitted a PR for handling this on the server side: https://github.com/juju/juju/pull/2355

I don't think we should threat this as a special case, since this is a library and
the RPC call should warn about that. But, if you have any other idea, please let me know.

@Felipe,

I re-submitted addressing your comments adding 2 new tests.

[Jorge Niedbalski (niedbalski)]

@Tim

I added an assertion if no target is passed, just in case.

[lp:~niedbalski/python-jujuclient/lp-1456332]

60. By Jorge Niedbalski on 2015-05-19

assert if no target is passed

Unmerged revisions

=== modified file 'jujuclient.py'
--- jujuclient.py	2015-05-11 14:33:56 +0000
+++ jujuclient.py	2015-05-19 01:17:38 +0000
@@ -273,7 +273,7 @@
     def _rpc(self, op):
         if not self._auth and not op.get("Request") == "Login":
             raise LoginRequired()
-        if not 'Params' in op:
+        if 'Params' not in op:
             op['Params'] = {}
         op['RequestId'] = self._request_id
         self._request_id += 1
@@ -813,14 +813,40 @@
             "Params": {"Commands": command,
                        "Timeout": timeout}})
 
-    def run(self, targets, command, timeout=None):
+    def run(self, command, timeout=None, machines=None,
+            services=None, units=None):
         """Run a shell command on the targets (services, units, or machines).
+        At least one target must be specified machines || services || units
         """
-        return self._rpc({
+
+        assert not (not machines and not services and not units), \
+            "You must specify a target"
+
+        rpc_dict = {
             "Type": "Client",
             "Request": "Run",
-            "Params": {"Commands": command,
-                       "Timeout": timeout}})
+            "Params": {
+                "Commands": command,
+                "Timeout": timeout,
+            }
+        }
+
+        if machines:
+            if not isinstance(machines, (list, tuple)):
+                machines = [machines]
+            rpc_dict.update({'Machines': machines})
+
+        if services:
+            if not isinstance(services, (list, tuple)):
+                services = [services]
+            rpc_dict.update({'Services': services})
+
+        if units:
+            if not isinstance(units, (list, tuple)):
+                units = [units]
+            rpc_dict.update({'Units': units})
+
+        return self._rpc(rpc_dict)
 
     # Machine ops
     def add_machine(self, series="", constraints=None,

=== modified file 'test_jujuclient.py'
--- test_jujuclient.py	2015-05-11 14:33:56 +0000
+++ test_jujuclient.py	2015-05-19 01:17:38 +0000
@@ -83,6 +83,7 @@
             ValueError, Connector.split_host_port, 'I am not an ip/port combo'
         )
 
+
 class KeyManagerTest(unittest.TestCase):
     def setUp(self):
         self.env = Environment.connect(ENV_NAME)
@@ -382,5 +383,68 @@
         pass
 
 
+class TestEnvironment(unittest.TestCase):
+
+    def setUp(self):
+        self.env = Environment.connect(ENV_NAME)
+
+    def test_run_no_target(self):
+        self.assertRaises(AssertionError, self.env.run, "sudo test")
+
+    def test_run_target_machines(self):
+        with mock.patch.object(self.env, '_rpc',
+                               return_value=None) as rpc:
+            self.env.run("sudo test", machines=["0", "1"])
+
+            rpc.assert_called_once_with({
+                "Type": "Client",
+                "Request": "Run",
+                "Params": {
+                    "Commands": "sudo test",
+                    "Timeout": None,
+                },
+                "Machines": [
+                    '0',
+                    '1',
+                ]
+            })
+
+    def test_run_target_services(self):
+        with mock.patch.object(self.env, '_rpc',
+                               return_value=None) as rpc:
+            self.env.run("sudo test", services=["cinder", "glance"])
+
+            rpc.assert_called_once_with({
+                "Type": "Client",
+                "Request": "Run",
+                "Params": {
+                    "Commands": "sudo test",
+                    "Timeout": None,
+                },
+                "Services": [
+                    'cinder',
+                    'glance',
+                ]
+            })
+
+    def test_run_target_units(self):
+        with mock.patch.object(self.env, '_rpc',
+                               return_value=None) as rpc:
+            self.env.run("sudo test", units=["mysql/0", "mysql/1"])
+
+            rpc.assert_called_once_with({
+                "Type": "Client",
+                "Request": "Run",
+                "Params": {
+                    "Commands": "sudo test",
+                    "Timeout": None,
+                },
+                "Units": [
+                    'mysql/0',
+                    'mysql/1',
+                ]
+            })
+
+
 if __name__ == '__main__':
     unittest.main()