openstack/python-cinderclient
Code Issues Proposed changes

Merge "Optional filters parameters should be passed only once"

Browse Source
This commit is contained in:
Zuul 2019-09-11 14:16:09 +00:00 committed by Gerrit Code Review
commit bafd84b9fa
4 changed files with 136 additions and 21 deletions

47
cinderclient/shell.py

@ -54,6 +54,33 @@ V3_SHELL = 'cinderclient.v3.shell'
HINT_HELP_MSG = (" [hint: use '--os-volume-api-version' flag to show help "
"message for proper version]")
FILTER_CHECK = ["type-list",
"backup-list",
"get-pools",
"list",
"group-list",
"group-snapshot-list",
"message-list",
"snapshot-list",
"attachment-list"]
RESOURCE_FILTERS = {
"list": ["name", "status", "metadata",
"bootable", "migration_status", "availability_zone",
"group_id", "size"],
"backup-list": ["name", "status", "volume_id"],
"snapshot-list": ["name", "status", "volume_id", "metadata",
"availability_zone"],
"group-list": ["name"],
"group-snapshot-list": ["name", "status", "group_id"],
"attachment-list": ["volume_id", "status", "instance_id", "attach_status"],
"message-list": ["resource_uuid", "resource_type", "event_id",
"request_id", "message_level"],
"get-pools": ["name", "volume_type"],
"type-list": ["is_public"]
}
logging.basicConfig()
logger = logging.getLogger(__name__)
@ -521,8 +548,28 @@ class OpenStackCinderShell(object):
logger.warning("downgrading to %s based on server support." %
discovered.get_string())
def check_duplicate_filters(self, argv, filter):
resource = RESOURCE_FILTERS[filter]
filters = []
for opt in range(len(argv)):
if argv[opt].startswith('--'):
if argv[opt] == '--filters':
key, __ = argv[opt + 1].split('=')
if key in resource:
filters.append(key)
elif argv[opt][2:] in resource:
filters.append(argv[opt][2:])
if len(set(filters)) != len(filters):
raise exc.CommandError(
"Filters are only allowed to be passed once.")
def main(self, argv):
# Parse args once to find version and debug settings
for filter in FILTER_CHECK:
if filter in argv:
self.check_duplicate_filters(argv, filter)
break
parser = self.get_base_parser()
(options, args) = parser.parse_known_args(argv)
self.setup_debugging(options.debug)

6
cinderclient/tests/unit/test_shell.py

@ -201,6 +201,12 @@ class ShellTest(utils.TestCase):
_shell = shell.OpenStackCinderShell()
_shell.main(['list'])
def test_duplicate_filters(self):
_shell = shell.OpenStackCinderShell()
self.assertRaises(exceptions.CommandError,
_shell.main,
['list', '--name', 'abc', '--filters', 'name=xyz'])
@unittest.skip("Skip cuz I broke it")
def test_cinder_service_name(self):
# Failing with 'No mock address' means we are not

36
cinderclient/tests/unit/v3/test_shell.py

