From 4776e0a5ae17c728f9344041f9bf634b3263fcd5 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 30 Jun 2022 11:10:18 +0100 Subject: [PATCH] image: Make better use of argparse Simplify some logic by using a common 'dest' for mutually exclusive options. Change-Id: Ie5f3600672953f40be52de51e84717c8912ddaf8 Signed-off-by: Stephen Finucane --- openstackclient/image/v2/image.py | 149 ++++++++++-------- .../tests/unit/image/v2/test_image.py | 116 +++++--------- 2 files changed, 116 insertions(+), 149 deletions(-) diff --git a/openstackclient/image/v2/image.py b/openstackclient/image/v2/image.py index 80eb8d22a3..17f0fb2a55 100644 --- a/openstackclient/image/v2/image.py +++ b/openstackclient/image/v2/image.py @@ -325,32 +325,44 @@ class CreateImage(command.ShowOne): protected_group.add_argument( "--protected", action="store_true", + dest="is_protected", + default=None, help=_("Prevent image from being deleted"), ) protected_group.add_argument( "--unprotected", - action="store_true", + action="store_false", + dest="is_protected", + default=None, help=_("Allow image to be deleted (default)"), ) public_group = parser.add_mutually_exclusive_group() public_group.add_argument( "--public", - action="store_true", + action="store_const", + const="public", + dest="visibility", help=_("Image is accessible to the public"), ) public_group.add_argument( "--private", - action="store_true", + action="store_const", + const="private", + dest="visibility", help=_("Image is inaccessible to the public (default)"), ) public_group.add_argument( "--community", - action="store_true", + action="store_const", + const="community", + dest="visibility", help=_("Image is accessible to the community"), ) public_group.add_argument( "--shared", - action="store_true", + action="store_const", + const="shared", + dest="visibility", help=_("Image can be shared"), ) parser.add_argument( @@ -440,18 +452,12 @@ class CreateImage(command.ShowOne): # 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 # setting a default. - if parsed_args.protected: - kwargs['is_protected'] = True - if parsed_args.unprotected: - kwargs['is_protected'] = False - if parsed_args.public: - 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.is_protected is not None: + kwargs['is_protected'] = parsed_args.is_protected + + if parsed_args.visibility is not None: + kwargs['visibility'] = parsed_args.visibility + if parsed_args.project: kwargs['owner_id'] = common.find_project( identity_client, @@ -557,16 +563,20 @@ class CreateImage(command.ShowOne): if volume_client.api_version >= api_versions.APIVersion('3.1'): mv_kwargs.update( visibility=kwargs.get('visibility', 'private'), - protected=bool(parsed_args.protected), + protected=bool(parsed_args.is_protected), ) else: - if kwargs.get('visibility') or parsed_args.protected: + if ( + parsed_args.visibility or + parsed_args.is_protected is not None + ): msg = _( '--os-volume-api-version 3.1 or greater is required ' 'to support the --public, --private, --community, ' '--shared or --protected option.' ) raise exceptions.CommandError(msg) + response, body = volume_client.volumes.upload_to_image( source_volume.id, parsed_args.force, @@ -637,37 +647,37 @@ class ListImage(command.Lister): public_group = parser.add_mutually_exclusive_group() public_group.add_argument( "--public", - dest="public", - action="store_true", - default=False, + action="store_const", + const="public", + dest="visibility", help=_("List only public images"), ) public_group.add_argument( "--private", - dest="private", - action="store_true", - default=False, + action="store_const", + const="private", + dest="visibility", help=_("List only private images"), ) public_group.add_argument( "--community", - dest="community", - action="store_true", - default=False, + action="store_const", + const="community", + dest="visibility", help=_("List only community images"), ) public_group.add_argument( "--shared", - dest="shared", - action="store_true", - default=False, + action="store_const", + const="shared", + dest="visibility", help=_("List only shared images"), ) public_group.add_argument( "--all", - dest="all", - action="store_true", - default=False, + action="store_const", + const="all", + dest="visibility", help=_("List all images"), ) parser.add_argument( @@ -724,6 +734,7 @@ class ListImage(command.Lister): parser.add_argument( '--hidden', action='store_true', + dest='is_hidden', default=False, help=_('List hidden images'), ) @@ -774,16 +785,8 @@ class ListImage(command.Lister): image_client = self.app.client_manager.image kwargs = {} - if parsed_args.public: - 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.all: - kwargs['visibility'] = 'all' + if parsed_args.visibility is not None: + kwargs['visibility'] = parsed_args.visibility if parsed_args.limit: kwargs['limit'] = parsed_args.limit if parsed_args.marker: @@ -804,8 +807,8 @@ class ListImage(command.Lister): parsed_args.project_domain, ).id kwargs['owner'] = project_id - if parsed_args.hidden: - kwargs['is_hidden'] = True + if parsed_args.is_hidden: + kwargs['is_hidden'] = parsed_args.is_hidden if parsed_args.long: columns = ( 'ID', @@ -1014,32 +1017,44 @@ class SetImage(command.Command): protected_group.add_argument( "--protected", action="store_true", + dest="is_protected", + default=None, help=_("Prevent image from being deleted"), ) protected_group.add_argument( "--unprotected", - action="store_true", + action="store_false", + dest="is_protected", + default=None, help=_("Allow image to be deleted (default)"), ) public_group = parser.add_mutually_exclusive_group() public_group.add_argument( "--public", - action="store_true", + action="store_const", + const="public", + dest="visibility", help=_("Image is accessible to the public"), ) public_group.add_argument( "--private", - action="store_true", + action="store_const", + const="private", + dest="visibility", help=_("Image is inaccessible to the public (default)"), ) public_group.add_argument( "--community", - action="store_true", + action="store_const", + const="community", + dest="visibility", help=_("Image is accessible to the community"), ) public_group.add_argument( "--shared", - action="store_true", + action="store_const", + const="shared", + dest="visibility", help=_("Image can be shared"), ) parser.add_argument( @@ -1120,7 +1135,7 @@ class SetImage(command.Command): parser.add_argument( "--%s" % deadopt, metavar="<%s>" % deadopt, - dest=deadopt.replace('-', '_'), + dest=f"dead_{deadopt.replace('-', '_')}", help=argparse.SUPPRESS, ) @@ -1153,14 +1168,14 @@ class SetImage(command.Command): hidden_group = parser.add_mutually_exclusive_group() hidden_group.add_argument( "--hidden", - dest='hidden', + dest="is_hidden", default=None, action="store_true", help=_("Hide the image"), ) hidden_group.add_argument( "--unhidden", - dest='hidden', + dest="is_hidden", default=None, action="store_false", help=_("Unhide the image"), @@ -1172,7 +1187,7 @@ class SetImage(command.Command): image_client = self.app.client_manager.image for deadopt in self.deadopts: - if getattr(parsed_args, deadopt.replace('-', '_'), None): + if getattr(parsed_args, f"dead_{deadopt.replace('-', '_')}", None): raise exceptions.CommandError( _( "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 # to do nothing when no options are present as opposed to always # setting a default. - if parsed_args.protected: - kwargs['is_protected'] = True - if parsed_args.unprotected: - kwargs['is_protected'] = False - if parsed_args.public: - 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.is_protected is not None: + kwargs['is_protected'] = parsed_args.is_protected + + if parsed_args.visibility is not None: + kwargs['visibility'] = parsed_args.visibility + if parsed_args.project: # We already did the project lookup above kwargs['owner_id'] = project_id + if parsed_args.tags: # Tags should be extended, but duplicates removed 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: image = image_client.update_image(image.id, **kwargs) diff --git a/openstackclient/tests/unit/image/v2/test_image.py b/openstackclient/tests/unit/image/v2/test_image.py index 9241b0a46a..02f7b4118f 100644 --- a/openstackclient/tests/unit/image/v2/test_image.py +++ b/openstackclient/tests/unit/image/v2/test_image.py @@ -32,7 +32,7 @@ from openstackclient.tests.unit.volume.v3 import fakes as volume_fakes class TestImage(image_fakes.TestImagev2, volume_fakes.TestVolume): def setUp(self): - super(TestImage, self).setUp() + super().setUp() # Get shortcuts to mocked image client self.client = self.app.client_manager.image @@ -62,7 +62,7 @@ class TestImageCreate(TestImage): domain = identity_fakes.FakeDomain.create_one_domain() def setUp(self): - super(TestImageCreate, self).setUp() + super().setUp() self.new_image = image_fakes.create_one_image() self.client.create_image.return_value = self.new_image @@ -134,10 +134,8 @@ class TestImageCreate(TestImage): ('disk_format', 'ami'), ('min_disk', 10), ('min_ram', 4), - ('protected', self.new_image.is_protected), - ('unprotected', not self.new_image.is_protected), - ('public', self.new_image.visibility == 'public'), - ('private', self.new_image.visibility == 'private'), + ('is_protected', self.new_image.is_protected), + ('visibility', self.new_image.visibility), ('project', self.new_image.owner_id), ('project_domain', self.domain.id), ('name', self.new_image.name), @@ -188,10 +186,8 @@ class TestImageCreate(TestImage): ('disk_format', 'ami'), ('min_disk', 10), ('min_ram', 4), - ('protected', True), - ('unprotected', False), - ('public', False), - ('private', True), + ('is_protected', True), + ('visibility', 'private'), ('project', 'unexist_owner'), ('name', 'graven'), ] @@ -222,10 +218,8 @@ class TestImageCreate(TestImage): ] verifylist = [ ('file', imagefile.name), - ('protected', self.new_image.is_protected), - ('unprotected', not self.new_image.is_protected), - ('public', self.new_image.visibility == 'public'), - ('private', self.new_image.visibility == 'private'), + ('is_protected', self.new_image.is_protected), + ('visibility', self.new_image.visibility), ('properties', {'Alpha': '1', 'Beta': '2'}), ('tags', self.new_image.tags), ('name', self.new_image.name), @@ -421,7 +415,7 @@ class TestAddProjectToImage(TestImage): ) def setUp(self): - super(TestAddProjectToImage, self).setUp() + super().setUp() # This is the return value for utils.find_resource() self.client.find_image.return_value = self._image @@ -485,7 +479,7 @@ class TestAddProjectToImage(TestImage): class TestImageDelete(TestImage): def setUp(self): - super(TestImageDelete, self).setUp() + super().setUp() self.client.delete_image.return_value = None @@ -576,7 +570,7 @@ class TestImageList(TestImage): ), def setUp(self): - super(TestImageList, self).setUp() + super().setUp() self.client.images.side_effect = [[self._image], []] @@ -586,11 +580,7 @@ class TestImageList(TestImage): def test_image_list_no_options(self): arglist = [] verifylist = [ - ('public', False), - ('private', False), - ('community', False), - ('shared', False), - ('all', False), + ('visibility', None), ('long', False), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -611,11 +601,7 @@ class TestImageList(TestImage): '--public', ] verifylist = [ - ('public', True), - ('private', False), - ('community', False), - ('shared', False), - ('all', False), + ('visibility', 'public'), ('long', False), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -636,11 +622,7 @@ class TestImageList(TestImage): '--private', ] verifylist = [ - ('public', False), - ('private', True), - ('community', False), - ('shared', False), - ('all', False), + ('visibility', 'private'), ('long', False), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -661,11 +643,7 @@ class TestImageList(TestImage): '--community', ] verifylist = [ - ('public', False), - ('private', False), - ('community', True), - ('shared', False), - ('all', False), + ('visibility', 'community'), ('long', False), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -686,11 +664,7 @@ class TestImageList(TestImage): '--shared', ] verifylist = [ - ('public', False), - ('private', False), - ('community', False), - ('shared', True), - ('all', False), + ('visibility', 'shared'), ('long', False), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -711,11 +685,7 @@ class TestImageList(TestImage): '--all', ] verifylist = [ - ('public', False), - ('private', False), - ('community', False), - ('shared', False), - ('all', True), + ('visibility', 'all'), ('long', False), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -737,11 +707,7 @@ class TestImageList(TestImage): '--member-status', 'all' ] verifylist = [ - ('public', False), - ('private', False), - ('community', False), - ('shared', True), - ('all', False), + ('visibility', 'shared'), ('long', False), ('member_status', 'all') ] @@ -765,11 +731,7 @@ class TestImageList(TestImage): '--member-status', 'ALl' ] verifylist = [ - ('public', False), - ('private', False), - ('community', False), - ('shared', True), - ('all', False), + ('visibility', 'shared'), ('long', False), ('member_status', 'all') ] @@ -959,7 +921,7 @@ class TestImageList(TestImage): '--hidden', ] verifylist = [ - ('hidden', True), + ('is_hidden', True), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -1006,7 +968,7 @@ class TestListImageProjects(TestImage): )] def setUp(self): - super(TestListImageProjects, self).setUp() + super().setUp() self.client.find_image.return_value = self._image self.client.members.return_value = [self.member] @@ -1036,7 +998,7 @@ class TestRemoveProjectImage(TestImage): domain = identity_fakes.FakeDomain.create_one_domain() def setUp(self): - super(TestRemoveProjectImage, self).setUp() + super().setUp() self._image = image_fakes.create_one_image() # This is the return value for utils.find_resource() @@ -1100,7 +1062,7 @@ class TestImageSet(TestImage): _image = image_fakes.create_one_image({'tags': []}) def setUp(self): - super(TestImageSet, self).setUp() + super().setUp() self.project_mock.get.return_value = self.project @@ -1282,10 +1244,8 @@ class TestImageSet(TestImage): 'graven', ] verifylist = [ - ('protected', True), - ('unprotected', False), - ('public', False), - ('private', True), + ('is_protected', True), + ('visibility', 'private'), ('image', 'graven'), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -1310,10 +1270,8 @@ class TestImageSet(TestImage): 'graven', ] verifylist = [ - ('protected', False), - ('unprotected', True), - ('public', True), - ('private', False), + ('is_protected', False), + ('visibility', 'public'), ('image', 'graven'), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -1532,7 +1490,7 @@ class TestImageSet(TestImage): 'graven', ] verifylist = [ - ('visibility', '1-mile'), + ('dead_visibility', '1-mile'), ('image', 'graven'), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -1574,9 +1532,8 @@ class TestImageSet(TestImage): 'graven', ] verifylist = [ - ('hidden', True), - ('public', True), - ('private', False), + ('is_hidden', True), + ('visibility', 'public'), ('image', 'graven'), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -1601,9 +1558,8 @@ class TestImageSet(TestImage): 'graven', ] verifylist = [ - ('hidden', False), - ('public', True), - ('private', False), + ('is_hidden', False), + ('visibility', 'public'), ('image', 'graven'), ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -1643,7 +1599,7 @@ class TestImageShow(TestImage): ) def setUp(self): - super(TestImageShow, self).setUp() + super().setUp() self.client.find_image = mock.Mock(return_value=self._data) @@ -1699,7 +1655,7 @@ class TestImageShow(TestImage): class TestImageUnset(TestImage): def setUp(self): - super(TestImageUnset, self).setUp() + super().setUp() attrs = {} attrs['tags'] = ['test'] @@ -1798,7 +1754,7 @@ class TestImageSave(TestImage): image = image_fakes.create_one_image({}) def setUp(self): - super(TestImageSave, self).setUp() + super().setUp() self.client.find_image.return_value = self.image self.client.download_image.return_value = self.image @@ -1827,7 +1783,7 @@ class TestImageSave(TestImage): class TestImageGetData(TestImage): def setUp(self): - super(TestImageGetData, self).setUp() + super().setUp() self.args = mock.Mock() def test_get_data_file_file(self):