From fc7ebb162169bd97959dfa2d37b4e45a433cd716 Mon Sep 17 00:00:00 2001 From: Steven Kaufer Date: Thu, 8 Jan 2015 19:42:30 +0000 Subject: [PATCH] Adopt CLI sorting argument guidelines CLI sorting guidelines are defined in the cross project specs repository: [1] This patch deprecates --sort_key and --sort_dir and adds support for --sort in the 'cinder list' command with the following syntax: cinder list --sort [:] The --sort parameter is comma-separated and is used to specify one or more sort keys and directions. Server-side work for sorting by multiple keys is handled in [2]. [1] https://github.com/openstack/openstack-specs/blob/master/specs/ cli-sorting-args.rst [2] https://review.openstack.org/#/c/141915/ Implements: blueprint cinder-pagination Change-Id: Ie8c2c62b8c129f647f85d66d5bcfe4c8f0f4eedb --- cinderclient/tests/v2/test_shell.py | 44 ++++++++++++- cinderclient/tests/v2/test_volumes.py | 54 ++++++++++++++++ cinderclient/v2/shell.py | 27 +++++--- cinderclient/v2/volumes.py | 89 ++++++++++++++++++++++----- 4 files changed, 189 insertions(+), 25 deletions(-) diff --git a/cinderclient/tests/v2/test_shell.py b/cinderclient/tests/v2/test_shell.py index 92f6aaa6e..b75a2ffe1 100644 --- a/cinderclient/tests/v2/test_shell.py +++ b/cinderclient/tests/v2/test_shell.py @@ -16,6 +16,7 @@ import fixtures import mock from requests_mock.contrib import fixture as requests_mock_fixture +from six.moves.urllib import parse from cinderclient import client from cinderclient import exceptions @@ -108,11 +109,16 @@ class ShellTest(utils.TestCase): self.run_command('list --sort_key=id --sort_dir=asc') self.assert_called('GET', '/volumes/detail?sort_dir=asc&sort_key=id') - def test_list_sort_name(self): + def test_list_sort_key_name(self): # Client 'name' key is mapped to 'display_name' self.run_command('list --sort_key=name') self.assert_called('GET', '/volumes/detail?sort_key=display_name') + def test_list_sort_name(self): + # Client 'name' key is mapped to 'display_name' + self.run_command('list --sort=name') + self.assert_called('GET', '/volumes/detail?sort=display_name') + def test_list_sort_key_invalid(self): self.assertRaises(ValueError, self.run_command, @@ -123,11 +129,45 @@ class ShellTest(utils.TestCase): self.run_command, 'list --sort_key=id --sort_dir=foo') + def test_list_mix_sort_args(self): + cmds = ['list --sort name:desc --sort_key=status', + 'list --sort name:desc --sort_dir=asc', + 'list --sort name:desc --sort_key=status --sort_dir=asc'] + for cmd in cmds: + self.assertRaises(exceptions.CommandError, self.run_command, cmd) + + def test_list_sort_single_key_only(self): + self.run_command('list --sort=id') + self.assert_called('GET', '/volumes/detail?sort=id') + + def test_list_sort_single_key_trailing_colon(self): + self.run_command('list --sort=id:') + self.assert_called('GET', '/volumes/detail?sort=id') + + def test_list_sort_single_key_and_dir(self): + self.run_command('list --sort=id:asc') + url = '/volumes/detail?%s' % parse.urlencode([('sort', 'id:asc')]) + self.assert_called('GET', url) + + def test_list_sort_multiple_keys_only(self): + self.run_command('list --sort=id,status,size') + url = ('/volumes/detail?%s' % + parse.urlencode([('sort', 'id,status,size')])) + self.assert_called('GET', url) + + def test_list_sort_multiple_keys_and_dirs(self): + self.run_command('list --sort=id:asc,status,size:desc') + url = ('/volumes/detail?%s' % + parse.urlencode([('sort', 'id:asc,status,size:desc')])) + self.assert_called('GET', url) + def test_list_reorder_with_sort(self): # sortby_index is None if there is sort information for cmd in ['list --sort_key=name', 'list --sort_dir=asc', - 'list --sort_key=name --sort_dir=asc']: + 'list --sort_key=name --sort_dir=asc', + 'list --sort=name', + 'list --sort=name:asc']: with mock.patch('cinderclient.utils.print_list') as mock_print: self.run_command(cmd) mock_print.assert_called_once_with( diff --git a/cinderclient/tests/v2/test_volumes.py b/cinderclient/tests/v2/test_volumes.py index 2ad289d80..aceb124e5 100644 --- a/cinderclient/tests/v2/test_volumes.py +++ b/cinderclient/tests/v2/test_volumes.py @@ -223,3 +223,57 @@ class VolumesTest(utils.TestCase): def test_get_pools_detail(self): cs.volumes.get_pools('--detail') cs.assert_called('GET', '/scheduler-stats/get_pools?detail=True') + + +class FormatSortParamTestCase(utils.TestCase): + + def test_format_sort_empty_input(self): + for s in [None, '', []]: + self.assertEqual(None, cs.volumes._format_sort_param(s)) + + def test_format_sort_string_single_key(self): + s = 'id' + self.assertEqual('id', cs.volumes._format_sort_param(s)) + + def test_format_sort_string_single_key_and_dir(self): + s = 'id:asc' + self.assertEqual('id:asc', cs.volumes._format_sort_param(s)) + + def test_format_sort_string_multiple(self): + s = 'id:asc,status,size:desc' + self.assertEqual('id:asc,status,size:desc', + cs.volumes._format_sort_param(s)) + + def test_format_sort_string_mappings(self): + s = 'id:asc,name,size:desc' + self.assertEqual('id:asc,display_name,size:desc', + cs.volumes._format_sort_param(s)) + + def test_format_sort_whitespace_trailing_comma(self): + s = ' id : asc ,status, size:desc,' + self.assertEqual('id:asc,status,size:desc', + cs.volumes._format_sort_param(s)) + + def test_format_sort_list_of_strings(self): + s = ['id:asc', 'status', 'size:desc'] + self.assertEqual('id:asc,status,size:desc', + cs.volumes._format_sort_param(s)) + + def test_format_sort_list_of_tuples(self): + s = [('id', 'asc'), 'status', ('size', 'desc')] + self.assertEqual('id:asc,status,size:desc', + cs.volumes._format_sort_param(s)) + + def test_format_sort_list_of_strings_and_tuples(self): + s = [('id', 'asc'), 'status', 'size:desc'] + self.assertEqual('id:asc,status,size:desc', + cs.volumes._format_sort_param(s)) + + def test_format_sort_invalid_direction(self): + for s in ['id:foo', + 'id:asc,status,size:foo', + ['id', 'status', 'size:foo'], + ['id', 'status', ('size', 'foo')]]: + self.assertRaises(ValueError, + cs.volumes._format_sort_param, + s) diff --git a/cinderclient/v2/shell.py b/cinderclient/v2/shell.py index c656248b8..746a87b1d 100644 --- a/cinderclient/v2/shell.py +++ b/cinderclient/v2/shell.py @@ -28,6 +28,7 @@ from cinderclient import exceptions from cinderclient import utils from cinderclient.openstack.common import strutils from cinderclient.v2 import availability_zones +from cinderclient.v2 import volumes def _poll_for_status(poll_fn, obj_id, action, final_ok_states, @@ -179,14 +180,18 @@ def _extract_metadata(args): @utils.arg('--sort_key', metavar='', default=None, - help='Key to be sorted, should be (`id`, `status`, `size`, ' - '`availability_zone`, `name`, `bootable`, `created_at`). ' - 'OPTIONAL: Default=None.') + help=argparse.SUPPRESS) @utils.arg('--sort_dir', metavar='', default=None, - help='Sort direction, should be `desc` or `asc`. ' - 'OPTIONAL: Default=None.') + help=argparse.SUPPRESS) +@utils.arg('--sort', + metavar='[:]', + default=None, + help=(('Comma-separated list of sort keys and directions in the ' + 'form of [:]. ' + 'Valid keys: %s. ' + 'OPTIONAL: Default=None.') % ', '.join(volumes.SORT_KEY_VALUES))) @utils.service_type('volumev2') def do_list(cs, args): """Lists all volumes.""" @@ -201,9 +206,17 @@ def do_list(cs, args): 'status': args.status, 'metadata': _extract_metadata(args) if args.metadata else None, } + + # --sort_key and --sort_dir deprecated in kilo and is not supported + # with --sort + if args.sort and (args.sort_key or args.sort_dir): + raise exceptions.CommandError( + 'The --sort_key and --sort_dir arguments are deprecated and are ' + 'not supported with --sort.') + volumes = cs.volumes.list(search_opts=search_opts, marker=args.marker, limit=args.limit, sort_key=args.sort_key, - sort_dir=args.sort_dir) + sort_dir=args.sort_dir, sort=args.sort) _translate_volume_keys(volumes) # Create a list of servers to which the volume is attached @@ -217,7 +230,7 @@ def do_list(cs, args): else: key_list = ['ID', 'Status', 'Name', 'Size', 'Volume Type', 'Bootable', 'Attached to'] - if args.sort_key or args.sort_dir: + if args.sort_key or args.sort_dir or args.sort: sortby_index = None else: sortby_index = 0 diff --git a/cinderclient/v2/volumes.py b/cinderclient/v2/volumes.py index c726c2af0..28e812954 100644 --- a/cinderclient/v2/volumes.py +++ b/cinderclient/v2/volumes.py @@ -233,8 +233,57 @@ class VolumeManager(base.ManagerWithFind): """ return self._get("/volumes/%s" % volume_id, "volume") + def _format_sort_param(self, sort): + '''Formats the sort information into the sort query string parameter. + + The input sort information can be any of the following: + - Comma-separated string in the form of + - List of strings in the form of + - List of either string keys, or tuples of (key, dir) + + For example, the following import sort values are valid: + - 'key1:dir1,key2,key3:dir3' + - ['key1:dir1', 'key2', 'key3:dir3'] + - [('key1', 'dir1'), 'key2', ('key3', dir3')] + + :param sort: Input sort information + :returns: Formatted query string parameter or None + :raise ValueError: If an invalid sort direction or invalid sort key is + given + ''' + if not sort: + return None + + if isinstance(sort, six.string_types): + # Convert the string into a list for consistent validation + sort = [s for s in sort.split(',') if s] + + sort_array = [] + for sort_item in sort: + if isinstance(sort_item, tuple): + sort_key = sort_item[0] + sort_dir = sort_item[1] + else: + sort_key, _sep, sort_dir = sort_item.partition(':') + sort_key = sort_key.strip() + if sort_key in SORT_KEY_VALUES: + sort_key = SORT_KEY_MAPPINGS.get(sort_key, sort_key) + else: + raise ValueError('sort_key must be one of the following: %s.' + % ', '.join(SORT_KEY_VALUES)) + if sort_dir: + sort_dir = sort_dir.strip() + if sort_dir not in SORT_DIR_VALUES: + msg = ('sort_dir must be one of the following: %s.' + % ', '.join(SORT_DIR_VALUES)) + raise ValueError(msg) + sort_array.append('%s:%s' % (sort_key, sort_dir)) + else: + sort_array.append(sort_key) + return ','.join(sort_array) + def list(self, detailed=True, search_opts=None, marker=None, limit=None, - sort_key=None, sort_dir=None): + sort_key=None, sort_dir=None, sort=None): """Lists all volumes. :param detailed: Whether to return detailed volume info. @@ -242,8 +291,10 @@ class VolumeManager(base.ManagerWithFind): :param marker: Begin returning volumes that appear later in the volume list than that represented by this volume id. :param limit: Maximum number of volumes to return. - :param sort_key: Key to be sorted. - :param sort_dir: Sort direction, should be 'desc' or 'asc'. + :param sort_key: Key to be sorted; deprecated in kilo + :param sort_dir: Sort direction, should be 'desc' or 'asc'; deprecated + in kilo + :param sort: Sort information :rtype: list of :class:`Volume` """ if search_opts is None: @@ -261,19 +312,25 @@ class VolumeManager(base.ManagerWithFind): if limit: qparams['limit'] = limit - if sort_key is not None: - if sort_key in SORT_KEY_VALUES: - qparams['sort_key'] = SORT_KEY_MAPPINGS.get(sort_key, sort_key) - else: - raise ValueError('sort_key must be one of the following: %s.' - % ', '.join(SORT_KEY_VALUES)) - - if sort_dir is not None: - if sort_dir in SORT_DIR_VALUES: - qparams['sort_dir'] = sort_dir - else: - raise ValueError('sort_dir must be one of the following: %s.' - % ', '.join(SORT_DIR_VALUES)) + # sort_key and sort_dir deprecated in kilo, prefer sort + if sort: + qparams['sort'] = self._format_sort_param(sort) + else: + if sort_key is not None: + if sort_key in SORT_KEY_VALUES: + qparams['sort_key'] = SORT_KEY_MAPPINGS.get(sort_key, + sort_key) + else: + msg = ('sort_key must be one of the following: %s.' + % ', '.join(SORT_KEY_VALUES)) + raise ValueError(msg) + if sort_dir is not None: + if sort_dir in SORT_DIR_VALUES: + qparams['sort_dir'] = sort_dir + else: + msg = ('sort_dir must be one of the following: %s.' + % ', '.join(SORT_DIR_VALUES)) + raise ValueError(msg) # Transform the dict to a sequence of two-element tuples in fixed # order, then the encoded string will be consistent in Python 2&3.