openstack/python-cinderclient
Code Issues Proposed changes

Simplify cinder manage command args

Browse Source 
The 'cinder manage' command argumenst are kind of a disaster.
The positionals are <host> <id> where <id> is undefined/free form
K/V pairs.  On top of it we then have --source-name and --source-id
optional arguments available.  There absolutely no way to tell from
the help what one should be entering here without referring to the
code, and even then it's not quite clear how the options work and
which one will be used.

To clean this up, change the positionals to <host> <identifier>
where both are strings.  Remove --source-name and --source-id
and consolidate those options with --id-type.

id-type will allow a user to specify the type of identifier
(source-name, source-id, or whatever), but defaults to
source-name which is most commonly used currently.

* NOTE *
Ideally we'd just do away with the designator source-name/source-id
altogether and just have an <identifier> positional arg.  Each driver
would then be responsible for figuring out what was passed an how to
deal with it.  This however requires changes in all of the drivers and
broader changes on the Cinder side which I don't think are warranted
during RC.

During Kilo we should clean all of this up, but for now at least the
syntax is somewhat ready for human consumption and a bit easier than
what we introduced.

Change-Id: I07696648ae647f17ab9180cd87b25f8cb888f5d6
Closes-Bug: 1376311
This commit is contained in:
John Griffith
2014-10-01 20:18:12 +00:00
parent 26e19ff186
commit 1cb1350d16
2 changed files with 26 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

21
cinderclient/tests/v2/test_shell.py
View File

