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

3 files changed, 284 insertions, 144 deletions

diff --git a/doc/source/command-objects/image.rst b/doc/source/command-objects/image.rst
index d2698b0b..8dce3662 100644
--- a/doc/source/command-objects/image.rst
+++ b/doc/source/command-objects/image.rst
@@ -238,6 +238,12 @@ Set image properties
         [--checksum <checksum>]
         [--stdin]
         [--property <key=value> [...] ]
+        [--architecture <architecture>]
+        [--instance-id <instance-id>]
+        [--kernel-id <kernel-id>]
+        [--os-distro <os-distro>]
+        [--os-version <os-version>]
+        [--ramdisk-id <ramdisk-id>]
         <image>
 
 .. option:: --name <name>
@@ -258,14 +264,11 @@ Set image properties
 
 .. option:: --container-format <container-format>
 
-    Container format of image.
-    Acceptable formats: ['ami', 'ari', 'aki', 'bare', 'ovf']
+    Image container format (default: bare)
 
 .. option:: --disk-format <disk-format>
 
-    Disk format of image.
-    Acceptable formats: ['ami', 'ari', 'aki', 'vhd', 'vmdk', 'raw', 'qcow2',
-    'vdi', 'iso']
+    Image disk format (default: raw)
 
 .. option:: --size <size>
 
@@ -339,45 +342,41 @@ Set image properties
 
 .. option:: --property <key=value>
 
-    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 <architecture>
 
-    Operating system Architecture
+    Operating system architecture
 
     .. versionadded:: 2
 
-.. option:: --ramdisk-id <ramdisk-id>
+.. option:: --instance-id <instance-id>
 
-    ID of image stored in Glance that should be used as
-    the ramdisk when booting an AMI-style image
+    ID of server instance used to create this image
 
     .. versionadded:: 2
 
-.. option:: --os-distro <os-distro>
+.. option:: --kernel-id <kernel-id>
 
-    Common name of operating system distribution
+    ID of kernel image used to boot this disk image
 
     .. versionadded:: 2
 
-.. option:: --os-version <os-version>
+.. option:: --os-distro <os-distro>
 
-    Operating system version as specified by the distributor
+    Operating system distribution name
 
     .. versionadded:: 2
 
-.. option:: --kernel-id <kernel-id>
+.. option:: --os-version <os-version>
 
-    ID of image in Glance that should be used as the
-    kernel when booting an AMI-style image
+    Operating system distribution version
 
     .. versionadded:: 2
 
-.. option:: --instance-uuid <instance_uuid>
+.. option:: --ramdisk-id <ramdisk-id>
 
-    ID of instance used to create this image
+    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 fff26c02..11c7483b 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="<image>",
-            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="<architecture>",
-            help="Operating system Architecture"
+            "--owner",
+            metavar="<project>",
+            help="New image owner project (name or ID)",
+        )
+        parser.add_argument(
+            "--min-disk",
+            type=int,
+            metavar="<disk-gb>",
+            help="Minimum disk size needed to boot image, in gigabytes"
+        )
+        parser.add_argument(
+            "--min-ram",
+            type=int,
+            metavar="<ram-mb>",
+            help="Minimum RAM size needed to boot image, in megabytes",
+        )
+        parser.add_argument(
+            "--container-format",
+            metavar="<container-format>",
+            help="Image container format "
+                 "(default: %s)" % DEFAULT_CONTAINER_FORMAT,
+        )
+        parser.add_argument(
+            "--disk-format",
+            metavar="<disk-format>",
+            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="<instance_uuid>",
-            help="ID of instance used to create this image"
+            "--property",
+            dest="properties",
+            metavar="<key=value>",
+            action=parseractions.KeyValueAction,
+            help="Set a property on this image "
+                 "(repeat option to set multiple properties)",
         )
         parser.add_argument(
-            "--min-disk",
-            type=int,
-            metavar="<disk-gb>",
-            help="Minimum disk size needed to boot image, in gigabytes"
-        )
-        visibility_choices = ["public", "private"]
-        public_group.add_argument(
-            "--visibility",
-            metavar="<visibility>",
-            choices=visibility_choices,
-            help=argparse.SUPPRESS
+            "--architecture",
+            metavar="<architecture>",
+            help="Operating system architecture",
         )
-        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="<kernel-id>",
-            help=help_msg
+            "--instance-id",
+            metavar="<instance-id>",
+            help="ID of server instance used to create this image",
         )
         parser.add_argument(
-            "--os-version",
-            metavar="<os-version>",
-            help="Operating system version as specified by the distributor"
+            "--instance-uuid",
+            metavar="<instance-id>",
+            dest="instance_id",
+            help=argparse.SUPPRESS,
         )
-        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="<disk-format>",
-            choices=disk_choices,
-            help=help_msg
+            "--kernel-id",
+            metavar="<kernel-id>",
+            help="ID of kernel image used to boot this disk image",
         )
         parser.add_argument(
             "--os-distro",
             metavar="<os-distro>",
-            help="Common name of operating system distribution"
+            help="Operating system distribution name",
         )
         parser.add_argument(
-            "--owner",
-            metavar="<owner>",
-            help="New Owner of the image"
+            "--os-version",
+            metavar="<os-version>",
+            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="<ramdisk-id>",
-            help=msg
-        )
-        parser.add_argument(
-            "--min-ram",
-            type=int,
-            metavar="<ram-mb>",
-            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="<container-format>",
-            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="<image>",
+            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 65d5e555..ce2974d6 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,11 +639,10 @@ class TestRemoveProjectImage(TestImage):
         )
 
 
-class TestImageShow(TestImage):
+class TestImageSet(TestImage):
 
     def setUp(self):
-        super(TestImageShow, self).setUp()
-
+        super(TestImageSet, self).setUp()
         # Set up the schema
         self.model = warlock.model_factory(
             image_fakes.IMAGE_schema,
@@ -651,33 +650,190 @@ class TestImageShow(TestImage):
         )
 
         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.ShowImage(self.app, None)
+        self.cmd = image.SetImage(self.app, None)
 
-    def test_image_show(self):
+    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)
-        self.images_mock.get.assert_called_with(
-            image_fakes.image_id,
-        )
+
+        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)
 
-class TestImageSet(TestImage):
+        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):
-        super(TestImageSet, self).setUp()
+        super(TestImageShow, self).setUp()
+
         # Set up the schema
         self.model = warlock.model_factory(
             image_fakes.IMAGE_schema,
@@ -685,39 +841,24 @@ class TestImageSet(TestImage):
         )
 
         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)
+        self.cmd = image.ShowImage(self.app, None)
 
-    def test_image_set_options(self):
+    def test_image_show(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.images_mock.get.assert_called_with(
+            image_fakes.image_id,
+        )
 
         self.assertEqual(image_fakes.IMAGE_columns, columns)
         self.assertEqual(image_fakes.IMAGE_data, data)