Merge lp:~abentley/juju-ci-tools/remove-machine-2 into lp:juju-ci-tools

[Aaron Bentley]

Add ModelClient.remove_machine, get unicode from ModelClient.get_full_path.

This branch adds changes useful to Hammer Time.

It adds remove_machine, so that it can return WaitMachineNotPresent.

It changes get_juju_path to return a unicode string (ModelClient.full_path is unicode when user-supplied).

It changes FakeBackend.add_machines so that it behaves more like Juju itself, puking when both -n and a placement directive are specified.

[Curtis Hovey (sinzui)]

Thank you. I have a trivial request inline.

review: Approve (code)

=== modified file 'assess_cloud.py'
--- assess_cloud.py	2017-02-22 16:08:27 +0000
+++ assess_cloud.py	2017-03-02 16:55:21 +0000
@@ -90,11 +90,10 @@
             client.juju('add-machine', ('--series', current_series))
         new_status = client.wait_for_started()
         new_machines = [k for k, v in new_status.iter_new_machines(old_status)]
+        conditions = []
         for machine in new_machines:
-            client.juju('remove-machine', (machine,))
-        new_status = client.wait_for(
-            ConditionList([client.make_remove_machine_condition(n)
-                           for n in new_machines]))
+            conditions.append(client.remove_machine(machine))
+        new_status = client.wait_for(ConditionList(conditions))
 
 
 def assess_cloud_kill_controller(bs_manager):

=== modified file 'assess_heterogeneous_control.py'
--- assess_heterogeneous_control.py	2017-01-17 18:04:08 +0000
+++ assess_heterogeneous_control.py	2017-03-02 16:55:21 +0000
@@ -174,9 +174,9 @@
         container_machine, = set(k for k, v in status.agent_items() if
                                  k.endswith('/{}/0'.format(container_type)))
         container_holder = container_machine.split('/')[0]
-        other.juju('remove-machine', (container_machine,))
+        other.remove_machine(container_machine)
         wait_until_removed(other, container_machine)
-        other.juju('remove-machine', (container_holder,))
+        other.remove_machine(container_holder)
         wait_until_removed(other, container_holder)
 
 # suppress nosetests

=== modified file 'industrial_test.py'
--- industrial_test.py	2017-01-20 20:58:04 +0000
+++ industrial_test.py	2017-03-02 16:55:21 +0000
@@ -749,7 +749,7 @@
         for application in applications:
             for unit in application['units'].values():
                 container_machines.add(unit['machine'])
