Code review comment for lp:~niedbalski/python-jujuclient/lp-1456332

> 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, 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()
> >
> >
> >