From 709eac73fbf0691d8012052773eec73006adf704 Mon Sep 17 00:00:00 2001 From: Dean Troyer Date: Fri, 14 Oct 2016 14:01:42 -0500 Subject: Fix volume transfers request commands * Fix volume transfer request accept to actually not crash when trying to call Volume API. * Fix volume transfer request accept syntax to have only one positional argument, which is the ID of the resource in the command * Change the output column order in volume transfer request list to have ID followed by Name then the remaining columns. Closes-bug: 1633582 Change-Id: I5cc005f039d171cc70859f60e7fe649b09ead229 --- .../functional/volume/v1/test_transfer_request.py | 12 ++-- .../functional/volume/v2/test_transfer_request.py | 12 ++-- .../tests/unit/volume/v1/test_transfer_request.py | 56 +++++++++++++--- .../tests/unit/volume/v2/test_transfer_request.py | 56 +++++++++++++--- .../volume/v1/volume_transfer_request.py | 75 ++++++++++++++++------ .../volume/v2/volume_transfer_request.py | 75 ++++++++++++++++------ 6 files changed, 222 insertions(+), 64 deletions(-) (limited to 'openstackclient') diff --git a/openstackclient/tests/functional/volume/v1/test_transfer_request.py b/openstackclient/tests/functional/volume/v1/test_transfer_request.py index e03cd717..3fe11913 100644 --- a/openstackclient/tests/functional/volume/v1/test_transfer_request.py +++ b/openstackclient/tests/functional/volume/v1/test_transfer_request.py @@ -10,6 +10,7 @@ # License for the specific language governing permissions and limitations # under the License. +import json import uuid from openstackclient.tests.functional.volume.v1 import common @@ -67,11 +68,12 @@ class TransferRequestTests(common.BaseVolumeTests): self.assertNotEqual('', auth_key) # accept the volume transfer request - opts = self.get_opts(self.FIELDS) - raw_output = self.openstack( - 'volume transfer request accept ' + name + - ' ' + auth_key + opts) - self.assertEqual(name + '\n', raw_output) + json_output = json.loads(self.openstack( + 'volume transfer request accept -f json ' + + name + ' ' + + '--auth-key ' + auth_key + )) + self.assertEqual(name, json_output.get('name')) # the volume transfer will be removed by default after accepted # so just need to delete the volume here diff --git a/openstackclient/tests/functional/volume/v2/test_transfer_request.py b/openstackclient/tests/functional/volume/v2/test_transfer_request.py index 1791f8ac..99d91ac0 100644 --- a/openstackclient/tests/functional/volume/v2/test_transfer_request.py +++ b/openstackclient/tests/functional/volume/v2/test_transfer_request.py @@ -10,6 +10,7 @@ # License for the specific language governing permissions and limitations # under the License. +import json import uuid from openstackclient.tests.functional.volume.v2 import common @@ -67,11 +68,12 @@ class TransferRequestTests(common.BaseVolumeTests): self.assertNotEqual('', auth_key) # accept the volume transfer request - opts = self.get_opts(self.FIELDS) - raw_output = self.openstack( - 'volume transfer request accept ' + name + - ' ' + auth_key + opts) - self.assertEqual(name + '\n', raw_output) + json_output = json.loads(self.openstack( + 'volume transfer request accept -f json ' + + name + ' ' + + '--auth-key ' + auth_key + )) + self.assertEqual(name, json_output.get('name')) # the volume transfer will be removed by default after accepted # so just need to delete the volume here diff --git a/openstackclient/tests/unit/volume/v1/test_transfer_request.py b/openstackclient/tests/unit/volume/v1/test_transfer_request.py index b3788d6e..4c013dc0 100644 --- a/openstackclient/tests/unit/volume/v1/test_transfer_request.py +++ b/openstackclient/tests/unit/volume/v1/test_transfer_request.py @@ -64,24 +64,62 @@ class TestTransferAccept(TestTransfer): def test_transfer_accept(self): arglist = [ + '--auth-key', 'key_value', self.volume_transfer.id, - 'auth_key', ] verifylist = [ ('transfer_request', self.volume_transfer.id), - ('auth_key', 'auth_key'), + ('auth_key', 'key_value'), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) columns, data = self.cmd.take_action(parsed_args) self.transfer_mock.get.assert_called_once_with( - self.volume_transfer.id) + self.volume_transfer.id, + ) self.transfer_mock.accept.assert_called_once_with( - self.volume_transfer.id, 'auth_key') + self.volume_transfer.id, + 'key_value', + ) self.assertEqual(self.columns, columns) self.assertEqual(self.data, data) + def test_transfer_accept_deprecated(self): + arglist = [ + self.volume_transfer.id, + 'key_value', + ] + verifylist = [ + ('transfer_request', self.volume_transfer.id), + ('old_auth_key', 'key_value'), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + columns, data = self.cmd.take_action(parsed_args) + + self.transfer_mock.accept.assert_called_once_with( + self.volume_transfer.id, + 'key_value', + ) + self.assertEqual(self.columns, columns) + self.assertEqual(self.data, data) + + def test_transfer_accept_no_option(self): + arglist = [ + self.volume_transfer.id, + ] + verifylist = [ + ('transfer_request', self.volume_transfer.id), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.assertRaises( + exceptions.CommandError, + self.cmd.take_action, + parsed_args, + ) + class TestTransferCreate(TestTransfer): @@ -219,7 +257,7 @@ class TestTransferDelete(TestTransfer): self.fail('CommandError should be raised.') except exceptions.CommandError as e: self.assertEqual('1 of 2 volume transfer requests failed ' - 'to delete.', str(e)) + 'to delete', str(e)) find_mock.assert_any_call( self.transfer_mock, self.volume_transfers[0].id) @@ -256,8 +294,8 @@ class TestTransferList(TestTransfer): expected_columns = [ 'ID', - 'Volume', 'Name', + 'Volume', ] # confirming if all expected columns are present in the result. @@ -265,8 +303,8 @@ class TestTransferList(TestTransfer): datalist = (( self.volume_transfers.id, - self.volume_transfers.volume_id, self.volume_transfers.name, + self.volume_transfers.volume_id, ), ) # confirming if all expected values are present in the result. @@ -295,8 +333,8 @@ class TestTransferList(TestTransfer): expected_columns = [ 'ID', - 'Volume', 'Name', + 'Volume', ] # confirming if all expected columns are present in the result. @@ -304,8 +342,8 @@ class TestTransferList(TestTransfer): datalist = (( self.volume_transfers.id, - self.volume_transfers.volume_id, self.volume_transfers.name, + self.volume_transfers.volume_id, ), ) # confirming if all expected values are present in the result. diff --git a/openstackclient/tests/unit/volume/v2/test_transfer_request.py b/openstackclient/tests/unit/volume/v2/test_transfer_request.py index 8cd6534b..37eed11e 100644 --- a/openstackclient/tests/unit/volume/v2/test_transfer_request.py +++ b/openstackclient/tests/unit/volume/v2/test_transfer_request.py @@ -64,24 +64,62 @@ class TestTransferAccept(TestTransfer): def test_transfer_accept(self): arglist = [ + '--auth-key', 'key_value', self.volume_transfer.id, - 'auth_key', ] verifylist = [ ('transfer_request', self.volume_transfer.id), - ('auth_key', 'auth_key'), + ('auth_key', 'key_value'), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) columns, data = self.cmd.take_action(parsed_args) self.transfer_mock.get.assert_called_once_with( - self.volume_transfer.id) + self.volume_transfer.id, + ) self.transfer_mock.accept.assert_called_once_with( - self.volume_transfer.id, 'auth_key') + self.volume_transfer.id, + 'key_value', + ) self.assertEqual(self.columns, columns) self.assertEqual(self.data, data) + def test_transfer_accept_deprecated(self): + arglist = [ + self.volume_transfer.id, + 'key_value', + ] + verifylist = [ + ('transfer_request', self.volume_transfer.id), + ('old_auth_key', 'key_value'), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + columns, data = self.cmd.take_action(parsed_args) + + self.transfer_mock.accept.assert_called_once_with( + self.volume_transfer.id, + 'key_value', + ) + self.assertEqual(self.columns, columns) + self.assertEqual(self.data, data) + + def test_transfer_accept_no_option(self): + arglist = [ + self.volume_transfer.id, + ] + verifylist = [ + ('transfer_request', self.volume_transfer.id), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.assertRaises( + exceptions.CommandError, + self.cmd.take_action, + parsed_args, + ) + class TestTransferCreate(TestTransfer): @@ -219,7 +257,7 @@ class TestTransferDelete(TestTransfer): self.fail('CommandError should be raised.') except exceptions.CommandError as e: self.assertEqual('1 of 2 volume transfer requests failed ' - 'to delete.', str(e)) + 'to delete', str(e)) find_mock.assert_any_call( self.transfer_mock, self.volume_transfers[0].id) @@ -256,8 +294,8 @@ class TestTransferList(TestTransfer): expected_columns = [ 'ID', - 'Volume', 'Name', + 'Volume', ] # confirming if all expected columns are present in the result. @@ -265,8 +303,8 @@ class TestTransferList(TestTransfer): datalist = (( self.volume_transfers.id, - self.volume_transfers.volume_id, self.volume_transfers.name, + self.volume_transfers.volume_id, ), ) # confirming if all expected values are present in the result. @@ -295,8 +333,8 @@ class TestTransferList(TestTransfer): expected_columns = [ 'ID', - 'Volume', 'Name', + 'Volume', ] # confirming if all expected columns are present in the result. @@ -304,8 +342,8 @@ class TestTransferList(TestTransfer): datalist = (( self.volume_transfers.id, - self.volume_transfers.volume_id, self.volume_transfers.name, + self.volume_transfers.volume_id, ), ) # confirming if all expected values are present in the result. diff --git a/openstackclient/volume/v1/volume_transfer_request.py b/openstackclient/volume/v1/volume_transfer_request.py index f24d5a56..f5b567b9 100644 --- a/openstackclient/volume/v1/volume_transfer_request.py +++ b/openstackclient/volume/v1/volume_transfer_request.py @@ -14,6 +14,7 @@ """Volume v1 transfer action implementations""" +import argparse import logging from osc_lib.command import command @@ -34,22 +35,54 @@ class AcceptTransferRequest(command.ShowOne): parser = super(AcceptTransferRequest, self).get_parser(prog_name) parser.add_argument( 'transfer_request', - metavar="", - help=_('Volume transfer request to accept (name or ID)'), + metavar="", + help=_('Volume transfer request to accept (ID only)'), + ) + parser.add_argument( + 'old_auth_key', + metavar="", + nargs="?", + help=argparse.SUPPRESS, ) parser.add_argument( - 'auth_key', - metavar="", - help=_('Authentication key of transfer request'), + '--auth-key', + metavar="", + help=_('Volume transfer request authentication key'), ) return parser def take_action(self, parsed_args): volume_client = self.app.client_manager.volume - transfer_request_id = utils.find_resource( - volume_client.transfers, parsed_args.transfer_request).id + + try: + transfer_request_id = utils.find_resource( + volume_client.transfers, + parsed_args.transfer_request + ).id + except exceptions.CommandError: + # Non-admin users will fail to lookup name -> ID so we just + # move on and attempt with the user-supplied information + transfer_request_id = parsed_args.transfer_request + + # Remain backward-compatible for the previous command layout + # TODO(dtroyer): Remove this back-compat in 4.0 or Oct 2017 + if not parsed_args.auth_key: + if parsed_args.old_auth_key: + # Move the old one into the correct place + parsed_args.auth_key = parsed_args.old_auth_key + self.log.warning(_( + 'Specifying the auth-key as a positional argument ' + 'has been deprecated. Please use the --auth-key ' + 'option in the future.' + )) + else: + msg = _("argument --auth-key is required") + raise exceptions.CommandError(msg) + transfer_accept = volume_client.transfers.accept( - transfer_request_id, parsed_args.auth_key) + transfer_request_id, + parsed_args.auth_key, + ) transfer_accept._info.pop("links", None) return zip(*sorted(six.iteritems(transfer_accept._info))) @@ -75,9 +108,12 @@ class CreateTransferRequest(command.ShowOne): def take_action(self, parsed_args): volume_client = self.app.client_manager.volume volume_id = utils.find_resource( - volume_client.volumes, parsed_args.volume).id + volume_client.volumes, + parsed_args.volume, + ).id volume_transfer_request = volume_client.transfers.create( - volume_id, parsed_args.name, + volume_id, + parsed_args.name, ) volume_transfer_request._info.pop("links", None) @@ -104,7 +140,9 @@ class DeleteTransferRequest(command.Command): for t in parsed_args.transfer_request: try: transfer_request_id = utils.find_resource( - volume_client.transfers, t).id + volume_client.transfers, + t, + ).id volume_client.transfers.delete(transfer_request_id) except Exception as e: result += 1 @@ -115,7 +153,7 @@ class DeleteTransferRequest(command.Command): if result > 0: total = len(parsed_args.transfer_request) msg = (_("%(result)s of %(total)s volume transfer requests failed" - " to delete.") % {'result': result, 'total': total}) + " to delete") % {'result': result, 'total': total}) raise exceptions.CommandError(msg) @@ -129,20 +167,19 @@ class ListTransferRequest(command.Lister): dest='all_projects', action="store_true", default=False, - help=_('Shows detail for all projects. Admin only. ' - '(defaults to False)') + help=_('Include all projects (admin only)'), ) return parser def take_action(self, parsed_args): - columns = ['ID', 'Volume ID', 'Name'] - column_headers = ['ID', 'Volume', 'Name'] + columns = ['ID', 'Name', 'Volume ID'] + column_headers = ['ID', 'Name', 'Volume'] volume_client = self.app.client_manager.volume volume_transfer_result = volume_client.transfers.list( detailed=True, - search_opts={'all_tenants': parsed_args.all_projects} + search_opts={'all_tenants': parsed_args.all_projects}, ) return (column_headers, ( @@ -165,7 +202,9 @@ class ShowTransferRequest(command.ShowOne): def take_action(self, parsed_args): volume_client = self.app.client_manager.volume volume_transfer_request = utils.find_resource( - volume_client.transfers, parsed_args.transfer_request) + volume_client.transfers, + parsed_args.transfer_request, + ) volume_transfer_request._info.pop("links", None) return zip(*sorted(six.iteritems(volume_transfer_request._info))) diff --git a/openstackclient/volume/v2/volume_transfer_request.py b/openstackclient/volume/v2/volume_transfer_request.py index aefe594a..2f531dc8 100644 --- a/openstackclient/volume/v2/volume_transfer_request.py +++ b/openstackclient/volume/v2/volume_transfer_request.py @@ -14,6 +14,7 @@ """Volume v2 transfer action implementations""" +import argparse import logging from osc_lib.command import command @@ -34,22 +35,54 @@ class AcceptTransferRequest(command.ShowOne): parser = super(AcceptTransferRequest, self).get_parser(prog_name) parser.add_argument( 'transfer_request', - metavar="", - help=_('Volume transfer request to accept (name or ID)'), + metavar="", + help=_('Volume transfer request to accept (ID only)'), + ) + parser.add_argument( + 'old_auth_key', + metavar="", + nargs="?", + help=argparse.SUPPRESS, ) parser.add_argument( - 'auth_key', - metavar="", - help=_('Authentication key of transfer request'), + '--auth-key', + metavar="", + help=_('Volume transfer request authentication key'), ) return parser def take_action(self, parsed_args): volume_client = self.app.client_manager.volume - transfer_request_id = utils.find_resource( - volume_client.transfers, parsed_args.transfer_request).id + + try: + transfer_request_id = utils.find_resource( + volume_client.transfers, + parsed_args.transfer_request + ).id + except exceptions.CommandError: + # Non-admin users will fail to lookup name -> ID so we just + # move on and attempt with the user-supplied information + transfer_request_id = parsed_args.transfer_request + + # Remain backward-compatible for the previous command layout + # TODO(dtroyer): Remove this back-compat in 4.0 or Oct 2017 + if not parsed_args.auth_key: + if parsed_args.old_auth_key: + # Move the old one into the correct place + parsed_args.auth_key = parsed_args.old_auth_key + self.log.warning(_( + 'Specifying the auth-key as a positional argument ' + 'has been deprecated. Please use the --auth-key ' + 'option in the future.' + )) + else: + msg = _("argument --auth-key is required") + raise exceptions.CommandError(msg) + transfer_accept = volume_client.transfers.accept( - transfer_request_id, parsed_args.auth_key) + transfer_request_id, + parsed_args.auth_key, + ) transfer_accept._info.pop("links", None) return zip(*sorted(six.iteritems(transfer_accept._info))) @@ -75,9 +108,12 @@ class CreateTransferRequest(command.ShowOne): def take_action(self, parsed_args): volume_client = self.app.client_manager.volume volume_id = utils.find_resource( - volume_client.volumes, parsed_args.volume).id + volume_client.volumes, + parsed_args.volume, + ).id volume_transfer_request = volume_client.transfers.create( - volume_id, parsed_args.name, + volume_id, + parsed_args.name, ) volume_transfer_request._info.pop("links", None) @@ -104,7 +140,9 @@ class DeleteTransferRequest(command.Command): for t in parsed_args.transfer_request: try: transfer_request_id = utils.find_resource( - volume_client.transfers, t).id + volume_client.transfers, + t, + ).id volume_client.transfers.delete(transfer_request_id) except Exception as e: result += 1 @@ -115,7 +153,7 @@ class DeleteTransferRequest(command.Command): if result > 0: total = len(parsed_args.transfer_request) msg = (_("%(result)s of %(total)s volume transfer requests failed" - " to delete.") % {'result': result, 'total': total}) + " to delete") % {'result': result, 'total': total}) raise exceptions.CommandError(msg) @@ -129,20 +167,19 @@ class ListTransferRequest(command.Lister): dest='all_projects', action="store_true", default=False, - help=_('Shows detail for all projects. Admin only. ' - '(defaults to False)') + help=_('Include all projects (admin only)'), ) return parser def take_action(self, parsed_args): - columns = ['ID', 'Volume ID', 'Name'] - column_headers = ['ID', 'Volume', 'Name'] + columns = ['ID', 'Name', 'Volume ID'] + column_headers = ['ID', 'Name', 'Volume'] volume_client = self.app.client_manager.volume volume_transfer_result = volume_client.transfers.list( detailed=True, - search_opts={'all_tenants': parsed_args.all_projects} + search_opts={'all_tenants': parsed_args.all_projects}, ) return (column_headers, ( @@ -165,7 +202,9 @@ class ShowTransferRequest(command.ShowOne): def take_action(self, parsed_args): volume_client = self.app.client_manager.volume volume_transfer_request = utils.find_resource( - volume_client.transfers, parsed_args.transfer_request) + volume_client.transfers, + parsed_args.transfer_request, + ) volume_transfer_request._info.pop("links", None) return zip(*sorted(six.iteritems(volume_transfer_request._info))) -- cgit v1.2.1