-                client.juju('remove-machine', ('--force', unit['machine']))
+                client.remove_machine(unit['machine'], force=True)
         remove_timeout = {
             LXC_MACHINE: 30,
             LXD_MACHINE: 900,

=== modified file 'jujupy/client.py'
--- jujupy/client.py	2017-03-01 19:02:24 +0000
+++ jujupy/client.py	2017-03-02 16:55:21 +0000
@@ -15,6 +15,7 @@
 import errno
 from itertools import chain
 import json
+from locale import getpreferredencoding
 import logging
 import os
 import re
@@ -1315,6 +1316,18 @@
         super(WaitMachineNotPresent, self).__init__(timeout)
         self.machine = machine
 
+    def __eq__(self, other):
+        if not type(self) is type(other):
+            return False
+        if self.timeout != other.timeout:
+            return False
+        if self.machine != other.machine:
+            return False
+        return True
+
+    def __ne__(self, other):
+        return not self.__eq__(other)
+
     def iter_blocking_state(self, status):
         for machine, info in status.iter_machines():
             if machine == self.machine:
@@ -1460,7 +1473,8 @@
     def get_full_path(cls):
         if sys.platform == 'win32':
             return WIN_JUJU_CMD
-        return subprocess.check_output(('which', 'juju')).rstrip(b'\n')
+        return subprocess.check_output(
+            ('which', 'juju')).decode(getpreferredencoding()).rstrip('\n')
 
     def clone_path_cls(self, juju_path):
         """Clone using the supplied path to determine the class."""
@@ -1621,6 +1635,19 @@
             timeout = 600
         return WaitMachineNotPresent(machine, timeout)
 
+    def remove_machine(self, machine_id, force=False):
+        """Remove a machine (or container).
+
+        :param machine_id: The id of the machine to remove.
+        :return: A WaitMachineNotPresent instance for client.wait_for.
+        """
+        if force:
+            options = ('--force',)
+        else:
+            options = ()
+        self.juju('remove-machine', options + (machine_id,))
+        return self.make_remove_machine_condition(machine_id)
+
     @staticmethod
     def get_cloud_region(cloud, region):
         if region is None:

=== modified file 'jujupy/fake.py'
--- jujupy/fake.py	2017-02-28 21:38:36 +0000
+++ jujupy/fake.py	2017-03-02 16:55:21 +0000
@@ -704,6 +704,9 @@
         parser.add_argument('-n', type=int, dest='count', default='1')
         parser.add_argument('--series')
         parsed = parser.parse_args(args)
+        if len(parsed.host_placement) > 0 and parsed.count != 1:
+                raise subprocess.CalledProcessError(
+                    1, 'cannot use -n when specifying a placement directive')
         if len(parsed.host_placement) == 1:
             split = parsed.host_placement[0].split(':')
             if len(split) == 1:

=== modified file 'jujupy/tests/test_client.py'
--- jujupy/tests/test_client.py	2017-02-28 21:38:36 +0000
+++ jujupy/tests/test_client.py	2017-03-02 16:55:21 +0000
@@ -307,6 +307,14 @@
         self.assertEqual(('ctrl1', 'user1', 'model1'), result)
 
 
+def backend_call(client, cmd, args, model=None, check=True, timeout=None,
+                 extra_env=None):
+    """Return the mock.call for this command."""
+    return call(cmd, args, client.used_feature_flags,
+                client.env.juju_home, model, check, timeout, extra_env,
+                suppress_err=False)
+
+
 class TestBaseCondition(ClientTest):
 
     def test_timeout(self):
@@ -361,6 +369,29 @@
 
 class TestModelClient(ClientTest):
 
+    def test_get_full_path(self):
+        with patch('subprocess.check_output',
+                   return_value=b'asdf\n') as co_mock:
+            with patch('sys.platform', 'linux2'):
+                path = ModelClient.get_full_path()
+        co_mock.assert_called_once_with(('which', 'juju'))
+        expected = u'asdf'
+        self.assertEqual(expected, path)
+        self.assertIs(type(expected), type(path))
+
+    def test_get_full_path_encoding(self):
+        # Test with non-ascii-compatible encoding
+        with patch('subprocess.check_output',
+                   return_value='asdf\n'.encode('EBCDIC-CP-BE')) as co_mock:
+            with patch('sys.platform', 'linux2'):
+                with patch('jujupy.client.getpreferredencoding',
+                           return_value='EBCDIC-CP-BE'):
+                    path = ModelClient.get_full_path()
+        co_mock.assert_called_once_with(('which', 'juju'))
+        expected = u'asdf'
+        self.assertEqual(expected, path)
+        self.assertIs(type(expected), type(path))
+
     def test_no_duplicate_env(self):
         env = JujuData('foo', {})
         client = ModelClient(env, '1.25', 'full_path')
@@ -1471,6 +1502,33 @@
             1)
         self.assertEqual(cc_mock.call_count, 2)
 
+    def test_remove_machine(self):
+        client = fake_juju_client()
+        with patch.object(client._backend, 'juju') as juju_mock:
+            condition = client.remove_machine('0')
+        call = backend_call(
+            client, 'remove-machine', ('0',), 'name:name')
+        juju_mock.assert_called_once_with(*call[1], **call[2])
+        self.assertEqual(condition, WaitMachineNotPresent('0', 600))
+
+    def test_remove_machine_force(self):
+        client = fake_juju_client()
+        with patch.object(client._backend, 'juju') as juju_mock:
+            client.remove_machine('0', force=True)
+        call = backend_call(
+            client, 'remove-machine', ('--force', '0'), 'name:name')
+        juju_mock.assert_called_once_with(*call[1], **call[2])
+
+    def test_remove_machine_azure(self):
+        client = fake_juju_client(JujuData('name', {
+            'type': 'azure',
+            'location': 'usnorth',
+            }))
+        client.bootstrap()
+        client.juju('add-machine', ())
+        condition = client.remove_machine('0')
+        self.assertEqual(condition, WaitMachineNotPresent('0', 1200))
+
     def test_wait_for_started(self):
         value = self.make_status_yaml('agent-state', 'started', 'started')
         client = ModelClient(JujuData('local'), None, None)

=== modified file 'tests/test_assess_cloud.py'
--- tests/test_assess_cloud.py	2017-02-25 06:09:40 +0000
+++ tests/test_assess_cloud.py	2017-03-02 16:55:21 +0000
@@ -3,7 +3,6 @@
 import logging
 
 from mock import (
-    call,
     patch,
     )
 from assess_cloud import (
@@ -22,6 +21,7 @@
     Juju2Backend,
     )
 from jujupy.version_client import ModelClient2_0
+from jujupy.tests.test_client import backend_call
 from tests import (
     FakeHomeTestCase,
     observable_temp_file,
@@ -33,14 +33,6 @@
     )
 
 
-def backend_call(client, cmd, args, model=None, check=True, timeout=None,
-                 extra_env=None):
-    """Return the mock.call for this command."""
-    return call(cmd, args, client.used_feature_flags,
-                client.env.juju_home, model, check, timeout, extra_env,
-                suppress_err=False)
-
-
 @contextmanager
 def mocked_bs_manager(juju_home):
     client = fake_juju_client()