Merge "Enhance exception handling for "network delete" command"

4 files changed, 199 insertions, 26 deletions

diff --git a/openstackclient/network/common.py b/openstackclient/network/common.py
index 1e2c4cce..a3047d84 100644
--- a/openstackclient/network/common.py
+++ b/openstackclient/network/common.py
@@ -15,6 +15,7 @@ import abc
 import six
 
 from openstackclient.common import command
+from openstackclient.common import exceptions
 
 
 @six.add_metaclass(abc.ABCMeta)
@@ -69,6 +70,42 @@ class NetworkAndComputeCommand(command.Command):
 
 
 @six.add_metaclass(abc.ABCMeta)
+class NetworkAndComputeDelete(NetworkAndComputeCommand):
+    """Network and Compute Delete
+
+    Delete class for commands that support implementation via
+    the network or compute endpoint. Such commands have different
+    implementations for take_action() and may even have different
+    arguments. This class supports bulk deletion, and error handling
+    following the rules in doc/source/command-errors.rst.
+    """
+
+    def take_action(self, parsed_args):
+        ret = 0
+        resources = getattr(parsed_args, self.resource, [])
+
+        for r in resources:
+            self.r = r
+            try:
+                if self.app.client_manager.is_network_endpoint_enabled():
+                    self.take_action_network(self.app.client_manager.network,
+                                             parsed_args)
+                else:
+                    self.take_action_compute(self.app.client_manager.compute,
+                                             parsed_args)
+            except Exception as e:
+                self.app.log.error("Failed to delete %s with name or ID "
+                                   "'%s': %s" % (self.resource, r, e))
+                ret += 1
+
+        if ret:
+            total = len(resources)
+            msg = "%s of %s %ss failed to delete." % (ret, total,
+                                                      self.resource)
+            raise exceptions.CommandError(msg)
+
+
+@six.add_metaclass(abc.ABCMeta)
 class NetworkAndComputeLister(command.Lister):
     """Network and Compute Lister
 
diff --git a/openstackclient/network/v2/network.py b/openstackclient/network/v2/network.py
index bdde9173..bf01e2ec 100644
--- a/openstackclient/network/v2/network.py
+++ b/openstackclient/network/v2/network.py
@@ -248,9 +248,13 @@ class CreateNetwork(common.NetworkAndComputeShowOne):
         return (columns, data)
 
 
-class DeleteNetwork(common.NetworkAndComputeCommand):
+class DeleteNetwork(common.NetworkAndComputeDelete):
     """Delete network(s)"""
 
+    # Used by base class to find resources in parsed_args.
+    resource = 'network'
+    r = None
+
     def update_parser_common(self, parser):
         parser.add_argument(
             'network',
@@ -258,20 +262,16 @@ class DeleteNetwork(common.NetworkAndComputeCommand):
             nargs="+",
             help=_("Network(s) to delete (name or ID)")
         )
+
         return parser
 
     def take_action_network(self, client, parsed_args):
-        for network in parsed_args.network:
-            obj = client.find_network(network)
-            client.delete_network(obj)
+        obj = client.find_network(self.r, ignore_missing=False)
+        client.delete_network(obj)
 
     def take_action_compute(self, client, parsed_args):
-        for network in parsed_args.network:
-            network = utils.find_resource(
-                client.networks,
-                network,
-            )
-            client.networks.delete(network.id)
+        network = utils.find_resource(client.networks, self.r)
+        client.networks.delete(network.id)
 
 
 class ListNetwork(common.NetworkAndComputeLister):
diff --git a/openstackclient/tests/compute/v2/fakes.py b/openstackclient/tests/compute/v2/fakes.py
index b3f3fb49..70fc386f 100644
--- a/openstackclient/tests/compute/v2/fakes.py
+++ b/openstackclient/tests/compute/v2/fakes.py
@@ -859,6 +859,25 @@ class FakeNetwork(object):
 
         return networks
 
+    @staticmethod
+    def get_networks(networks=None, count=2):
+        """Get an iterable MagicMock object with a list of faked networks.
+
+        If networks list is provided, then initialize the Mock object with the
+        list. Otherwise create one.
+
+        :param List networks:
+            A list of FakeResource objects faking networks
+        :param int count:
+            The number of networks to fake
+        :return:
+            An iterable Mock object with side_effect set to a list of faked
+            networks
+        """
+        if networks is None:
+            networks = FakeNetwork.create_networks(count=count)
+        return mock.Mock(side_effect=networks)
+
 
 class FakeHost(object):
     """Fake one host."""
diff --git a/openstackclient/tests/network/v2/test_network.py b/openstackclient/tests/network/v2/test_network.py
index c6736c56..ba810f16 100644
--- a/openstackclient/tests/network/v2/test_network.py
+++ b/openstackclient/tests/network/v2/test_network.py
@@ -14,6 +14,7 @@
 import copy
 import mock
 
+from mock import call
 from openstackclient.common import exceptions
 from openstackclient.common import utils
 from openstackclient.network.v2 import network
@@ -323,33 +324,88 @@ class TestCreateNetworkIdentityV2(TestNetwork):
 
 class TestDeleteNetwork(TestNetwork):
 
-    # The network to delete.
-    _network = network_fakes.FakeNetwork.create_one_network()
-
     def setUp(self):
         super(TestDeleteNetwork, self).setUp()
 
+        # The networks to delete
+        self._networks = network_fakes.FakeNetwork.create_networks(count=3)
+
         self.network.delete_network = mock.Mock(return_value=None)
 
-        self.network.find_network = mock.Mock(return_value=self._network)
+        self.network.find_network = network_fakes.FakeNetwork.get_networks(
+            networks=self._networks)
 
         # Get the command object to test
         self.cmd = network.DeleteNetwork(self.app, self.namespace)
 
-    def test_delete(self):
+    def test_delete_one_network(self):
         arglist = [
-            self._network.name,
+            self._networks[0].name,
         ]
         verifylist = [
-            ('network', [self._network.name]),
+            ('network', [self._networks[0].name]),
         ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
 
+        self.network.delete_network.assert_called_once_with(self._networks[0])
+        self.assertIsNone(result)
+
+    def test_delete_multiple_networks(self):
+        arglist = []
+        for n in self._networks:
+            arglist.append(n.id)
+        verifylist = [
+            ('network', arglist),
+        ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
         result = self.cmd.take_action(parsed_args)
 
-        self.network.delete_network.assert_called_once_with(self._network)
+        calls = []
+        for n in self._networks:
+            calls.append(call(n))
+        self.network.delete_network.assert_has_calls(calls)
         self.assertIsNone(result)
 
+    def test_delete_multiple_networks_exception(self):
+        arglist = [
+            self._networks[0].id,
+            'xxxx-yyyy-zzzz',
+            self._networks[1].id,
+        ]
+        verifylist = [
+            ('network', arglist),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        # Fake exception in find_network()
+        ret_find = [
+            self._networks[0],
+            exceptions.NotFound('404'),
+            self._networks[1],
+        ]
+        self.network.find_network = mock.Mock(side_effect=ret_find)
+
+        # Fake exception in delete_network()
+        ret_delete = [
+            None,
+            exceptions.NotFound('404'),
+        ]
+        self.network.delete_network = mock.Mock(side_effect=ret_delete)
+
+        self.assertRaises(exceptions.CommandError, self.cmd.take_action,
+                          parsed_args)
+
+        # The second call of find_network() should fail. So delete_network()
+        # was only called twice.
+        calls = [
+            call(self._networks[0]),
+            call(self._networks[1]),
+        ]
+        self.network.delete_network.assert_has_calls(calls)
+
 
 class TestListNetwork(TestNetwork):
 
@@ -752,36 +808,97 @@ class TestCreateNetworkCompute(TestNetworkCompute):
 
 class TestDeleteNetworkCompute(TestNetworkCompute):
 
-    # The network to delete.
-    _network = compute_fakes.FakeNetwork.create_one_network()
-
     def setUp(self):
         super(TestDeleteNetworkCompute, self).setUp()
 
         self.app.client_manager.network_endpoint_enabled = False
 
+        # The networks to delete
+        self._networks = compute_fakes.FakeNetwork.create_networks(count=3)
+
         self.compute.networks.delete.return_value = None
 
         # Return value of utils.find_resource()
-        self.compute.networks.get.return_value = self._network
+        self.compute.networks.get = \
+            compute_fakes.FakeNetwork.get_networks(networks=self._networks)
 
         # Get the command object to test
         self.cmd = network.DeleteNetwork(self.app, None)
 
-    def test_network_delete(self):
+    def test_delete_one_network(self):
         arglist = [
-            self._network.label,
+            self._networks[0].label,
         ]
         verifylist = [
-            ('network', [self._network.label]),
+            ('network', [self._networks[0].label]),
         ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
 
+        self.compute.networks.delete.assert_called_once_with(
+            self._networks[0].id)
+        self.assertIsNone(result)
+
+    def test_delete_multiple_networks(self):
+        arglist = []
+        for n in self._networks:
+            arglist.append(n.label)
+        verifylist = [
+            ('network', arglist),
+        ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
         result = self.cmd.take_action(parsed_args)
 
-        self.compute.networks.delete.assert_called_once_with(self._network.id)
+        calls = []
+        for n in self._networks:
+            calls.append(call(n.id))
+        self.compute.networks.delete.assert_has_calls(calls)
         self.assertIsNone(result)
 
+    def test_delete_multiple_networks_exception(self):
+        arglist = [
+            self._networks[0].id,
+            'xxxx-yyyy-zzzz',
+            self._networks[1].id,
+        ]
+        verifylist = [
+            ('network', arglist),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        # Fake exception in utils.find_resource()
+        # In compute v2, we use utils.find_resource() to find a network.
+        # It calls get() several times, but find() only one time. So we
+        # choose to fake get() always raise exception, then pass through.
+        # And fake find() to find the real network or not.
+        self.compute.networks.get.side_effect = Exception()
+        ret_find = [
+            self._networks[0],
+            Exception(),
+            self._networks[1],
+        ]
+        self.compute.networks.find.side_effect = ret_find
+
+        # Fake exception in delete()
+        ret_delete = [
+            None,
+            Exception(),
+        ]
+        self.compute.networks.delete = mock.Mock(side_effect=ret_delete)
+
+        self.assertRaises(exceptions.CommandError, self.cmd.take_action,
+                          parsed_args)
+
+        # The second call of utils.find_resource() should fail. So delete()
+        # was only called twice.
+        calls = [
+            call(self._networks[0].id),
+            call(self._networks[1].id),
+        ]
+        self.compute.networks.delete.assert_has_calls(calls)
+
 
 class TestListNetworkCompute(TestNetworkCompute):