openstack/python-openstackclient
Code Issues Proposed changes

image: Make better use of argparse

Browse Source 
Simplify some logic by using a common 'dest' for mutually exclusive
options.

Change-Id: Ie5f3600672953f40be52de51e84717c8912ddaf8
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>
This commit is contained in:
Stephen Finucane 2022-06-30 11:10:18 +01:00
parent 2290b38ab3
commit 4776e0a5ae
2 changed files with 116 additions and 149 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

149
openstackclient/image/v2/image.py
View File

@ -325,32 +325,44 @@ class CreateImage(command.ShowOne):
protected_group.add_argument( protected_group.add_argument(
"--protected", "--protected",
action="store_true", action="store_true",
dest="is_protected",
default=None,
help=_("Prevent image from being deleted"), help=_("Prevent image from being deleted"),
) )
protected_group.add_argument( protected_group.add_argument(
"--unprotected", "--unprotected",
action="store_true", action="store_false",
dest="is_protected",
default=None,
help=_("Allow image to be deleted (default)"), help=_("Allow image to be deleted (default)"),
) )
public_group = parser.add_mutually_exclusive_group() public_group = parser.add_mutually_exclusive_group()
public_group.add_argument( public_group.add_argument(
"--public", "--public",
action="store_true", action="store_const",
const="public",
dest="visibility",
help=_("Image is accessible to the public"), help=_("Image is accessible to the public"),
) )
public_group.add_argument( public_group.add_argument(
"--private", "--private",
action="store_true", action="store_const",
const="private",
dest="visibility",
help=_("Image is inaccessible to the public (default)"), help=_("Image is inaccessible to the public (default)"),
) )
public_group.add_argument( public_group.add_argument(
"--community", "--community",
action="store_true", action="store_const",
const="community",
dest="visibility",
help=_("Image is accessible to the community"), help=_("Image is accessible to the community"),
) )
public_group.add_argument( public_group.add_argument(
"--shared", "--shared",
action="store_true", action="store_const",
const="shared",
dest="visibility",
help=_("Image can be shared"), help=_("Image can be shared"),
) )
parser.add_argument( parser.add_argument(
@ -440,18 +452,12 @@ class CreateImage(command.ShowOne):
# a single value for the pair of options because the default must be # a single value for the pair of options because the default must be
# to do nothing when no options are present as opposed to always # to do nothing when no options are present as opposed to always
# setting a default. # setting a default.
if parsed_args.protected: if parsed_args.is_protected is not None:
kwargs['is_protected'] = True kwargs['is_protected'] = parsed_args.is_protected
if parsed_args.unprotected:
kwargs['is_protected'] = False if parsed_args.visibility is not None:
if parsed_args.public: kwargs['visibility'] = parsed_args.visibility
kwargs['visibility'] = 'public'
if parsed_args.private:
kwargs['visibility'] = 'private'
if parsed_args.community:
kwargs['visibility'] = 'community'
if parsed_args.shared:
kwargs['visibility'] = 'shared'
if parsed_args.project: if parsed_args.project:
kwargs['owner_id'] = common.find_project( kwargs['owner_id'] = common.find_project(
identity_client, identity_client,
@ -557,16 +563,20 @@ class CreateImage(command.ShowOne):
if volume_client.api_version >= api_versions.APIVersion('3.1'): if volume_client.api_version >= api_versions.APIVersion('3.1'):
mv_kwargs.update( mv_kwargs.update(
visibility=kwargs.get('visibility', 'private'), visibility=kwargs.get('visibility', 'private'),
protected=bool(parsed_args.protected), protected=bool(parsed_args.is_protected),
) )
else: else:
if kwargs.get('visibility') or parsed_args.protected: if (
parsed_args.visibility or
parsed_args.is_protected is not None
):
msg = _( msg = _(
'--os-volume-api-version 3.1 or greater is required ' '--os-volume-api-version 3.1 or greater is required '
'to support the --public, --private, --community, ' 'to support the --public, --private, --community, '
'--shared or --protected option.' '--shared or --protected option.'
) )
raise exceptions.CommandError(msg) raise exceptions.CommandError(msg)
response, body = volume_client.volumes.upload_to_image( response, body = volume_client.volumes.upload_to_image(
source_volume.id, source_volume.id,
parsed_args.force, parsed_args.force,
@ -637,37 +647,37 @@ class ListImage(command.Lister):
public_group = parser.add_mutually_exclusive_group() public_group = parser.add_mutually_exclusive_group()
public_group.add_argument( public_group.add_argument(
"--public", "--public",
dest="public", action="store_const",
action="store_true", const="public",
default=False, dest="visibility",
help=_("List only public images"), help=_("List only public images"),
) )
public_group.add_argument( public_group.add_argument(
"--private", "--private",
dest="private", action="store_const",
action="store_true", const="private",
default=False, dest="visibility",
help=_("List only private images"), help=_("List only private images"),
) )
public_group.add_argument( public_group.add_argument(
"--community", "--community",
dest="community", action="store_const",
action="store_true", const="community",
default=False, dest="visibility",
help=_("List only community images"), help=_("List only community images"),
) )
public_group.add_argument( public_group.add_argument(
"--shared", "--shared",
dest="shared", action="store_const",
action="store_true", const="shared",
default=False, dest="visibility",
help=_("List only shared images"), help=_("List only shared images"),
) )
public_group.add_argument( public_group.add_argument(
"--all", "--all",
dest="all", action="store_const",
action="store_true", const="all",
default=False, dest="visibility",
help=_("List all images"), help=_("List all images"),
) )
parser.add_argument( parser.add_argument(
@ -724,6 +734,7 @@ class ListImage(command.Lister):
parser.add_argument( parser.add_argument(
'--hidden', '--hidden',
action='store_true', action='store_true',
dest='is_hidden',
default=False, default=False,
help=_('List hidden images'), help=_('List hidden images'),
) )
@ -774,16 +785,8 @@ class ListImage(command.Lister):
image_client = self.app.client_manager.image image_client = self.app.client_manager.image
kwargs = {} kwargs = {}
if parsed_args.public: if parsed_args.visibility is not None:
kwargs['visibility'] = 'public' kwargs['visibility'] = parsed_args.visibility
if parsed_args.private:
kwargs['visibility'] = 'private'
if parsed_args.community:
kwargs['visibility'] = 'community'
if parsed_args.shared:
kwargs['visibility'] = 'shared'
if parsed_args.all:
kwargs['visibility'] = 'all'
if parsed_args.limit: if parsed_args.limit:
kwargs['limit'] = parsed_args.limit kwargs['limit'] = parsed_args.limit
if parsed_args.marker: if parsed_args.marker:
@ -804,8 +807,8 @@ class ListImage(command.Lister):
parsed_args.project_domain, parsed_args.project_domain,
).id ).id
kwargs['owner'] = project_id kwargs['owner'] = project_id
if parsed_args.hidden: if parsed_args.is_hidden:
kwargs['is_hidden'] = True kwargs['is_hidden'] = parsed_args.is_hidden
if parsed_args.long: if parsed_args.long:
columns = ( columns = (
'ID', 'ID',
@ -1014,32 +1017,44 @@ class SetImage(command.Command):
protected_group.add_argument( protected_group.add_argument(
"--protected", "--protected",
action="store_true", action="store_true",
dest="is_protected",
default=None,
help=_("Prevent image from being deleted"), help=_("Prevent image from being deleted"),
) )
protected_group.add_argument( protected_group.add_argument(
"--unprotected", "--unprotected",
action="store_true", action="store_false",
dest="is_protected",
default=None,
help=_("Allow image to be deleted (default)"), help=_("Allow image to be deleted (default)"),
) )
public_group = parser.add_mutually_exclusive_group() public_group = parser.add_mutually_exclusive_group()
public_group.add_argument( public_group.add_argument(
"--public", "--public",
action="store_true", action="store_const",
const="public",
dest="visibility",
help=_("Image is accessible to the public"), help=_("Image is accessible to the public"),
) )
public_group.add_argument( public_group.add_argument(
"--private", "--private",
action="store_true", action="store_const",
const="private",
dest="visibility",
help=_("Image is inaccessible to the public (default)"), help=_("Image is inaccessible to the public (default)"),
) )
public_group.add_argument( public_group.add_argument(
"--community", "--community",
action="store_true", action="store_const",
const="community",
dest="visibility",
help=_("Image is accessible to the community"), help=_("Image is accessible to the community"),
) )
public_group.add_argument( public_group.add_argument(
"--shared", "--shared",
action="store_true", action="store_const",
const="shared",
dest="visibility",
help=_("Image can be shared"), help=_("Image can be shared"),
) )
parser.add_argument( parser.add_argument(
@ -1120,7 +1135,7 @@ class SetImage(command.Command):
parser.add_argument( parser.add_argument(
"--%s" % deadopt, "--%s" % deadopt,
metavar="<%s>" % deadopt, metavar="<%s>" % deadopt,
dest=deadopt.replace('-', '_'), dest=f"dead_{deadopt.replace('-', '_')}",
help=argparse.SUPPRESS, help=argparse.SUPPRESS,
) )
@ -1153,14 +1168,14 @@ class SetImage(command.Command):
hidden_group = parser.add_mutually_exclusive_group() hidden_group = parser.add_mutually_exclusive_group()
hidden_group.add_argument( hidden_group.add_argument(
"--hidden", "--hidden",
dest='hidden', dest="is_hidden",
default=None, default=None,
action="store_true", action="store_true",
help=_("Hide the image"), help=_("Hide the image"),
) )
hidden_group.add_argument( hidden_group.add_argument(
"--unhidden", "--unhidden",
dest='hidden', dest="is_hidden",
default=None, default=None,
action="store_false", action="store_false",
help=_("Unhide the image"), help=_("Unhide the image"),
@ -1172,7 +1187,7 @@ class SetImage(command.Command):
image_client = self.app.client_manager.image image_client = self.app.client_manager.image
for deadopt in self.deadopts: for deadopt in self.deadopts:
if getattr(parsed_args, deadopt.replace('-', '_'), None): if getattr(parsed_args, f"dead_{deadopt.replace('-', '_')}", None):
raise exceptions.CommandError( raise exceptions.CommandError(
_( _(
"ERROR: --%s was given, which is an Image v1 option" "ERROR: --%s was given, which is an Image v1 option"
@ -1258,26 +1273,22 @@ class SetImage(command.Command):
# a single value for the pair of options because the default must be # a single value for the pair of options because the default must be
# to do nothing when no options are present as opposed to always # to do nothing when no options are present as opposed to always
# setting a default. # setting a default.
if parsed_args.protected: if parsed_args.is_protected is not None:
kwargs['is_protected'] = True kwargs['is_protected'] = parsed_args.is_protected
if parsed_args.unprotected:
kwargs['is_protected'] = False if parsed_args.visibility is not None:
if parsed_args.public: kwargs['visibility'] = parsed_args.visibility
kwargs['visibility'] = 'public'
if parsed_args.private:
kwargs['visibility'] = 'private'
if parsed_args.community:
kwargs['visibility'] = 'community'
if parsed_args.shared:
kwargs['visibility'] = 'shared'
if parsed_args.project: if parsed_args.project:
# We already did the project lookup above # We already did the project lookup above
kwargs['owner_id'] = project_id kwargs['owner_id'] = project_id
if parsed_args.tags: if parsed_args.tags:
# Tags should be extended, but duplicates removed # Tags should be extended, but duplicates removed
kwargs['tags'] = list(set(image.tags).union(set(parsed_args.tags))) kwargs['tags'] = list(set(image.tags).union(set(parsed_args.tags)))
if parsed_args.hidden is not None:
kwargs['is_hidden'] = parsed_args.hidden if parsed_args.is_hidden is not None:
kwargs['is_hidden'] = parsed_args.is_hidden
try: try:
image = image_client.update_image(image.id, **kwargs) image = image_client.update_image(image.id, **kwargs)

116
openstackclient/tests/unit/image/v2/test_image.py
View File

@ -32,7 +32,7 @@ from openstackclient.tests.unit.volume.v3 import fakes as volume_fakes
class TestImage(image_fakes.TestImagev2, volume_fakes.TestVolume): class TestImage(image_fakes.TestImagev2, volume_fakes.TestVolume):
def setUp(self): def setUp(self):
super(TestImage, self).setUp() super().setUp()
# Get shortcuts to mocked image client # Get shortcuts to mocked image client
self.client = self.app.client_manager.image self.client = self.app.client_manager.image
@ -62,7 +62,7 @@ class TestImageCreate(TestImage):
domain = identity_fakes.FakeDomain.create_one_domain() domain = identity_fakes.FakeDomain.create_one_domain()
def setUp(self): def setUp(self):
super(TestImageCreate, self).setUp() super().setUp()
self.new_image = image_fakes.create_one_image() self.new_image = image_fakes.create_one_image()
self.client.create_image.return_value = self.new_image self.client.create_image.return_value = self.new_image
@ -134,10 +134,8 @@ class TestImageCreate(TestImage):
('disk_format', 'ami'), ('disk_format', 'ami'),
('min_disk', 10), ('min_disk', 10),
('min_ram', 4), ('min_ram', 4),
('protected', self.new_image.is_protected), ('is_protected', self.new_image.is_protected),
('unprotected', not self.new_image.is_protected), ('visibility', self.new_image.visibility),
('public', self.new_image.visibility == 'public'),
('private', self.new_image.visibility == 'private'),
('project', self.new_image.owner_id), ('project', self.new_image.owner_id),
('project_domain', self.domain.id), ('project_domain', self.domain.id),
('name', self.new_image.name), ('name', self.new_image.name),
@ -188,10 +186,8 @@ class TestImageCreate(TestImage):
('disk_format', 'ami'), ('disk_format', 'ami'),
('min_disk', 10), ('min_disk', 10),
('min_ram', 4), ('min_ram', 4),
('protected', True), ('is_protected', True),
('unprotected', False), ('visibility', 'private'),
('public', False),
('private', True),
('project', 'unexist_owner'), ('project', 'unexist_owner'),
('name', 'graven'), ('name', 'graven'),
] ]
@ -222,10 +218,8 @@ class TestImageCreate(TestImage):
] ]
verifylist = [ verifylist = [
('file', imagefile.name), ('file', imagefile.name),
('protected', self.new_image.is_protected), ('is_protected', self.new_image.is_protected),
('unprotected', not self.new_image.is_protected), ('visibility', self.new_image.visibility),
('public', self.new_image.visibility == 'public'),
('private', self.new_image.visibility == 'private'),
('properties', {'Alpha': '1', 'Beta': '2'}), ('properties', {'Alpha': '1', 'Beta': '2'}),
('tags', self.new_image.tags), ('tags', self.new_image.tags),
('name', self.new_image.name), ('name', self.new_image.name),
@ -421,7 +415,7 @@ class TestAddProjectToImage(TestImage):
) )
def setUp(self): def setUp(self):
super(TestAddProjectToImage, self).setUp() super().setUp()
# This is the return value for utils.find_resource() # This is the return value for utils.find_resource()
self.client.find_image.return_value = self._image self.client.find_image.return_value = self._image
@ -485,7 +479,7 @@ class TestAddProjectToImage(TestImage):
class TestImageDelete(TestImage): class TestImageDelete(TestImage):
def setUp(self): def setUp(self):
super(TestImageDelete, self).setUp() super().setUp()
self.client.delete_image.return_value = None self.client.delete_image.return_value = None
@ -576,7 +570,7 @@ class TestImageList(TestImage):
), ),
def setUp(self): def setUp(self):
super(TestImageList, self).setUp() super().setUp()
self.client.images.side_effect = [[self._image], []] self.client.images.side_effect = [[self._image], []]
@ -586,11 +580,7 @@ class TestImageList(TestImage):
def test_image_list_no_options(self): def test_image_list_no_options(self):
arglist = [] arglist = []
verifylist = [ verifylist = [
('public', False), ('visibility', None),
('private', False),
('community', False),
('shared', False),
('all', False),
('long', False), ('long', False),
] ]
parsed_args = self.check_parser(self.cmd, arglist, verifylist) parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -611,11 +601,7 @@ class TestImageList(TestImage):
'--public', '--public',
] ]
verifylist = [ verifylist = [
('public', True), ('visibility', 'public'),
('private', False),
('community', False),
('shared', False),
('all', False),
('long', False), ('long', False),
] ]
parsed_args = self.check_parser(self.cmd, arglist, verifylist) parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -636,11 +622,7 @@ class TestImageList(TestImage):
'--private', '--private',
] ]
verifylist = [ verifylist = [
('public', False), ('visibility', 'private'),
('private', True),
('community', False),
('shared', False),
('all', False),
('long', False), ('long', False),
] ]
parsed_args = self.check_parser(self.cmd, arglist, verifylist) parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -661,11 +643,7 @@ class TestImageList(TestImage):
'--community', '--community',
] ]
verifylist = [ verifylist = [
('public', False), ('visibility', 'community'),
('private', False),
('community', True),
('shared', False),
('all', False),
('long', False), ('long', False),
] ]
parsed_args = self.check_parser(self.cmd, arglist, verifylist) parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -686,11 +664,7 @@ class TestImageList(TestImage):
'--shared', '--shared',
] ]
verifylist = [ verifylist = [
('public', False), ('visibility', 'shared'),
('private', False),
('community', False),
('shared', True),
('all', False),
('long', False), ('long', False),
] ]
parsed_args = self.check_parser(self.cmd, arglist, verifylist) parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -711,11 +685,7 @@ class TestImageList(TestImage):
'--all', '--all',
] ]
verifylist = [ verifylist = [
('public', False), ('visibility', 'all'),
('private', False),
('community', False),
('shared', False),
('all', True),
('long', False), ('long', False),
] ]
parsed_args = self.check_parser(self.cmd, arglist, verifylist) parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -737,11 +707,7 @@ class TestImageList(TestImage):
'--member-status', 'all' '--member-status', 'all'
] ]
verifylist = [ verifylist = [
('public', False), ('visibility', 'shared'),
('private', False),
('community', False),
('shared', True),
('all', False),
('long', False), ('long', False),
('member_status', 'all') ('member_status', 'all')
] ]
@ -765,11 +731,7 @@ class TestImageList(TestImage):
'--member-status', 'ALl' '--member-status', 'ALl'
] ]
verifylist = [ verifylist = [
('public', False), ('visibility', 'shared'),
('private', False),
('community', False),
('shared', True),
('all', False),
('long', False), ('long', False),
('member_status', 'all') ('member_status', 'all')
] ]
@ -959,7 +921,7 @@ class TestImageList(TestImage):
'--hidden', '--hidden',
] ]
verifylist = [ verifylist = [
('hidden', True), ('is_hidden', True),
] ]
parsed_args = self.check_parser(self.cmd, arglist, verifylist) parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -1006,7 +968,7 @@ class TestListImageProjects(TestImage):
)] )]
def setUp(self): def setUp(self):
super(TestListImageProjects, self).setUp() super().setUp()
self.client.find_image.return_value = self._image self.client.find_image.return_value = self._image
self.client.members.return_value = [self.member] self.client.members.return_value = [self.member]
@ -1036,7 +998,7 @@ class TestRemoveProjectImage(TestImage):
domain = identity_fakes.FakeDomain.create_one_domain() domain = identity_fakes.FakeDomain.create_one_domain()
def setUp(self): def setUp(self):
super(TestRemoveProjectImage, self).setUp() super().setUp()
self._image = image_fakes.create_one_image() self._image = image_fakes.create_one_image()
# This is the return value for utils.find_resource() # This is the return value for utils.find_resource()
@ -1100,7 +1062,7 @@ class TestImageSet(TestImage):
_image = image_fakes.create_one_image({'tags': []}) _image = image_fakes.create_one_image({'tags': []})
def setUp(self): def setUp(self):
super(TestImageSet, self).setUp() super().setUp()
self.project_mock.get.return_value = self.project self.project_mock.get.return_value = self.project
@ -1282,10 +1244,8 @@ class TestImageSet(TestImage):
'graven', 'graven',
] ]
verifylist = [ verifylist = [
('protected', True), ('is_protected', True),
('unprotected', False), ('visibility', 'private'),
('public', False),
('private', True),
('image', 'graven'), ('image', 'graven'),
] ]
parsed_args = self.check_parser(self.cmd, arglist, verifylist) parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -1310,10 +1270,8 @@ class TestImageSet(TestImage):
'graven', 'graven',
] ]
verifylist = [ verifylist = [
('protected', False), ('is_protected', False),
('unprotected', True), ('visibility', 'public'),
('public', True),
('private', False),
('image', 'graven'), ('image', 'graven'),
] ]
parsed_args = self.check_parser(self.cmd, arglist, verifylist) parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -1532,7 +1490,7 @@ class TestImageSet(TestImage):
'graven', 'graven',
] ]
verifylist = [ verifylist = [
('visibility', '1-mile'), ('dead_visibility', '1-mile'),
('image', 'graven'), ('image', 'graven'),
] ]
parsed_args = self.check_parser(self.cmd, arglist, verifylist) parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -1574,9 +1532,8 @@ class TestImageSet(TestImage):
'graven', 'graven',
] ]
verifylist = [ verifylist = [
('hidden', True), ('is_hidden', True),
('public', True), ('visibility', 'public'),
('private', False),
('image', 'graven'), ('image', 'graven'),
] ]
parsed_args = self.check_parser(self.cmd, arglist, verifylist) parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -1601,9 +1558,8 @@ class TestImageSet(TestImage):
'graven', 'graven',
] ]
verifylist = [ verifylist = [
('hidden', False), ('is_hidden', False),
('public', True), ('visibility', 'public'),
('private', False),
('image', 'graven'), ('image', 'graven'),
] ]
parsed_args = self.check_parser(self.cmd, arglist, verifylist) parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@ -1643,7 +1599,7 @@ class TestImageShow(TestImage):
) )
def setUp(self): def setUp(self):
super(TestImageShow, self).setUp() super().setUp()
self.client.find_image = mock.Mock(return_value=self._data) self.client.find_image = mock.Mock(return_value=self._data)
@ -1699,7 +1655,7 @@ class TestImageShow(TestImage):
class TestImageUnset(TestImage): class TestImageUnset(TestImage):
def setUp(self): def setUp(self):
super(TestImageUnset, self).setUp() super().setUp()
attrs = {} attrs = {}
attrs['tags'] = ['test'] attrs['tags'] = ['test']
@ -1798,7 +1754,7 @@ class TestImageSave(TestImage):
image = image_fakes.create_one_image({}) image = image_fakes.create_one_image({})
def setUp(self): def setUp(self):
super(TestImageSave, self).setUp() super().setUp()
self.client.find_image.return_value = self.image self.client.find_image.return_value = self.image
self.client.download_image.return_value = self.image self.client.download_image.return_value = self.image
@ -1827,7 +1783,7 @@ class TestImageSave(TestImage):
class TestImageGetData(TestImage): class TestImageGetData(TestImage):
def setUp(self): def setUp(self):
super(TestImageGetData, self).setUp() super().setUp()
self.args = mock.Mock() self.args = mock.Mock()
def test_get_data_file_file(self): def test_get_data_file_file(self):