@@ -445,12 +445,12 @@ class ShellTest(utils.TestCase):
self.assert_called('DELETE', '/snapshots/5678') self.assert_called('DELETE', '/snapshots/5678')
def test_volume_manage(self): def test_volume_manage(self):
self.run_command('manage host1 key1=val1 key2=val2 ' self.run_command('manage host1 some_fake_name '
'--name foo --description bar ' '--name foo --description bar '
'--volume-type baz --availability-zone az ' '--volume-type baz --availability-zone az '
'--metadata k1=v1 k2=v2') '--metadata k1=v1 k2=v2')
expected = {'volume': {'host': 'host1', expected = {'volume': {'host': 'host1',
'ref': {'key1': 'val1', 'key2': 'val2'}, 'ref': {'source-name': 'some_fake_name'},
'name': 'foo', 'name': 'foo',
'description': 'bar', 'description': 'bar',
'volume_type': 'baz', 'volume_type': 'baz',
@@ -466,12 +466,12 @@ class ShellTest(utils.TestCase):
If this flag is specified, then the resulting POST should contain If this flag is specified, then the resulting POST should contain
bootable: True. bootable: True.
""" """
self.run_command('manage host1 key1=val1 key2=val2 ' self.run_command('manage host1 some_fake_name '
'--name foo --description bar --bootable ' '--name foo --description bar --bootable '
'--volume-type baz --availability-zone az ' '--volume-type baz --availability-zone az '
'--metadata k1=v1 k2=v2') '--metadata k1=v1 k2=v2')
expected = {'volume': {'host': 'host1', expected = {'volume': {'host': 'host1',
'ref': {'key1': 'val1', 'key2': 'val2'}, 'ref': {'source-name': 'some_fake_name'},
'name': 'foo', 'name': 'foo',
'description': 'bar', 'description': 'bar',
'volume_type': 'baz', 'volume_type': 'baz',
@@ -487,14 +487,12 @@ class ShellTest(utils.TestCase):
Checks that the --source-name option correctly updates the Checks that the --source-name option correctly updates the
ref structure that is passed in the HTTP POST ref structure that is passed in the HTTP POST
""" """
self.run_command('manage host1 key1=val1 key2=val2 ' self.run_command('manage host1 VolName '
'--source-name VolName '
'--name foo --description bar ' '--name foo --description bar '
'--volume-type baz --availability-zone az ' '--volume-type baz --availability-zone az '
'--metadata k1=v1 k2=v2') '--metadata k1=v1 k2=v2')
expected = {'volume': {'host': 'host1', expected = {'volume': {'host': 'host1',
'ref': {'source-name': 'VolName', 'ref': {'source-name': 'VolName'},
'key1': 'val1', 'key2': 'val2'},
'name': 'foo', 'name': 'foo',
'description': 'bar', 'description': 'bar',
'volume_type': 'baz', 'volume_type': 'baz',
@@ -510,14 +508,13 @@ class ShellTest(utils.TestCase):
Checks that the --source-id option correctly updates the Checks that the --source-id option correctly updates the
ref structure that is passed in the HTTP POST ref structure that is passed in the HTTP POST
""" """
self.run_command('manage host1 key1=val1 key2=val2 ' self.run_command('manage host1 1234 '
'--source-id 1234 ' '--id-type source-id '
'--name foo --description bar ' '--name foo --description bar '
'--volume-type baz --availability-zone az ' '--volume-type baz --availability-zone az '
'--metadata k1=v1 k2=v2') '--metadata k1=v1 k2=v2')
expected = {'volume': {'host': 'host1', expected = {'volume': {'host': 'host1',
'ref': {'source-id': '1234', 'ref': {'source-id': '1234'},
'key1': 'val1', 'key2': 'val2'},
'name': 'foo', 'name': 'foo',
'description': 'bar', 'description': 'bar',
'volume_type': 'baz', 'volume_type': 'baz',

38
cinderclient/v2/shell.py
View File

@@ -1621,36 +1621,33 @@ def do_set_bootable(cs, args):
@utils.arg('host', @utils.arg('host',
metavar='<host>', metavar='<host>',
help='Cinder host on which the existing volume resides') help='Cinder host on which the existing volume resides; '
@utils.arg('ref', 'takes the form: host@backend-name#pool')
type=str, @utils.arg('identifier',
nargs='*', metavar='<identifier>',
metavar='<key=value>', help='Name or other Identifier for existing volume')
help='Driver-specific reference to the existing volume as ' @utils.arg('--id-type',
'key=value pairs') metavar='<id-type>',
@utils.arg('--source-name', default='source-name',
metavar='<source-name>', help='Type of backend device identifier provided, '
help='Name of the volume to manage (Optional)') 'typically source-name or source-id (Default=source-name)')
@utils.arg('--source-id',
metavar='<source-id>',
help='ID of the volume to manage (Optional)')
@utils.arg('--name', @utils.arg('--name',
metavar='<name>', metavar='<name>',
help='Volume name (Optional, Default=None)') help='Volume name (Default=None)')
@utils.arg('--description', @utils.arg('--description',
metavar='<description>', metavar='<description>',
help='Volume description (Optional, Default=None)') help='Volume description (Default=None)')
@utils.arg('--volume-type', @utils.arg('--volume-type',
metavar='<volume-type>', metavar='<volume-type>',
help='Volume type (Optional, Default=None)') help='Volume type (Default=None)')
@utils.arg('--availability-zone', @utils.arg('--availability-zone',
metavar='<availability-zone>', metavar='<availability-zone>',
help='Availability zone for volume (Optional, Default=None)') help='Availability zone for volume (Default=None)')
@utils.arg('--metadata', @utils.arg('--metadata',
type=str, type=str,
nargs='*', nargs='*',
metavar='<key=value>', metavar='<key=value>',
help='Metadata key=value pairs (Optional, Default=None)') help='Metadata key=value pairs (Default=None)')
@utils.arg('--bootable', @utils.arg('--bootable',
action='store_true', action='store_true',
help='Specifies that the newly created volume should be' help='Specifies that the newly created volume should be'
@@ -1664,9 +1661,7 @@ def do_manage(cs, args):
# Build a dictionary of key/value pairs to pass to the API. # Build a dictionary of key/value pairs to pass to the API.
ref_dict = {} ref_dict = {}
for pair in args.ref: ref_dict[args.id_type] = args.identifier
(k, v) = pair.split('=', 1)
ref_dict[k] = v
# The recommended way to specify an existing volume is by ID or name, and # The recommended way to specify an existing volume is by ID or name, and
# have the Cinder driver look for 'source-name' or 'source-id' elements in # have the Cinder driver look for 'source-name' or 'source-id' elements in
@@ -1677,6 +1672,7 @@ def do_manage(cs, args):
# Note how argparse converts hyphens to underscores. We use hyphens in the # Note how argparse converts hyphens to underscores. We use hyphens in the
# dictionary so that it is consistent with what the user specified on the # dictionary so that it is consistent with what the user specified on the
# CLI. # CLI.
if hasattr(args, 'source_name') and \ if hasattr(args, 'source_name') and \
args.source_name is not None: args.source_name is not None:
ref_dict['source-name'] = args.source_name ref_dict['source-name'] = args.source_name