Merge lp:~gz/juju-ci-tools/substrate_maas_refac into lp:juju-ci-tools

[Martin Packman]

Changes to maas substrate implementation

As part of adding a bunch more maas substrate commands, I ended up changing some details of how the existing code worked that meant updating the existing tests. This branch is just the existing code with those changes to separate out the new additions from the implementation changes.

Note that login/logout still dump the maas output straight to stdout (and our ci logs), but other commands now consume the output given by maas and parse it as json. Commands that want to output something for the ci logs should include logging of those maas return values.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== modified file 'substrate.py'
--- substrate.py	2016-05-31 23:12:41 +0000
+++ substrate.py	2016-06-17 15:01:00 +0000
@@ -414,33 +414,36 @@
         self.url = urlparse.urljoin(url, self._API_PATH)
         self.oauth = oauth
 
+    def _maas(self, *args):
+        """Call maas api with given arguments and parse json result."""
+        output = subprocess.check_output(('maas',) + args)
+        return json.loads(output)
+
     def login(self):
         """Login with the maas cli."""
-        subprocess.check_call(
-            ['maas', 'login', self.profile, self.url, self.oauth])
+        subprocess.check_call([
+            'maas', 'login', self.profile, self.url, self.oauth])
 
     def logout(self):
         """Logout with the maas cli."""
-        subprocess.check_call(
-            ['maas', 'logout', self.profile])
+        subprocess.check_call(['maas', 'logout', self.profile])
 
     def _machine_release_args(self, machine_id):
-        return ['maas', self.profile, 'machine', 'release', machine_id]
+        return (self.profile, 'machine', 'release', machine_id)
 
     def terminate_instances(self, instance_ids):
         """Terminate the specified instances."""
         for instance in instance_ids:
             maas_system_id = instance.split('/')[5]
             log.info('Deleting %s.' % instance)
-            subprocess.check_call(self._machine_release_args(maas_system_id))
+            self._maas(*self._machine_release_args(maas_system_id))
 
     def _list_allocated_args(self):
-        return ['maas', self.profile, 'machines', 'list-allocated']
+        return (self.profile, 'machines', 'list-allocated')
 
     def get_allocated_nodes(self):
         """Return a dict of allocated nodes with the hostname as keys."""
-        data = subprocess.check_output(self._list_allocated_args())
-        nodes = json.loads(data)
+        nodes = self._maas(*self._list_allocated_args())
         allocated = {node['hostname']: node for node in nodes}
         return allocated
 
@@ -462,10 +465,10 @@
     _API_PATH = 'api/1.0/'
 
     def _list_allocated_args(self):
-        return ['maas', self.profile, 'nodes', 'list-allocated']
+        return (self.profile, 'nodes', 'list-allocated')
 
     def _machine_release_args(self, machine_id):
-        return ['maas', self.profile, 'node', 'release', machine_id]
+        return (self.profile, 'node', 'release', machine_id)
 
 
 @contextmanager

=== modified file 'tests/test_substrate.py'
--- tests/test_substrate.py	2016-05-31 23:12:41 +0000
+++ tests/test_substrate.py	2016-06-17 15:01:00 +0000
@@ -171,18 +171,17 @@
 
     def test_terminate_maas(self):
         env = get_maas_env()
-        with patch('subprocess.check_call') as cc_mock:
-            terminate_instances(env, ['/A/B/C/D/node-3d/'])
-        expected = (
-            ['maas', 'login', 'mas', 'http://10.0.10.10/MAAS/api/2.0/',
-             'a:password:string'],
-        )
-        self.assertEqual(expected, cc_mock.call_args_list[0][0])
-        expected = (['maas', 'mas', 'machine', 'release', 'node-3d'],)
-        self.assertEqual(expected, cc_mock.call_args_list[1][0])
-        expected = (['maas', 'logout', 'mas'],)
-        self.assertEqual(expected, cc_mock.call_args_list[2][0])
-        self.assertEqual(3, len(cc_mock.call_args_list))
+        with patch('subprocess.check_call', autospec=True) as cc_mock:
+            with patch('subprocess.check_output', autospec=True,
+                       return_value='{}') as co_mock:
+                terminate_instances(env, ['/A/B/C/D/node-3d/'])
+        self.assertEquals(cc_mock.call_args_list, [
+            call(['maas', 'login', 'mas', 'http://10.0.10.10/MAAS/api/2.0/',
+                  'a:password:string']),
+            call(['maas', 'logout', 'mas']),
+        ])
+        co_mock.assert_called_once_with(
+            ('maas', 'mas', 'machine', 'release', 'node-3d'))
         self.assertEqual(
             self.log_stream.getvalue(),
             'INFO Deleting /A/B/C/D/node-3d/.\n')
@@ -763,17 +762,18 @@
         account.logout()
         cc_mock.assert_called_once_with(['maas', 'logout', 'mas'])
 
