From 201b1cee86a4df8ede6c97d962ac331ad0378140 Mon Sep 17 00:00:00 2001 From: Dean Troyer Date: Tue, 22 Sep 2015 17:17:37 -0500 Subject: [PATCH] Clean up Image v2 image set command Make the Image v2 image set command meet at the intersection of the v1 image set command and the v2 image create command: * Add visibility to the deadopts list and remove the option * Put the options in the same order as v1 image set * Make the help text match * Add --properties * Move the additional options that do not appear in either v1 image set or v2 image create after --property as they are really pre-defined properties * Add tests for v2 image set to match v1 and then some * Put the SetImage class in v2/image.py in alphabetical order Change-Id: I102b914e8ad09a014f6fdd846c5766b6c2eaadb8 --- doc/source/command-objects/image.rst | 55 +++-- openstackclient/image/v2/image.py | 182 +++++++------- openstackclient/tests/image/v2/test_image.py | 241 +++++++++++++++---- 3 files changed, 309 insertions(+), 169 deletions(-) diff --git a/doc/source/command-objects/image.rst b/doc/source/command-objects/image.rst index d2698b0b97..8dce3662f3 100644 --- a/doc/source/command-objects/image.rst +++ b/doc/source/command-objects/image.rst @@ -238,6 +238,12 @@ Set image properties [--checksum ] [--stdin] [--property [...] ] + [--architecture ] + [--instance-id ] + [--kernel-id ] + [--os-distro ] + [--os-version ] + [--ramdisk-id ] .. option:: --name @@ -258,14 +264,11 @@ Set image properties .. option:: --container-format - Container format of image. - Acceptable formats: ['ami', 'ari', 'aki', 'bare', 'ovf'] + Image container format (default: bare) .. option:: --disk-format - Disk format of image. - Acceptable formats: ['ami', 'ari', 'aki', 'vhd', 'vmdk', 'raw', 'qcow2', - 'vdi', 'iso'] + Image disk format (default: raw) .. option:: --size @@ -339,45 +342,41 @@ Set image properties .. option:: --property - Set a property on this image (repeat for multiple values) - - *Image version 1 only.* + Set a property on this image (repeat option to set multiple properties) .. option:: --architecture - Operating system Architecture + Operating system architecture .. versionadded:: 2 -.. option:: --ramdisk-id +.. option:: --instance-id - ID of image stored in Glance that should be used as - the ramdisk when booting an AMI-style image - - .. versionadded:: 2 - -.. option:: --os-distro - - Common name of operating system distribution - - .. versionadded:: 2 - -.. option:: --os-version - - Operating system version as specified by the distributor + ID of server instance used to create this image .. versionadded:: 2 .. option:: --kernel-id - ID of image in Glance that should be used as the - kernel when booting an AMI-style image + ID of kernel image used to boot this disk image .. versionadded:: 2 -.. option:: --instance-uuid +.. option:: --os-distro - ID of instance used to create this image + Operating system distribution name + + .. versionadded:: 2 + +.. option:: --os-version + + Operating system distribution version + + .. versionadded:: 2 + +.. option:: --ramdisk-id + + ID of ramdisk image used to boot this disk image .. versionadded:: 2 diff --git a/openstackclient/image/v2/image.py b/openstackclient/image/v2/image.py index fff26c0249..11c7483bdb 100644 --- a/openstackclient/image/v2/image.py +++ b/openstackclient/image/v2/image.py @@ -210,7 +210,7 @@ class CreateImage(show.ShowOne): "--%s" % deadopt, metavar="<%s>" % deadopt, dest=deadopt.replace('-', '_'), - help=argparse.SUPPRESS + help=argparse.SUPPRESS, ) return parser @@ -522,38 +522,11 @@ class SaveImage(command.Command): gc_utils.save_image(data, parsed_args.file) -class ShowImage(show.ShowOne): - """Display image details""" - - log = logging.getLogger(__name__ + ".ShowImage") - - def get_parser(self, prog_name): - parser = super(ShowImage, self).get_parser(prog_name) - parser.add_argument( - "image", - metavar="", - help="Image to display (name or ID)", - ) - return parser - - def take_action(self, parsed_args): - self.log.debug("take_action(%s)", parsed_args) - - image_client = self.app.client_manager.image - image = utils.find_resource( - image_client.images, - parsed_args.image, - ) - - info = _format_image(image) - return zip(*sorted(six.iteritems(info))) - - class SetImage(show.ShowOne): """Set image properties""" log = logging.getLogger(__name__ + ".SetImage") - deadopts = ('size', 'store', 'location', 'copy-from', 'checksum') + deadopts = ('visibility',) def get_parser(self, prog_name): parser = super(SetImage, self).get_parser(prog_name) @@ -567,7 +540,6 @@ class SetImage(show.ShowOne): # --force - needs adding # --checksum - maybe could be done client side # --stdin - could be implemented - # --property - needs adding # --tags - needs adding parser.add_argument( "image", @@ -580,20 +552,44 @@ class SetImage(show.ShowOne): help="New image name" ) parser.add_argument( - "--architecture", - metavar="", - help="Operating system Architecture" + "--owner", + metavar="", + help="New image owner project (name or ID)", + ) + parser.add_argument( + "--min-disk", + type=int, + metavar="", + help="Minimum disk size needed to boot image, in gigabytes" + ) + parser.add_argument( + "--min-ram", + type=int, + metavar="", + help="Minimum RAM size needed to boot image, in megabytes", + ) + parser.add_argument( + "--container-format", + metavar="", + help="Image container format " + "(default: %s)" % DEFAULT_CONTAINER_FORMAT, + ) + parser.add_argument( + "--disk-format", + metavar="", + help="Image disk format " + "(default: %s)" % DEFAULT_DISK_FORMAT, ) protected_group = parser.add_mutually_exclusive_group() protected_group.add_argument( "--protected", action="store_true", - help="Prevent image from being deleted" + help="Prevent image from being deleted", ) protected_group.add_argument( "--unprotected", action="store_true", - help="Allow image to be deleted (default)" + help="Allow image to be deleted (default)", ) public_group = parser.add_mutually_exclusive_group() public_group.add_argument( @@ -607,82 +603,55 @@ class SetImage(show.ShowOne): help="Image is inaccessible to the public (default)", ) parser.add_argument( - "--instance-uuid", - metavar="", - help="ID of instance used to create this image" + "--property", + dest="properties", + metavar="", + action=parseractions.KeyValueAction, + help="Set a property on this image " + "(repeat option to set multiple properties)", ) parser.add_argument( - "--min-disk", - type=int, - metavar="", - help="Minimum disk size needed to boot image, in gigabytes" + "--architecture", + metavar="", + help="Operating system architecture", ) - visibility_choices = ["public", "private"] - public_group.add_argument( - "--visibility", - metavar="", - choices=visibility_choices, - help=argparse.SUPPRESS + parser.add_argument( + "--instance-id", + metavar="", + help="ID of server instance used to create this image", + ) + parser.add_argument( + "--instance-uuid", + metavar="", + dest="instance_id", + help=argparse.SUPPRESS, ) - help_msg = ("ID of image in Glance that should be used as the kernel" - " when booting an AMI-style image") parser.add_argument( "--kernel-id", metavar="", - help=help_msg - ) - parser.add_argument( - "--os-version", - metavar="", - help="Operating system version as specified by the distributor" - ) - disk_choices = ["None", "ami", "ari", "aki", "vhd", "vmdk", "raw", - "qcow2", "vdi", "iso"] - help_msg = ("Format of the disk. Valid values: %s" % disk_choices) - parser.add_argument( - "--disk-format", - metavar="", - choices=disk_choices, - help=help_msg + help="ID of kernel image used to boot this disk image", ) parser.add_argument( "--os-distro", metavar="", - help="Common name of operating system distribution" + help="Operating system distribution name", ) parser.add_argument( - "--owner", - metavar="", - help="New Owner of the image" + "--os-version", + metavar="", + help="Operating system distribution version", ) - msg = ("ID of image stored in Glance that should be used as the " - "ramdisk when booting an AMI-style image") parser.add_argument( "--ramdisk-id", metavar="", - help=msg - ) - parser.add_argument( - "--min-ram", - type=int, - metavar="", - help="Amount of RAM (in MB) required to boot image" - ) - container_choices = ["None", "ami", "ari", "aki", "bare", "ovf", "ova"] - help_msg = ("Format of the container. Valid values: %s" - % container_choices) - parser.add_argument( - "--container-format", - metavar="", - choices=container_choices, - help=help_msg + help="ID of ramdisk image used to boot this disk image", ) for deadopt in self.deadopts: parser.add_argument( "--%s" % deadopt, metavar="<%s>" % deadopt, dest=deadopt.replace('-', '_'), - help=argparse.SUPPRESS + help=argparse.SUPPRESS, ) return parser @@ -698,10 +667,9 @@ class SetImage(show.ShowOne): kwargs = {} copy_attrs = ('architecture', 'container_format', 'disk_format', - 'file', 'kernel_id', 'locations', 'name', + 'file', 'instance_id', 'kernel_id', 'locations', 'min_disk', 'min_ram', 'name', 'os_distro', 'os_version', - 'owner', 'prefix', 'progress', 'ramdisk_id', - 'visibility') + 'owner', 'prefix', 'progress', 'ramdisk_id') for attr in copy_attrs: if attr in parsed_args: val = getattr(parsed_args, attr, None) @@ -710,6 +678,11 @@ class SetImage(show.ShowOne): # actually present on the command line kwargs[attr] = val + # Properties should get flattened into the general kwargs + if getattr(parsed_args, 'properties', None): + for k, v in six.iteritems(parsed_args.properties): + kwargs[k] = str(v) + # Handle exclusive booleans with care # Avoid including attributes in kwargs if an option is not # present on the command line. These exclusive booleans are not @@ -736,3 +709,30 @@ class SetImage(show.ShowOne): info = {} info.update(image) return zip(*sorted(six.iteritems(info))) + + +class ShowImage(show.ShowOne): + """Display image details""" + + log = logging.getLogger(__name__ + ".ShowImage") + + def get_parser(self, prog_name): + parser = super(ShowImage, self).get_parser(prog_name) + parser.add_argument( + "image", + metavar="", + help="Image to display (name or ID)", + ) + return parser + + def take_action(self, parsed_args): + self.log.debug("take_action(%s)", parsed_args) + + image_client = self.app.client_manager.image + image = utils.find_resource( + image_client.images, + parsed_args.image, + ) + + info = _format_image(image) + return zip(*sorted(six.iteritems(info))) diff --git a/openstackclient/tests/image/v2/test_image.py b/openstackclient/tests/image/v2/test_image.py index 65d5e555f6..ce2974d692 100644 --- a/openstackclient/tests/image/v2/test_image.py +++ b/openstackclient/tests/image/v2/test_image.py @@ -210,7 +210,7 @@ class TestImageCreate(TestImage): self.assertEqual(image_fakes.IMAGE_columns, columns) self.assertEqual(image_fakes.IMAGE_data, data) - def test_image_dead_options(self): + def test_image_create_dead_options(self): arglist = [ '--owner', 'nobody', @@ -639,6 +639,196 @@ class TestRemoveProjectImage(TestImage): ) +class TestImageSet(TestImage): + + def setUp(self): + super(TestImageSet, self).setUp() + # Set up the schema + self.model = warlock.model_factory( + image_fakes.IMAGE_schema, + schemas.SchemaBasedModel, + ) + + self.images_mock.get.return_value = self.model(**image_fakes.IMAGE) + self.images_mock.update.return_value = self.model(**image_fakes.IMAGE) + # Get the command object to test + self.cmd = image.SetImage(self.app, None) + + def test_image_set_options(self): + arglist = [ + '--name', 'new-name', + '--owner', 'new-owner', + '--min-disk', '2', + '--min-ram', '4', + '--container-format', 'ovf', + '--disk-format', 'vmdk', + image_fakes.image_id, + ] + verifylist = [ + ('name', 'new-name'), + ('owner', 'new-owner'), + ('min_disk', 2), + ('min_ram', 4), + ('container_format', 'ovf'), + ('disk_format', 'vmdk'), + ('image', image_fakes.image_id), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # DisplayCommandBase.take_action() returns two tuples + columns, data = self.cmd.take_action(parsed_args) + + kwargs = { + 'name': 'new-name', + 'owner': 'new-owner', + 'min_disk': 2, + 'min_ram': 4, + 'container_format': 'ovf', + 'disk_format': 'vmdk', + } + # ImageManager.update(image, **kwargs) + self.images_mock.update.assert_called_with( + image_fakes.image_id, **kwargs) + + self.assertEqual(image_fakes.IMAGE_columns, columns) + self.assertEqual(image_fakes.IMAGE_data, data) + + def test_image_set_bools1(self): + arglist = [ + '--protected', + '--private', + image_fakes.image_name, + ] + verifylist = [ + ('protected', True), + ('unprotected', False), + ('public', False), + ('private', True), + ('image', image_fakes.image_name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # DisplayCommandBase.take_action() returns two tuples + self.cmd.take_action(parsed_args) + + kwargs = { + 'protected': True, + 'visibility': 'private', + } + # ImageManager.update(image, **kwargs) + self.images_mock.update.assert_called_with( + image_fakes.image_id, + **kwargs + ) + + def test_image_set_bools2(self): + arglist = [ + '--unprotected', + '--public', + image_fakes.image_name, + ] + verifylist = [ + ('protected', False), + ('unprotected', True), + ('public', True), + ('private', False), + ('image', image_fakes.image_name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # DisplayCommandBase.take_action() returns two tuples + self.cmd.take_action(parsed_args) + + kwargs = { + 'protected': False, + 'visibility': 'public', + } + # ImageManager.update(image, **kwargs) + self.images_mock.update.assert_called_with( + image_fakes.image_id, + **kwargs + ) + + def test_image_set_properties(self): + arglist = [ + '--property', 'Alpha=1', + '--property', 'Beta=2', + image_fakes.image_name, + ] + verifylist = [ + ('properties', {'Alpha': '1', 'Beta': '2'}), + ('image', image_fakes.image_name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # DisplayCommandBase.take_action() returns two tuples + self.cmd.take_action(parsed_args) + + kwargs = { + 'Alpha': '1', + 'Beta': '2', + } + # ImageManager.update(image, **kwargs) + self.images_mock.update.assert_called_with( + image_fakes.image_id, + **kwargs + ) + + def test_image_set_fake_properties(self): + arglist = [ + '--architecture', 'z80', + '--instance-id', '12345', + '--kernel-id', '67890', + '--os-distro', 'cpm', + '--os-version', '2.2H', + '--ramdisk-id', 'xyzpdq', + image_fakes.image_name, + ] + verifylist = [ + ('architecture', 'z80'), + ('instance_id', '12345'), + ('kernel_id', '67890'), + ('os_distro', 'cpm'), + ('os_version', '2.2H'), + ('ramdisk_id', 'xyzpdq'), + ('image', image_fakes.image_name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # DisplayCommandBase.take_action() returns two tuples + self.cmd.take_action(parsed_args) + + kwargs = { + 'architecture': 'z80', + 'instance_id': '12345', + 'kernel_id': '67890', + 'os_distro': 'cpm', + 'os_version': '2.2H', + 'ramdisk_id': 'xyzpdq', + } + # ImageManager.update(image, **kwargs) + self.images_mock.update.assert_called_with( + image_fakes.image_id, + **kwargs + ) + + def test_image_set_dead_options(self): + + arglist = [ + '--visibility', '1-mile', + image_fakes.image_name, + ] + verifylist = [ + ('visibility', '1-mile'), + ('image', image_fakes.image_name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + self.assertRaises( + exceptions.CommandError, + self.cmd.take_action, parsed_args) + + class TestImageShow(TestImage): def setUp(self): @@ -672,52 +862,3 @@ class TestImageShow(TestImage): self.assertEqual(image_fakes.IMAGE_columns, columns) self.assertEqual(image_fakes.IMAGE_data, data) - - -class TestImageSet(TestImage): - - def setUp(self): - super(TestImageSet, self).setUp() - # Set up the schema - self.model = warlock.model_factory( - image_fakes.IMAGE_schema, - schemas.SchemaBasedModel, - ) - - self.images_mock.get.return_value = self.model(**image_fakes.IMAGE) - self.images_mock.update.return_value = self.model(**image_fakes.IMAGE) - # Get the command object to test - self.cmd = image.SetImage(self.app, None) - - def test_image_set_options(self): - arglist = [ - '--name', 'new-name', - '--owner', 'new-owner', - '--min-disk', '2', - '--min-ram', '4', - image_fakes.image_id, - ] - verifylist = [ - ('name', 'new-name'), - ('owner', 'new-owner'), - ('min_disk', 2), - ('min_ram', 4), - ('image', image_fakes.image_id), - ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) - - # DisplayCommandBase.take_action() returns two tuples - columns, data = self.cmd.take_action(parsed_args) - - kwargs = { - 'name': 'new-name', - 'owner': 'new-owner', - 'min_disk': 2, - 'min_ram': 4 - } - # ImageManager.update(image, **kwargs) - self.images_mock.update.assert_called_with( - image_fakes.image_id, **kwargs) - - self.assertEqual(image_fakes.IMAGE_columns, columns) - self.assertEqual(image_fakes.IMAGE_data, data)