@ -145,6 +145,10 @@ class ShellTest(utils.TestCase):
u'list --filters name~=Σ',
'expected':
'/volumes/detail?name~=%CE%A3'},
{'command':
u'list --filters name=abc --filters size=1',
'expected':
'/volumes/detail?name=abc&size=1'},
# testcases for list group
{'command':
'group-list --filters name=456',
@ -158,6 +162,10 @@ class ShellTest(utils.TestCase):
'group-list --filters name~=456',
'expected':
'/groups/detail?name~=456'},
{'command':
'group-list --filters name=abc --filters status=available',
'expected':
'/groups/detail?name=abc&status=available'},
# testcases for list group-snapshot
{'command':
'group-snapshot-list --status=error --filters status=available',
@ -171,6 +179,11 @@ class ShellTest(utils.TestCase):
'group-snapshot-list --filters status~=available',
'expected':
'/group_snapshots/detail?status~=available'},
{'command':
'group-snapshot-list --filters status=available '
'--filters availability_zone=123',
'expected':
'/group_snapshots/detail?availability_zone=123&status=available'},
# testcases for list message
{'command':
'message-list --event_id=123 --filters event_id=456',
@ -184,6 +197,10 @@ class ShellTest(utils.TestCase):
'message-list --filters request_id~=123',
'expected':
'/messages?request_id~=123'},
{'command':
'message-list --filters request_id=123 --filters event_id=456',
'expected':
'/messages?event_id=456&request_id=123'},
# testcases for list attachment
{'command':
'attachment-list --volume-id=123 --filters volume_id=456',
@ -197,6 +214,11 @@ class ShellTest(utils.TestCase):
'attachment-list --filters volume_id~=456',
'expected':
'/attachments?volume_id~=456'},
{'command':
'attachment-list --filters volume_id=123 '
'--filters mountpoint=456',
'expected':
'/attachments?mountpoint=456&volume_id=123'},
# testcases for list backup
{'command':
'backup-list --volume-id=123 --filters volume_id=456',
@ -210,6 +232,10 @@ class ShellTest(utils.TestCase):
'backup-list --filters volume_id~=456',
'expected':
'/backups/detail?volume_id~=456'},
{'command':
'backup-list --filters volume_id=123 --filters name=456',
'expected':
'/backups/detail?name=456&volume_id=123'},
# testcases for list snapshot
{'command':
'snapshot-list --volume-id=123 --filters volume_id=456',
@ -223,6 +249,10 @@ class ShellTest(utils.TestCase):
'snapshot-list --filters volume_id~=456',
'expected':
'/snapshots/detail?volume_id~=456'},
{'command':
'snapshot-list --filters volume_id=123 --filters name=456',
'expected':
'/snapshots/detail?name=456&volume_id=123'},
# testcases for get pools
{'command':
'get-pools --filters name=456 --detail',
@ -231,7 +261,11 @@ class ShellTest(utils.TestCase):
{'command':
'get-pools --filters name=456',
'expected':
'/scheduler-stats/get_pools?name=456'}
'/scheduler-stats/get_pools?name=456'},
{'command':
'get-pools --filters name=456 --filters detail=True',
'expected':
'/scheduler-stats/get_pools?detail=True&name=456'}
)
@ddt.unpack
def test_list_with_filters_mixed(self, command, expected):

68
cinderclient/v3/shell.py