-    @patch('subprocess.check_call', autospec=True)
-    def test_terminate_instances(self, cc_mock):
+    def test_terminate_instances(self):
         config = get_maas_env().config
         account = MAASAccount(
             config['name'], config['maas-server'], config['maas-oauth'])
         instance_ids = ['/A/B/C/D/node-1d/', '/A/B/C/D/node-2d/']
-        account.terminate_instances(instance_ids)
-        cc_mock.assert_any_call(
-            ['maas', 'mas', 'machine', 'release', 'node-1d'])
-        cc_mock.assert_called_with(
-            ['maas', 'mas', 'machine', 'release', 'node-2d'])
+        with patch('subprocess.check_output', autospec=True,
+                   return_value='{}') as co_mock:
+            account.terminate_instances(instance_ids)
+        co_mock.assert_any_call(
+            ('maas', 'mas', 'machine', 'release', 'node-1d'))
+        co_mock.assert_called_with(
+            ('maas', 'mas', 'machine', 'release', 'node-2d'))
 
     def test_get_allocated_nodes(self):
         config = get_maas_env().config
@@ -785,7 +785,7 @@
                    return_value=allocated_nodes_string) as co_mock:
             allocated = account.get_allocated_nodes()
         co_mock.assert_called_once_with(
-            ['maas', 'mas', 'machines', 'list-allocated'])
+            ('maas', 'mas', 'machines', 'list-allocated'))
         self.assertEqual(node, allocated['maas-node-1.maas'])
 
     def test_get_allocated_ips(self):
@@ -798,7 +798,7 @@
                    return_value=allocated_nodes_string) as co_mock:
             ips = account.get_allocated_ips()
         co_mock.assert_called_once_with(
-            ['maas', 'mas', 'machines', 'list-allocated'])
+            ('maas', 'mas', 'machines', 'list-allocated'))
         self.assertEqual('10.0.30.165', ips['maas-node-1.maas'])
 
     def test_get_allocated_ips_empty(self):
@@ -812,7 +812,7 @@
                    return_value=allocated_nodes_string) as co_mock:
             ips = account.get_allocated_ips()
         co_mock.assert_called_once_with(
-            ['maas', 'mas', 'machines', 'list-allocated'])
+            ('maas', 'mas', 'machines', 'list-allocated'))
         self.assertEqual({}, ips)
 
 
@@ -836,17 +836,18 @@
         account.logout()
         cc_mock.assert_called_once_with(['maas', 'logout', 'mas'])
 
-    @patch('subprocess.check_call', autospec=True)
-    def test_terminate_instances(self, cc_mock):
+    def test_terminate_instances(self):
         config = get_maas_env().config
         account = MAAS1Account(
             config['name'], config['maas-server'], config['maas-oauth'])
         instance_ids = ['/A/B/C/D/node-1d/', '/A/B/C/D/node-2d/']
-        account.terminate_instances(instance_ids)
-        cc_mock.assert_any_call(
-            ['maas', 'mas', 'node', 'release', 'node-1d'])
-        cc_mock.assert_called_with(
-            ['maas', 'mas', 'node', 'release', 'node-2d'])
+        with patch('subprocess.check_output', autospec=True,
+                   return_value='{}') as co_mock:
+            account.terminate_instances(instance_ids)
+        co_mock.assert_any_call(
+            ('maas', 'mas', 'node', 'release', 'node-1d'))
+        co_mock.assert_called_with(
+            ('maas', 'mas', 'node', 'release', 'node-2d'))
 
     def test_get_allocated_nodes(self):
         config = get_maas_env().config
@@ -858,7 +859,7 @@
                    return_value=allocated_nodes_string) as co_mock:
             allocated = account.get_allocated_nodes()
         co_mock.assert_called_once_with(
-            ['maas', 'mas', 'nodes', 'list-allocated'])
+            ('maas', 'mas', 'nodes', 'list-allocated'))
         self.assertEqual(node, allocated['maas-node-1.maas'])
 
     def test_get_allocated_ips(self):
@@ -871,7 +872,7 @@
                    return_value=allocated_nodes_string) as co_mock:
             ips = account.get_allocated_ips()
         co_mock.assert_called_once_with(
-            ['maas', 'mas', 'nodes', 'list-allocated'])
+            ('maas', 'mas', 'nodes', 'list-allocated'))
         self.assertEqual('10.0.30.165', ips['maas-node-1.maas'])
 
     def test_get_allocated_ips_empty(self):
@@ -885,7 +886,7 @@
                    return_value=allocated_nodes_string) as co_mock:
             ips = account.get_allocated_ips()
         co_mock.assert_called_once_with(
-            ['maas', 'mas', 'nodes', 'list-allocated'])
+            ('maas', 'mas', 'nodes', 'list-allocated'))
         self.assertEqual({}, ips)