diff --git a/openstackclient/tests/unit/volume/v2/test_volume.py b/openstackclient/tests/unit/volume/v2/test_volume.py index 5df96f27d4..ea43e78e24 100644 --- a/openstackclient/tests/unit/volume/v2/test_volume.py +++ b/openstackclient/tests/unit/volume/v2/test_volume.py @@ -13,6 +13,9 @@ from unittest import mock +from openstack.block_storage.v2 import volume as _volume +from openstack import exceptions as sdk_exceptions +from openstack.test import fakes as sdk_fakes from osc_lib.cli import format_columns from osc_lib import exceptions from osc_lib import utils @@ -46,12 +49,6 @@ class TestVolume(volume_fakes.TestVolume): self.consistencygroups_mock = self.volume_client.consistencygroups self.consistencygroups_mock.reset_mock() - def setup_volumes_mock(self, count): - volumes = volume_fakes.create_volumes(count=count) - - self.volumes_mock.get = volume_fakes.get_volumes(volumes, 0) - return volumes - class TestVolumeCreate(TestVolume): project = identity_fakes.FakeProject.create_one_project() @@ -662,37 +659,37 @@ class TestVolumeCreate(TestVolume): self.assertCountEqual(self.datalist, data) -class TestVolumeDelete(TestVolume): +class TestVolumeDelete(volume_fakes.TestVolume): def setUp(self): super().setUp() - self.volumes_mock.delete.return_value = None + self.volumes = list(sdk_fakes.generate_fake_resources(_volume.Volume)) + self.volume_sdk_client.find_volume.side_effect = self.volumes + self.volume_sdk_client.delete_volume.return_value = None - # Get the command object to mock self.cmd = volume.DeleteVolume(self.app, None) def test_volume_delete_one_volume(self): - volumes = self.setup_volumes_mock(count=1) - - arglist = [volumes[0].id] + arglist = [self.volumes[0].id] verifylist = [ ("force", False), ("purge", False), - ("volumes", [volumes[0].id]), + ("volumes", [self.volumes[0].id]), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - self.volumes_mock.delete.assert_called_once_with( - volumes[0].id, cascade=False - ) self.assertIsNone(result) - def test_volume_delete_multi_volumes(self): - volumes = self.setup_volumes_mock(count=3) + self.volume_sdk_client.find_volume.assert_called_once_with( + self.volumes[0].id, ignore_missing=False + ) + self.volume_sdk_client.delete_volume.assert_called_once_with( + self.volumes[0].id, cascade=False, force=False + ) - arglist = [v.id for v in volumes] + def test_volume_delete_multi_volumes(self): + arglist = [v.id for v in self.volumes] verifylist = [ ('force', False), ('purge', False), @@ -701,83 +698,95 @@ class TestVolumeDelete(TestVolume): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - calls = [mock.call(v.id, cascade=False) for v in volumes] - self.volumes_mock.delete.assert_has_calls(calls) self.assertIsNone(result) + self.volume_sdk_client.find_volume.assert_has_calls( + [mock.call(v.id, ignore_missing=False) for v in self.volumes] + ) + self.volume_sdk_client.delete_volume.assert_has_calls( + [mock.call(v.id, cascade=False, force=False) for v in self.volumes] + ) + def test_volume_delete_multi_volumes_with_exception(self): - volumes = self.setup_volumes_mock(count=2) + self.volume_sdk_client.find_volume.side_effect = [ + self.volumes[0], + sdk_exceptions.NotFoundException(), + ] arglist = [ - volumes[0].id, + self.volumes[0].id, 'unexist_volume', ] verifylist = [ ('force', False), ('purge', False), - ('volumes', arglist), + ('volumes', [self.volumes[0].id, 'unexist_volume']), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) - find_mock_result = [volumes[0], exceptions.CommandError] - with mock.patch.object( - utils, 'find_resource', side_effect=find_mock_result - ) as find_mock: - try: - self.cmd.take_action(parsed_args) - self.fail('CommandError should be raised.') - except exceptions.CommandError as e: - self.assertEqual('1 of 2 volumes failed to delete.', str(e)) + exc = self.assertRaises( + exceptions.CommandError, + self.cmd.take_action, + parsed_args, + ) + self.assertEqual('1 of 2 volumes failed to delete.', str(exc)) - find_mock.assert_any_call(self.volumes_mock, volumes[0].id) - find_mock.assert_any_call(self.volumes_mock, 'unexist_volume') - - self.assertEqual(2, find_mock.call_count) - self.volumes_mock.delete.assert_called_once_with( - volumes[0].id, cascade=False - ) + self.volume_sdk_client.find_volume.assert_has_calls( + [ + mock.call(self.volumes[0].id, ignore_missing=False), + mock.call('unexist_volume', ignore_missing=False), + ] + ) + self.volume_sdk_client.delete_volume.assert_has_calls( + [ + mock.call(self.volumes[0].id, cascade=False, force=False), + ] + ) def test_volume_delete_with_purge(self): - volumes = self.setup_volumes_mock(count=1) - arglist = [ '--purge', - volumes[0].id, + self.volumes[0].id, ] verifylist = [ ('force', False), ('purge', True), - ('volumes', [volumes[0].id]), + ('volumes', [self.volumes[0].id]), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - self.volumes_mock.delete.assert_called_once_with( - volumes[0].id, cascade=True - ) self.assertIsNone(result) - def test_volume_delete_with_force(self): - volumes = self.setup_volumes_mock(count=1) + self.volume_sdk_client.find_volume.assert_called_once_with( + self.volumes[0].id, ignore_missing=False + ) + self.volume_sdk_client.delete_volume.assert_called_once_with( + self.volumes[0].id, cascade=True, force=False + ) + def test_volume_delete_with_force(self): arglist = [ '--force', - volumes[0].id, + self.volumes[0].id, ] verifylist = [ ('force', True), ('purge', False), - ('volumes', [volumes[0].id]), + ('volumes', [self.volumes[0].id]), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - self.volumes_mock.force_delete.assert_called_once_with(volumes[0].id) self.assertIsNone(result) + self.volume_sdk_client.find_volume.assert_called_once_with( + self.volumes[0].id, ignore_missing=False + ) + self.volume_sdk_client.delete_volume.assert_called_once_with( + self.volumes[0].id, cascade=False, force=True + ) + class TestVolumeList(TestVolume): project = identity_fakes.FakeProject.create_one_project() diff --git a/openstackclient/tests/unit/volume/v3/test_volume.py b/openstackclient/tests/unit/volume/v3/test_volume.py index 6bf43d73f4..359e3d06a7 100644 --- a/openstackclient/tests/unit/volume/v3/test_volume.py +++ b/openstackclient/tests/unit/volume/v3/test_volume.py @@ -17,6 +17,7 @@ from unittest import mock from openstack.block_storage.v3 import block_storage_summary as _summary from openstack.block_storage.v3 import snapshot as _snapshot from openstack.block_storage.v3 import volume as _volume +from openstack import exceptions as sdk_exceptions from openstack.test import fakes as sdk_fakes from osc_lib.cli import format_columns from osc_lib import exceptions @@ -954,16 +955,17 @@ class TestVolumeCreate(volume_fakes.TestVolume): ) -class TestVolumeDeleteLegacy(volume_fakes.TestVolume): +class TestVolumeDelete(volume_fakes.TestVolume): def setUp(self): super().setUp() self.volumes_mock = self.volume_client.volumes self.volumes_mock.reset_mock() - self.volumes_mock.delete.return_value = None - self.volumes = volume_fakes.create_volumes(count=1) - self.volumes_mock.get = volume_fakes.get_volumes(self.volumes, 0) + self.volumes = list(sdk_fakes.generate_fake_resources(_volume.Volume)) + self.volume_sdk_client.find_volume.side_effect = self.volumes + self.volume_sdk_client.delete_volume.return_value = None + self.volume_sdk_client.unmanage_volume.return_value = None # Get the command object to mock self.cmd = volume.DeleteVolume(self.app, None) @@ -978,12 +980,15 @@ class TestVolumeDeleteLegacy(volume_fakes.TestVolume): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - self.volumes_mock.delete.assert_called_once_with( - self.volumes[0].id, cascade=False - ) self.assertIsNone(result) + self.volume_sdk_client.find_volume.assert_called_once_with( + self.volumes[0].id, ignore_missing=False + ) + self.volume_sdk_client.delete_volume.assert_called_once_with( + self.volumes[0].id, cascade=False, force=False + ) + def test_volume_delete_multi_volumes(self): arglist = [v.id for v in self.volumes] verifylist = [ @@ -994,12 +999,21 @@ class TestVolumeDeleteLegacy(volume_fakes.TestVolume): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - calls = [mock.call(v.id, cascade=False) for v in self.volumes] - self.volumes_mock.delete.assert_has_calls(calls) self.assertIsNone(result) + self.volume_sdk_client.find_volume.assert_has_calls( + [mock.call(v.id, ignore_missing=False) for v in self.volumes] + ) + self.volume_sdk_client.delete_volume.assert_has_calls( + [mock.call(v.id, cascade=False, force=False) for v in self.volumes] + ) + def test_volume_delete_multi_volumes_with_exception(self): + self.volume_sdk_client.find_volume.side_effect = [ + self.volumes[0], + sdk_exceptions.NotFoundException(), + ] + arglist = [ self.volumes[0].id, 'unexist_volume', @@ -1007,27 +1021,28 @@ class TestVolumeDeleteLegacy(volume_fakes.TestVolume): verifylist = [ ('force', False), ('purge', False), - ('volumes', arglist), + ('volumes', [self.volumes[0].id, 'unexist_volume']), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) - find_mock_result = [self.volumes[0], exceptions.CommandError] - with mock.patch.object( - utils, 'find_resource', side_effect=find_mock_result - ) as find_mock: - try: - self.cmd.take_action(parsed_args) - self.fail('CommandError should be raised.') - except exceptions.CommandError as e: - self.assertEqual('1 of 2 volumes failed to delete.', str(e)) + exc = self.assertRaises( + exceptions.CommandError, + self.cmd.take_action, + parsed_args, + ) + self.assertEqual('1 of 2 volumes failed to delete.', str(exc)) - find_mock.assert_any_call(self.volumes_mock, self.volumes[0].id) - find_mock.assert_any_call(self.volumes_mock, 'unexist_volume') - - self.assertEqual(2, find_mock.call_count) - self.volumes_mock.delete.assert_called_once_with( - self.volumes[0].id, cascade=False - ) + self.volume_sdk_client.find_volume.assert_has_calls( + [ + mock.call(self.volumes[0].id, ignore_missing=False), + mock.call('unexist_volume', ignore_missing=False), + ] + ) + self.volume_sdk_client.delete_volume.assert_has_calls( + [ + mock.call(self.volumes[0].id, cascade=False, force=False), + ] + ) def test_volume_delete_with_purge(self): arglist = [ @@ -1042,12 +1057,15 @@ class TestVolumeDeleteLegacy(volume_fakes.TestVolume): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - self.volumes_mock.delete.assert_called_once_with( - self.volumes[0].id, cascade=True - ) self.assertIsNone(result) + self.volume_sdk_client.find_volume.assert_called_once_with( + self.volumes[0].id, ignore_missing=False + ) + self.volume_sdk_client.delete_volume.assert_called_once_with( + self.volumes[0].id, cascade=True, force=False + ) + def test_volume_delete_with_force(self): arglist = [ '--force', @@ -1061,49 +1079,38 @@ class TestVolumeDeleteLegacy(volume_fakes.TestVolume): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - self.volumes_mock.force_delete.assert_called_once_with( - self.volumes[0].id - ) self.assertIsNone(result) - -class TestVolumeDelete(volume_fakes.TestVolume): - def setUp(self): - super().setUp() - - self.volumes_mock = self.volume_client.volumes - self.volumes_mock.reset_mock() - self.volume_sdk_client.unmanage_volume.return_value = None - - # Get the command object to mock - self.cmd = volume.DeleteVolume(self.app, None) + self.volume_sdk_client.find_volume.assert_called_once_with( + self.volumes[0].id, ignore_missing=False + ) + self.volume_sdk_client.delete_volume.assert_called_once_with( + self.volumes[0].id, cascade=False, force=True + ) def test_volume_delete_remote(self): - vol = sdk_fakes.generate_fake_resource(_volume.Volume, **{'size': 1}) - self.volumes_mock.get.return_value = vol - - arglist = ['--remote', vol.id] + arglist = ['--remote', self.volumes[0].id] verifylist = [ ("remote", True), ("force", False), ("purge", False), - ("volumes", [vol.id]), + ("volumes", [self.volumes[0].id]), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - self.volume_sdk_client.unmanage_volume.assert_called_once_with(vol.id) self.assertIsNone(result) - def test_volume_delete_multi_volumes_remote(self): - volumes = sdk_fakes.generate_fake_resources( - _volume.Volume, count=3, attrs={'size': 1} + self.volume_sdk_client.find_volume.assert_called_once_with( + self.volumes[0].id, ignore_missing=False + ) + self.volume_sdk_client.delete_volume.assert_not_called() + self.volume_sdk_client.unmanage_volume.assert_called_once_with( + self.volumes[0].id ) - arglist = ['--remote'] - arglist += [v.id for v in volumes] + def test_volume_delete_multi_volumes_remote(self): + arglist = ['--remote'] + [v.id for v in self.volumes] verifylist = [ ('remote', True), ('force', False), @@ -1113,24 +1120,27 @@ class TestVolumeDelete(volume_fakes.TestVolume): parsed_args = self.check_parser(self.cmd, arglist, verifylist) result = self.cmd.take_action(parsed_args) - - calls = [mock.call(v.id) for v in volumes] - self.volume_sdk_client.unmanage_volume.assert_has_calls(calls) self.assertIsNone(result) - def test_volume_delete_remote_with_purge(self): - vol = sdk_fakes.generate_fake_resource(_volume.Volume, **{'size': 1}) + self.volume_sdk_client.find_volume.assert_has_calls( + [mock.call(v.id, ignore_missing=False) for v in self.volumes] + ) + self.volume_sdk_client.delete_volume.assert_not_called() + self.volume_sdk_client.unmanage_volume.assert_has_calls( + [mock.call(v.id) for v in self.volumes] + ) + def test_volume_delete_remote_with_purge(self): arglist = [ '--remote', '--purge', - vol.id, + self.volumes[0].id, ] verifylist = [ ('remote', True), ('force', False), ('purge', True), - ('volumes', [vol.id]), + ('volumes', [self.volumes[0].id]), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -1143,19 +1153,21 @@ class TestVolumeDelete(volume_fakes.TestVolume): str(exc), ) - def test_volume_delete_remote_with_force(self): - vol = sdk_fakes.generate_fake_resource(_volume.Volume, **{'size': 1}) + self.volume_sdk_client.find_volume.assert_not_called() + self.volume_sdk_client.delete_volume.assert_not_called() + self.volume_sdk_client.unmanage_volume.assert_not_called() + def test_volume_delete_remote_with_force(self): arglist = [ '--remote', '--force', - vol.id, + self.volumes[0].id, ] verifylist = [ ('remote', True), ('force', True), ('purge', False), - ('volumes', [vol.id]), + ('volumes', [self.volumes[0].id]), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -1168,6 +1180,10 @@ class TestVolumeDelete(volume_fakes.TestVolume): str(exc), ) + self.volume_sdk_client.find_volume.assert_not_called() + self.volume_sdk_client.delete_volume.assert_not_called() + self.volume_sdk_client.unmanage_volume.assert_not_called() + class TestVolumeList(volume_fakes.TestVolume): project = identity_fakes.FakeProject.create_one_project() diff --git a/openstackclient/volume/v2/volume.py b/openstackclient/volume/v2/volume.py index 1b5e4666cb..ad1fbe1ac2 100644 --- a/openstackclient/volume/v2/volume.py +++ b/openstackclient/volume/v2/volume.py @@ -364,18 +364,19 @@ class DeleteVolume(command.Command): return parser def take_action(self, parsed_args): - volume_client = self.app.client_manager.volume + volume_client = self.app.client_manager.sdk_connection.volume result = 0 - for i in parsed_args.volumes: + for volume in parsed_args.volumes: try: - volume_obj = utils.find_resource(volume_client.volumes, i) - if parsed_args.force: - volume_client.volumes.force_delete(volume_obj.id) - else: - volume_client.volumes.delete( - volume_obj.id, cascade=parsed_args.purge - ) + volume_obj = volume_client.find_volume( + volume, ignore_missing=False + ) + volume_client.delete_volume( + volume_obj.id, + force=parsed_args.force, + cascade=parsed_args.purge, + ) except Exception as e: result += 1 LOG.error( @@ -383,7 +384,7 @@ class DeleteVolume(command.Command): "Failed to delete volume with " "name or ID '%(volume)s': %(e)s" ), - {'volume': i, 'e': e}, + {'volume': volume, 'e': e}, ) if result > 0: diff --git a/openstackclient/volume/v3/volume.py b/openstackclient/volume/v3/volume.py index 6786f99671..6852944ec2 100644 --- a/openstackclient/volume/v3/volume.py +++ b/openstackclient/volume/v3/volume.py @@ -505,8 +505,7 @@ class DeleteVolume(command.Command): return parser def take_action(self, parsed_args): - volume_client = self.app.client_manager.volume - volume_client_sdk = self.app.client_manager.sdk_connection.volume + volume_client = self.app.client_manager.sdk_connection.volume result = 0 if parsed_args.remote and (parsed_args.force or parsed_args.purge): @@ -516,16 +515,18 @@ class DeleteVolume(command.Command): ) raise exceptions.CommandError(msg) - for i in parsed_args.volumes: + for volume in parsed_args.volumes: try: - volume_obj = utils.find_resource(volume_client.volumes, i) + volume_obj = volume_client.find_volume( + volume, ignore_missing=False + ) if parsed_args.remote: - volume_client_sdk.unmanage_volume(volume_obj.id) - elif parsed_args.force: - volume_client.volumes.force_delete(volume_obj.id) + volume_client.unmanage_volume(volume_obj.id) else: - volume_client.volumes.delete( - volume_obj.id, cascade=parsed_args.purge + volume_client.delete_volume( + volume_obj.id, + force=parsed_args.force, + cascade=parsed_args.purge, ) except Exception as e: result += 1 @@ -534,7 +535,7 @@ class DeleteVolume(command.Command): "Failed to delete volume with " "name or ID '%(volume)s': %(e)s" ), - {'volume': i, 'e': e}, + {'volume': volume, 'e': e}, ) if result > 0: