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.

[Kapil Thangavelu (hazmat)]

thats some api breakage with this merge proposal... and seems like breakage
in core if its needed.. all that said looks reasonable

On Mon, May 18, 2015 at 9:19 PM, Jorge Niedbalski <
<email address hidden>> wrote:

> Jorge Niedbalski has proposed merging
> lp:~niedbalski/python-jujuclient/lp-1456332 into lp:python-jujuclient.
>
> Requested reviews:
> Felipe Reyes (freyes)
> Tim Van Steenburgh (tvansteenburgh)
> Related bugs:
> Bug #1456332 in python-jujuclient: "Run command doesn't supports
> specific targets"
> https://bugs.launchpad.net/python-jujuclient/+bug/1456332
>
> For more details, see:
>
> https://code.launchpad.net/~niedbalski/python-jujuclient/lp-1456332/+merge/259450
>
> This patch fixes LP: #1456332
> --
> Your team juju-deployers is subscribed to branch lp:python-jujuclient.
>
> === modified file 'jujuclient.py'
> --- jujuclient.py 2015-05-11 14:33:56 +0000
> +++ jujuclient.py 2015-05-19 01:18:19 +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:18:19 +0000
> @@ -83,6 +83,7 @@
> ValueError, Connector.split_host_port, 'I am not an ip/port
> combo'
> )
>
> +
> class KeyManagerTest(unittest.TestCase):
> def...

[Felipe Reyes (freyes)]

Thanks Jorge, LGTM.

review: Approve

[Tim Van Steenburgh (tvansteenburgh)]

> thats some api breakage with this merge proposal... and seems like breakage
> in core if its needed.. all that said looks reasonable
>

Kapil can you elaborate on what the api breakage is? The existing version of run() doesn't even pass the intended targets through to the rpc call (i.e., it appears to be broken).

> On Mon, May 18, 2015 at 9:19 PM, Jorge Niedbalski <
> <email address hidden>> wrote:
>
> > Jorge Niedbalski has proposed merging
> > lp:~niedbalski/python-jujuclient/lp-1456332 into lp:python-jujuclient.
> >
> > Requested reviews:
> > Felipe Reyes (freyes)
> > Tim Van Steenburgh (tvansteenburgh)
> > Related bugs:
> > Bug #1456332 in python-jujuclient: "Run command doesn't supports
> > specific targets"
> > https://bugs.launchpad.net/python-jujuclient/+bug/1456332
> >
> > For more details, see:
> >
> > https://code.launchpad.net/~niedbalski/python-
> jujuclient/lp-1456332/+merge/259450
> >
> > This patch fixes LP: #1456332
> > --
> > Your team juju-deployers is subscribed to branch lp:python-jujuclient.
> >
> > === modified file 'jujuclient.py'
> > --- jujuclient.py 2015-05-11 14:33:56 +0000
> > +++ jujuclient.py 2015-05-19 01:18:19 +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, s...

[Tim Van Steenburgh (tvansteenburgh)]

LGTM Jorge, thanks. There are failures in the test suite (unrelated to your changes), but that's a preexisting condition and there is already a bug open for them.

review: Approve

[Jorge Niedbalski (niedbalski)]

On Tue, May 19, 2015 at 10:20 AM, Tim Van Steenburgh
<email address hidden> wrote:
>> thats some api breakage with this merge proposal... and seems like breakage
>> in core if its needed.. all that said looks reasonable
>>
>
> Kapil can you elaborate on what the api breakage is? The existing version of run() doesn't even pass the intended targets through to the rpc call (i.e., it appears to be broken).
>
Tim,

I think he refers to the method signature change, but AFAIK there is
no internal calls to this method, and since targets were never
used on the rpc call, seems to me that this code is broken since it
was introduced.

=== modified file 'jujuclient.py'
--- jujuclient.py	2015-05-11 14:33:56 +0000
+++ jujuclient.py	2015-05-19 01:18:19 +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:18:19 +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()