diff --git a/doc/source/backwards-incompatible.rst b/doc/source/backwards-incompatible.rst index 4f38aa540..bce867173 100644 --- a/doc/source/backwards-incompatible.rst +++ b/doc/source/backwards-incompatible.rst @@ -16,6 +16,17 @@ this backwards incompatible change handling. Backwards Incompatible Changes ============================== +.. Carry this section as comments until 4.0 release +.. Release 4.0 +.. ----------- + +.. 1. Change ``volume transfer request accept`` to use new option ``--auth-key`` +.. rather than a second positional argument. + +.. * As of: 4.0 +.. * Remove in: <5.0> +.. * Commit: + Release 3.0 ----------- diff --git a/doc/source/command-objects/volume-transfer-request.rst b/doc/source/command-objects/volume-transfer-request.rst index 77c364296..23cd3d3e1 100644 --- a/doc/source/command-objects/volume-transfer-request.rst +++ b/doc/source/command-objects/volume-transfer-request.rst @@ -13,17 +13,19 @@ Accept volume transfer request .. code:: bash openstack volume transfer request accept - - + --auth-key + + +.. option:: --auth-key + + Volume transfer request authentication key .. _volume_transfer_request_accept: -.. describe:: +.. describe:: - Volume transfer request to accept (name or ID) + Volume transfer request to accept (ID only) -.. describe:: - - Authentication key of transfer request + Non-admin users are only able to specify the transfer request by ID. volume transfer request create ------------------------------ @@ -65,7 +67,7 @@ Delete volume transfer request(s) volume transfer request list ---------------------------- -Lists all volume transfer requests. +Lists all volume transfer requests .. program:: volume transfer request list .. code:: bash @@ -75,8 +77,7 @@ Lists all volume transfer requests. .. option:: --all-projects - Shows detail for all projects. Admin only. - (defaults to False) + Include all projects (admin only) volume transfer request show ---------------------------- diff --git a/openstackclient/tests/functional/volume/v1/test_transfer_request.py b/openstackclient/tests/functional/volume/v1/test_transfer_request.py index e03cd717c..3fe11913f 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 1791f8ac1..99d91ac0e 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 b3788d6ec..4c013dc00 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 8cd6534ba..37eed11e5 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 f24d5a566..f5b567b94 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( - 'auth_key', - metavar="", - help=_('Authentication key of transfer request'), + 'old_auth_key', + metavar="", + nargs="?", + help=argparse.SUPPRESS, + ) + parser.add_argument( + '--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 aefe594a3..2f531dc87 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( - 'auth_key', - metavar="", - help=_('Authentication key of transfer request'), + 'old_auth_key', + metavar="", + nargs="?", + help=argparse.SUPPRESS, + ) + parser.add_argument( + '--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/releasenotes/notes/bug-1633582-df2bee534c2da7fc.yaml b/releasenotes/notes/bug-1633582-df2bee534c2da7fc.yaml new file mode 100644 index 000000000..f99457e4d --- /dev/null +++ b/releasenotes/notes/bug-1633582-df2bee534c2da7fc.yaml @@ -0,0 +1,17 @@ +--- +deprecations: + - | + ``volume transfer request accept`` has been changed to move the ``auth-key`` + positional argument to a requried option ``--auth-key``. This leaves + the transfer request ID as the only positional arguemnt, as per the + OpenStackClient command format. The old format is still functional, but is + dperecated and will be removed in the next major release. +fixes: + - | + Fix ``volume transfer request accept`` to not fail the transfer request + name/ID lookup for non-admin users as the Volume API does not allow non-admin + users access to transfers in other projects. + [Bug `1633582 `_] + - | + Change the output column order in ``volume transfer request list`` to have + ``ID`` followed by ``Name`` then the remaining columns.