@ -37,6 +37,14 @@ FILTER_DEPRECATED = ("This option is deprecated and will be removed in "
"is introduced since 3.33 instead.")
class AppendFilters(argparse.Action):
filters = []
def __call__(self, parser, namespace, values, option_string):
AppendFilters.filters.append(values[0])
@api_versions.wraps('3.33')
@utils.arg('--resource',
metavar='<resource>',
@ -52,6 +60,7 @@ def do_list_filters(cs, args):
@utils.arg('--filters',
action=AppendFilters,
type=six.text_type,
nargs='*',
start_version='3.52',
@ -66,8 +75,8 @@ def do_type_list(cs, args):
# pylint: disable=function-redefined
search_opts = {}
# Update search option with `filters`
if hasattr(args, 'filters') and args.filters is not None:
search_opts.update(shell_utils.extract_filters(args.filters))
if AppendFilters.filters:
search_opts.update(shell_utils.extract_filters(AppendFilters.filters))
vtypes = cs.volume_types.list(search_opts=search_opts)
shell_utils.print_volume_type_list(vtypes)
@ -83,6 +92,7 @@ def do_type_list(cs, args):
mode="w"):
for vtype in vtypes:
cs.volume_types.write_to_completion_cache('name', vtype.name)
AppendFilters.filters = []
@utils.arg('--all-tenants',
@ -132,6 +142,7 @@ def do_type_list(cs, args):
'Valid keys: %s. '
'Default=None.') % ', '.join(base.SORT_KEY_VALUES)))
@utils.arg('--filters',
action=AppendFilters,
type=six.text_type,
nargs='*',
start_version='3.33',
@ -163,8 +174,8 @@ def do_backup_list(cs, args):
}
# Update search option with `filters`
if hasattr(args, 'filters') and args.filters is not None:
search_opts.update(shell_utils.extract_filters(args.filters))
if AppendFilters.filters:
search_opts.update(shell_utils.extract_filters(AppendFilters.filters))
total_count = 0
if show_count:
@ -205,12 +216,14 @@ def do_backup_list(cs, args):
for backup in backups:
if backup.name is not None:
cs.backups.write_to_completion_cache('name', backup.name)
AppendFilters.filters = []
@utils.arg('--detail',
action='store_true',
help='Show detailed information about pools.')
@utils.arg('--filters',
action=AppendFilters,
type=six.text_type,
nargs='*',
start_version='3.33',
@ -223,8 +236,8 @@ def do_get_pools(cs, args):
# pylint: disable=function-redefined
search_opts = {}
# Update search option with `filters`
if hasattr(args, 'filters') and args.filters is not None:
search_opts.update(shell_utils.extract_filters(args.filters))
if AppendFilters.filters:
search_opts.update(shell_utils.extract_filters(AppendFilters.filters))
if cs.api_version >= api_versions.APIVersion("3.33"):
pools = cs.volumes.get_pools(args.detail, search_opts)
else:
@ -238,6 +251,7 @@ def do_get_pools(cs, args):
if args.detail:
backend.update(info['capabilities'])
utils.print_dict(backend)
AppendFilters.filters = []
RESET_STATE_RESOURCES = {'volume': utils.find_volume,
@ -345,6 +359,7 @@ RESET_STATE_RESOURCES = {'volume': utils.find_volume,
metavar='<tenant>',
help='Display information from single tenant (Admin only).')
@utils.arg('--filters',
action=AppendFilters,
type=six.text_type,
nargs='*',
start_version='3.33',
@ -387,8 +402,8 @@ def do_list(cs, args):
'group_id': getattr(args, 'group_id', None),
}
# Update search option with `filters`
if hasattr(args, 'filters') and args.filters is not None:
search_opts.update(shell_utils.extract_filters(args.filters))
if AppendFilters.filters:
search_opts.update(shell_utils.extract_filters(AppendFilters.filters))
# If unavailable/non-existent fields are specified, these fields will
# be removed from key_list at the print_list() during key validation.
@ -458,6 +473,7 @@ def do_list(cs, args):
sortby_index=sortby_index)
if show_count:
print("Volume in total: %s" % total_count)
AppendFilters.filters = []
@utils.arg('entity', metavar='<entity>', nargs='+',
@ -754,6 +770,7 @@ def do_summary(cs, args):
@api_versions.wraps('3.11')
@utils.arg('--filters',
action=AppendFilters,
type=six.text_type,
nargs='*',
start_version='3.52',
@ -764,10 +781,11 @@ def do_group_type_list(cs, args):
"""Lists available 'group types'. (Admin only will see private types)"""
search_opts = {}
# Update search option with `filters`
if hasattr(args, 'filters') and args.filters is not None:
search_opts.update(shell_utils.extract_filters(args.filters))
if AppendFilters.filters:
search_opts.update(shell_utils.extract_filters(AppendFilters.filters))
gtypes = cs.group_types.list(search_opts=search_opts)
shell_utils.print_group_type_list(gtypes)
AppendFilters.filters = []
@api_versions.wraps('3.11')
@ -1342,6 +1360,7 @@ def do_manageable_list(cs, args):
default=utils.env('ALL_TENANTS', default=None),
help='Shows details for all tenants. Admin only.')
@utils.arg('--filters',
action=AppendFilters,
type=six.text_type,
nargs='*',
start_version='3.33',
@ -1355,8 +1374,8 @@ def do_group_list(cs, args):
search_opts = {'all_tenants': args.all_tenants}
# Update search option with `filters`
if hasattr(args, 'filters') and args.filters is not None:
search_opts.update(shell_utils.extract_filters(args.filters))
if AppendFilters.filters:
search_opts.update(shell_utils.extract_filters(AppendFilters.filters))
groups = cs.groups.list(search_opts=search_opts)
@ -1376,6 +1395,7 @@ def do_group_list(cs, args):
if group.name is None:
continue
cs.groups.write_to_completion_cache('name', group.name)
AppendFilters.filters = []
@api_versions.wraps('3.13')
@ -1652,6 +1672,7 @@ def do_group_list_replication_targets(cs, args):
help="Filters results by a group ID. Default=None. "
"%s" % FILTER_DEPRECATED)
@utils.arg('--filters',
action=AppendFilters,
type=six.text_type,
nargs='*',
start_version='3.33',
@ -1669,13 +1690,14 @@ def do_group_snapshot_list(cs, args):
'group_id': args.group_id,
}
# Update search option with `filters`
if hasattr(args, 'filters') and args.filters is not None:
search_opts.update(shell_utils.extract_filters(args.filters))
if AppendFilters.filters:
search_opts.update(shell_utils.extract_filters(AppendFilters.filters))
group_snapshots = cs.group_snapshots.list(search_opts=search_opts)
columns = ['ID', 'Status', 'Name']
utils.print_list(group_snapshots, columns)
AppendFilters.filters = []
@api_versions.wraps('3.14')
@ -1902,6 +1924,7 @@ def do_revert_to_snapshot(cs, args):
help="Filters results by the message level. Default=None. "
"%s" % FILTER_DEPRECATED)
@utils.arg('--filters',
action=AppendFilters,
type=six.text_type,
nargs='*',
start_version='3.33',
@ -1918,8 +1941,8 @@ def do_message_list(cs, args):
'request_id': args.request_id,
}
# Update search option with `filters`
if hasattr(args, 'filters') and args.filters is not None:
search_opts.update(shell_utils.extract_filters(args.filters))
if AppendFilters.filters:
search_opts.update(shell_utils.extract_filters(AppendFilters.filters))
if args.resource_type:
search_opts['resource_type'] = args.resource_type.upper()
if args.level:
@ -1941,6 +1964,7 @@ def do_message_list(cs, args):
else:
sortby_index = 0
utils.print_list(messages, columns, sortby_index=sortby_index)
AppendFilters.filters = []
@api_versions.wraps("3.3")
@ -2040,6 +2064,7 @@ def do_message_delete(cs, args):
"volume api version >=3.22. Default=None. "
"%s" % FILTER_DEPRECATED)
@utils.arg('--filters',
action=AppendFilters,
type=six.text_type,
nargs='*',
start_version='3.33',
@ -2085,8 +2110,8 @@ def do_snapshot_list(cs, args):
}
# Update search option with `filters`
if hasattr(args, 'filters') and args.filters is not None:
search_opts.update(shell_utils.extract_filters(args.filters))
if AppendFilters.filters:
search_opts.update(shell_utils.extract_filters(AppendFilters.filters))
total_count = 0
if show_count:
@ -2122,6 +2147,7 @@ def do_snapshot_list(cs, args):
mode='w'):
for snapshot in snapshots:
cs.volume_snapshots.write_to_completion_cache('uuid', snapshot.id)
AppendFilters.filters = []
@api_versions.wraps('3.27')
@ -2167,6 +2193,7 @@ def do_snapshot_list(cs, args):
metavar='<tenant>',
help='Display information from single tenant (Admin only).')
@utils.arg('--filters',
action=AppendFilters,
type=six.text_type,
nargs='*',
start_version='3.33',
@ -2184,8 +2211,8 @@ def do_attachment_list(cs, args):
'volume_id': args.volume_id,
}
# Update search option with `filters`
if hasattr(args, 'filters') and args.filters is not None:
search_opts.update(shell_utils.extract_filters(args.filters))
if AppendFilters.filters:
search_opts.update(shell_utils.extract_filters(AppendFilters.filters))
attachments = cs.attachments.list(search_opts=search_opts,
marker=args.marker,
@ -2199,6 +2226,7 @@ def do_attachment_list(cs, args):
else:
sortby_index = 0
utils.print_list(attachments, columns, sortby_index=sortby_index)
AppendFilters.filters = []
@api_versions.wraps('3.27')