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

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: <tbd>
+
 Release 3.0
 -----------
 

doc/source/command-objects/volume-transfer-request.rst

@@ -13,17 +13,19 @@ Accept volume transfer request
 .. code:: bash
 
     openstack volume transfer request accept
-        <transfer-request>
-        <auth-key>
+        --auth-key <key>
+        <transfer-request-id>
 
-.. _volume_transfer_request_accept:
-.. describe:: <transfer-request>
+.. option:: --auth-key <key>
 
-    Volume transfer request to accept (name or ID)
+    Volume transfer request authentication key
+
+.. _volume_transfer_request_accept:
+.. describe:: <transfer-request-id>
 
-.. describe:: <auth-key>
+    Volume transfer request to accept (ID only)
 
-    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
 ----------------------------

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

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

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.

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.

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="<transfer-request>",
-            help=_('Volume transfer request to accept (name or ID)'),
+            metavar="<transfer-request-id>",
+            help=_('Volume transfer request to accept (ID only)'),
+        )
+        parser.add_argument(
+            'old_auth_key',
+            metavar="<key>",
+            nargs="?",
+            help=argparse.SUPPRESS,
         )
         parser.add_argument(
-            'auth_key',
-            metavar="<auth-key>",
-            help=_('Authentication key of transfer request'),
+            '--auth-key',
+            metavar="<key>",
+            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)))

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="<transfer-request>",
-            help=_('Volume transfer request to accept (name or ID)'),
+            metavar="<transfer-request-id>",
+            help=_('Volume transfer request to accept (ID only)'),
+        )
+        parser.add_argument(
+            'old_auth_key',
+            metavar="<key>",
+            nargs="?",
+            help=argparse.SUPPRESS,
         )
         parser.add_argument(
-            'auth_key',
-            metavar="<auth-key>",
-            help=_('Authentication key of transfer request'),
+            '--auth-key',
+            metavar="<key>",
+            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)))

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 <https://bugs.launchpad.net/python-openstackclient/+bug/1633582>`_]
+  - |
+    Change the output column order in ``volume transfer request list`` to have
+    ``ID`` followed by ``Name`` then the remaining columns.