Support "--no-property" option in volume snapshot set

Supporting "--no-property" option will apply user a convenient
way to clean all properties of volume snapshot in a short command,
and this kind of behavior is the recommended way to devref.
The patch adds "--no-property" option in "volume snapshot set" command,
and update related test cases and devref document.

Change-Id: I5f10cc2b5814553699920c4343995b2e11416e4e
Implements: blueprint allow-overwrite-set-options

doc/source/command-objects/volume-snapshot.rst

@@ -133,6 +133,7 @@ Set volume snapshot properties
     openstack volume snapshot set
         [--name <name>]
         [--description <description>]
+        [--no-property]
         [--property <key=value> [...] ]
         [--state <state>]
         <snapshot>
@@ -145,6 +146,12 @@ Set volume snapshot properties
 
     New snapshot description
 
+.. option:: --no-property
+
+    Remove all properties from :ref:`\<snapshot\> <volume_snapshot_set-snapshot>`
+    (specify both :option:`--no-property` and :option:`--property` to
+    remove the current properties before setting new properties.)
+
 .. option:: --property <key=value>
 
     Property to add or modify for this snapshot (repeat option to set multiple properties)

openstackclient/tests/functional/volume/v1/test_snapshot.py

@@ -10,6 +10,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import json
 import time
 import uuid
 
@@ -20,9 +21,6 @@ class VolumeSnapshotTests(common.BaseVolumeTests):
     """Functional tests for volume snapshot. """
 
     VOLLY = uuid.uuid4().hex
-    NAME = uuid.uuid4().hex
-    OTHER_NAME = uuid.uuid4().hex
-    HEADERS = ['"Name"']
 
     @classmethod
     def wait_for_status(cls, command, status, tries):
@@ -30,59 +28,223 @@ class VolumeSnapshotTests(common.BaseVolumeTests):
         for attempt in range(tries):
             time.sleep(1)
             raw_output = cls.openstack(command + opts)
-            if (raw_output == status):
+            if (raw_output.rstrip() == status):
                 return
         cls.assertOutput(status, raw_output)
 
     @classmethod
     def setUpClass(cls):
         super(VolumeSnapshotTests, cls).setUpClass()
-        cls.openstack('volume create --size 1 ' + cls.VOLLY)
+        # create a volume for all tests to create snapshot
-        cls.wait_for_status('volume show ' + cls.VOLLY, 'available\n', 3)
+        cmd_output = json.loads(cls.openstack(
-        opts = cls.get_opts(['status'])
+            'volume create -f json ' +
-        raw_output = cls.openstack('volume snapshot create --volume ' +
+            '--size 1 ' +
-                                   cls.VOLLY + ' ' + cls.NAME + opts)
+            cls.VOLLY
-        cls.assertOutput('creating\n', raw_output)
+        ))
-        cls.wait_for_status(
+        cls.wait_for_status('volume show ' + cls.VOLLY, 'available', 6)
-            'volume snapshot show ' + cls.NAME, 'available\n', 3)
+        cls.VOLUME_ID = cmd_output['id']
 
     @classmethod
     def tearDownClass(cls):
-        # Rename test
+        cls.wait_for_status('volume show ' + cls.VOLLY, 'available', 6)
-        raw_output = cls.openstack(
+        raw_output = cls.openstack('volume delete --force ' + cls.VOLLY)
-            'volume snapshot set --name ' + cls.OTHER_NAME + ' ' + cls.NAME)
         cls.assertOutput('', raw_output)
-        # Delete test
-        raw_output_snapshot = cls.openstack(
-            'volume snapshot delete ' + cls.OTHER_NAME)
-        cls.wait_for_status('volume show ' + cls.VOLLY, 'available\n', 6)
-        raw_output_volume = cls.openstack('volume delete --force ' + cls.VOLLY)
-        cls.assertOutput('', raw_output_snapshot)
-        cls.assertOutput('', raw_output_volume)
 
-    def test_snapshot_list(self):
+    def test_volume_snapshot__delete(self):
-        opts = self.get_opts(self.HEADERS)
+        """Test create, delete multiple"""
-        raw_output = self.openstack('volume snapshot list' + opts)
+        name1 = uuid.uuid4().hex
-        self.assertIn(self.NAME, raw_output)
+        cmd_output = json.loads(self.openstack(
+            'volume snapshot create -f json ' +
+            name1 +
+            ' --volume ' + self.VOLLY
+        ))
+        self.assertEqual(
+            name1,
+            cmd_output["display_name"],
+        )
 
-    def test_snapshot_set_unset_properties(self):
+        name2 = uuid.uuid4().hex
+        cmd_output = json.loads(self.openstack(
+            'volume snapshot create -f json ' +
+            name2 +
+            ' --volume ' + self.VOLLY
+        ))
+        self.assertEqual(
+            name2,
+            cmd_output["display_name"],
+        )
+
+        self.wait_for_status(
+            'volume snapshot show ' + name1, 'available', 6)
+        self.wait_for_status(
+            'volume snapshot show ' + name2, 'available', 6)
+
+        del_output = self.openstack(
+            'volume snapshot delete ' + name1 + ' ' + name2)
+        self.assertOutput('', del_output)
+
+    def test_volume_snapshot_list(self):
+        """Test create, list filter"""
+        name1 = uuid.uuid4().hex
+        cmd_output = json.loads(self.openstack(
+            'volume snapshot create -f json ' +
+            name1 +
+            ' --volume ' + self.VOLLY
+        ))
+        self.addCleanup(self.openstack, 'volume snapshot delete ' + name1)
+        self.assertEqual(
+            name1,
+            cmd_output["display_name"],
+        )
+        self.assertEqual(
+            self.VOLUME_ID,
+            cmd_output["volume_id"],
+        )
+        self.assertEqual(
+            1,
+            cmd_output["size"],
+        )
+        self.wait_for_status(
+            'volume snapshot show ' + name1, 'available', 6)
+
+        name2 = uuid.uuid4().hex
+        cmd_output = json.loads(self.openstack(
+            'volume snapshot create -f json ' +
+            name2 +
+            ' --volume ' + self.VOLLY
+        ))
+        self.addCleanup(self.openstack, 'volume snapshot delete ' + name2)
+        self.assertEqual(
+            name2,
+            cmd_output["display_name"],
+        )
+        self.assertEqual(
+            self.VOLUME_ID,
+            cmd_output["volume_id"],
+        )
+        self.assertEqual(
+            1,
+            cmd_output["size"],
+        )
+        self.wait_for_status(
+            'volume snapshot show ' + name2, 'available', 6)
+
+        # Test list --long, --status
+        cmd_output = json.loads(self.openstack(
+            'volume snapshot list -f json ' +
+            '--long ' +
+            '--status error'
+        ))
+        names = [x["Name"] for x in cmd_output]
+        self.assertNotIn(name1, names)
+        self.assertNotIn(name2, names)
+
+        # Test list --volume
+        cmd_output = json.loads(self.openstack(
+            'volume snapshot list -f json ' +
+            '--volume ' + self.VOLLY
+        ))
+        names = [x["Name"] for x in cmd_output]
+        self.assertIn(name1, names)
+        self.assertIn(name2, names)
+
+        # Test list --name
+        cmd_output = json.loads(self.openstack(
+            'volume snapshot list -f json ' +
+            '--name ' + name1
+        ))
+        names = [x["Name"] for x in cmd_output]
+        self.assertIn(name1, names)
+        self.assertNotIn(name2, names)
+
+    def test_snapshot_set(self):
+        """Test create, set, unset, show, delete volume snapshot"""
+        name = uuid.uuid4().hex
+        new_name = name + "_"
+        cmd_output = json.loads(self.openstack(
+            'volume snapshot create -f json ' +
+            '--volume ' + self.VOLLY +
+            ' --description aaaa ' +
+            name
+        ))
+        self.addCleanup(self.openstack, 'volume snapshot delete ' + new_name)
+        self.assertEqual(
+            name,
+            cmd_output["display_name"],
+        )
+        self.assertEqual(
+            1,
+            cmd_output["size"],
+        )
+        self.assertEqual(
+            'aaaa',
+            cmd_output["display_description"],
+        )
+        self.wait_for_status(
+            'volume snapshot show ' + name, 'available', 6)
+
+        # Test volume snapshot set
         raw_output = self.openstack(
-            'volume snapshot set --property a=b --property c=d ' + self.NAME)
+            'volume snapshot set ' +
-        self.assertEqual("", raw_output)
+            '--name ' + new_name +
-        opts = self.get_opts(["properties"])
+            ' --description bbbb ' +
-        raw_output = self.openstack('volume snapshot show ' + self.NAME + opts)
+            '--property Alpha=a ' +
-        self.assertEqual("a='b', c='d'\n", raw_output)
+            '--property Beta=b ' +
+            name,
+        )
+        self.assertOutput('', raw_output)
 
-        raw_output = self.openstack(
+        # Show snapshot set result
-            'volume snapshot unset --property a ' + self.NAME)
+        cmd_output = json.loads(self.openstack(
-        self.assertEqual("", raw_output)
+            'volume snapshot show -f json ' +
-        raw_output = self.openstack('volume snapshot show ' + self.NAME + opts)
+            new_name
-        self.assertEqual("c='d'\n", raw_output)
+        ))
+        self.assertEqual(
+            new_name,
+            cmd_output["display_name"],
+        )
+        self.assertEqual(
+            1,
+            cmd_output["size"],
+        )
+        self.assertEqual(
+            'bbbb',
+            cmd_output["display_description"],
+        )
+        self.assertEqual(
+            "Alpha='a', Beta='b'",
+            cmd_output["properties"],
+        )
 
-    def test_snapshot_set_description(self):
+        # Test volume unset
         raw_output = self.openstack(
-            'volume snapshot set --description backup ' + self.NAME)
+            'volume snapshot unset ' +
-        self.assertEqual("", raw_output)
+            '--property Alpha ' +
-        opts = self.get_opts(["display_description", "display_name"])
+            new_name,
-        raw_output = self.openstack('volume snapshot show ' + self.NAME + opts)
+        )
-        self.assertEqual("backup\n" + self.NAME + "\n", raw_output)
+        self.assertOutput('', raw_output)
+
+        cmd_output = json.loads(self.openstack(
+            'volume snapshot show -f json ' +
+            new_name
+        ))
+        self.assertEqual(
+            "Beta='b'",
+            cmd_output["properties"],
+        )
+
+        # Test volume snapshot set --no-property
+        raw_output = self.openstack(
+            'volume snapshot set ' +
+            '--no-property ' +
+            new_name,
+        )
+        self.assertOutput('', raw_output)
+        cmd_output = json.loads(self.openstack(
+            'volume snapshot show -f json ' +
+            new_name
+        ))
+        self.assertNotIn(
+            "Beta='b'",
+            cmd_output["properties"],
+        )

openstackclient/tests/functional/volume/v2/test_snapshot.py

@@ -28,7 +28,7 @@ class VolumeSnapshotTests(common.BaseVolumeTests):
         for attempt in range(tries):
             time.sleep(1)
             raw_output = cls.openstack(command + opts)
-            if (raw_output == status):
+            if (raw_output.rstrip() == status):
                 return
         cls.assertOutput(status, raw_output)
 
@@ -41,12 +41,12 @@ class VolumeSnapshotTests(common.BaseVolumeTests):
             '--size 1 ' +
             cls.VOLLY
         ))
-        cls.wait_for_status('volume show ' + cls.VOLLY, 'available\n', 6)
+        cls.wait_for_status('volume show ' + cls.VOLLY, 'available', 6)
         cls.VOLUME_ID = cmd_output['id']
 
     @classmethod
     def tearDownClass(cls):
-        cls.wait_for_status('volume show ' + cls.VOLLY, 'available\n', 6)
+        cls.wait_for_status('volume show ' + cls.VOLLY, 'available', 6)
         raw_output = cls.openstack('volume delete --force ' + cls.VOLLY)
         cls.assertOutput('', raw_output)
 
@@ -75,9 +75,9 @@ class VolumeSnapshotTests(common.BaseVolumeTests):
         )
 
         self.wait_for_status(
-            'volume snapshot show ' + name1, 'available\n', 6)
+            'volume snapshot show ' + name1, 'available', 6)
         self.wait_for_status(
-            'volume snapshot show ' + name2, 'available\n', 6)
+            'volume snapshot show ' + name2, 'available', 6)
 
         del_output = self.openstack(
             'volume snapshot delete ' + name1 + ' ' + name2)
@@ -105,7 +105,7 @@ class VolumeSnapshotTests(common.BaseVolumeTests):
             cmd_output["size"],
         )
         self.wait_for_status(
-            'volume snapshot show ' + name1, 'available\n', 6)
+            'volume snapshot show ' + name1, 'available', 6)
 
         name2 = uuid.uuid4().hex
         cmd_output = json.loads(self.openstack(
@@ -127,7 +127,7 @@ class VolumeSnapshotTests(common.BaseVolumeTests):
             cmd_output["size"],
         )
         self.wait_for_status(
-            'volume snapshot show ' + name2, 'available\n', 6)
+            'volume snapshot show ' + name2, 'available', 6)
         raw_output = self.openstack(
             'volume snapshot set ' +
             '--state error ' +
@@ -163,7 +163,7 @@ class VolumeSnapshotTests(common.BaseVolumeTests):
         self.assertIn(name1, names)
         self.assertNotIn(name2, names)
 
-    def test_snapshot_set(self):
+    def test_volume_snapshot_set(self):
         """Test create, set, unset, show, delete volume snapshot"""
         name = uuid.uuid4().hex
         new_name = name + "_"
@@ -192,7 +192,7 @@ class VolumeSnapshotTests(common.BaseVolumeTests):
             cmd_output["properties"],
         )
         self.wait_for_status(
-            'volume snapshot show ' + name, 'available\n', 6)
+            'volume snapshot show ' + name, 'available', 6)
 
         # Test volume snapshot set
         raw_output = self.openstack(
@@ -227,7 +227,7 @@ class VolumeSnapshotTests(common.BaseVolumeTests):
             cmd_output["properties"],
         )
 
-        # Test volume unset
+        # Test volume snapshot unset
         raw_output = self.openstack(
             'volume snapshot unset ' +
             '--property Alpha ' +
@@ -243,3 +243,19 @@ class VolumeSnapshotTests(common.BaseVolumeTests):
             "Beta='b'",
             cmd_output["properties"],
         )
+
+        # Test volume snapshot set --no-property
+        raw_output = self.openstack(
+            'volume snapshot set ' +
+            '--no-property ' +
+            new_name,
+        )
+        self.assertOutput('', raw_output)
+        cmd_output = json.loads(self.openstack(
+            'volume snapshot show -f json ' +
+            new_name
+        ))
+        self.assertNotIn(
+            "Beta='b'",
+            cmd_output["properties"],
+        )

openstackclient/tests/unit/volume/v1/test_snapshot.py

@@ -429,15 +429,17 @@ class TestSnapshotSet(TestSnapshot):
         arglist = [
             "--name", "new_snapshot",
             "--description", "new_description",
-            "--property", "x=y",
+            "--property", "foo_1=foo_1",
-            "--property", "foo=foo",
+            "--property", "foo_2=foo_2",
+            "--no-property",
             self.snapshot.id,
         ]
-        new_property = {"x": "y", "foo": "foo"}
+        new_property = {"foo_1": "foo_1", "foo_2": "foo_2"}
         verifylist = [
             ("name", "new_snapshot"),
             ("description", "new_description"),
             ("property", new_property),
+            ("no_property", True),
             ("snapshot", self.snapshot.id),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@@ -449,8 +451,11 @@ class TestSnapshotSet(TestSnapshot):
             "display_description": "new_description",
         }
         self.snapshot.update.assert_called_with(**kwargs)
+        self.snapshots_mock.delete_metadata.assert_called_with(
+            self.snapshot.id, ["foo"]
+        )
         self.snapshots_mock.set_metadata.assert_called_with(
-            self.snapshot.id, new_property
+            self.snapshot.id, {"foo_2": "foo_2", "foo_1": "foo_1"}
         )
         self.assertIsNone(result)
 

openstackclient/tests/unit/volume/v2/test_snapshot.py

@@ -499,7 +499,23 @@ class TestSnapshotSet(TestSnapshot):
         # Get the command object to mock
         self.cmd = volume_snapshot.SetVolumeSnapshot(self.app, None)
 
-    def test_snapshot_set(self):
+    def test_snapshot_set_no_option(self):
+        arglist = [
+            self.snapshot.id,
+        ]
+        verifylist = [
+            ("snapshot", self.snapshot.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+        self.snapshots_mock.get.assert_called_once_with(parsed_args.snapshot)
+        self.assertNotCalled(self.snapshots_mock.reset_state)
+        self.assertNotCalled(self.snapshots_mock.update)
+        self.assertNotCalled(self.snapshots_mock.set_metadata)
+        self.assertIsNone(result)
+
+    def test_snapshot_set_name_and_property(self):
         arglist = [
             "--name", "new_snapshot",
             "--property", "x=y",
@@ -526,6 +542,51 @@ class TestSnapshotSet(TestSnapshot):
         )
         self.assertIsNone(result)
 
+    def test_snapshot_set_with_no_property(self):
+        arglist = [
+            "--no-property",
+            self.snapshot.id,
+        ]
+        verifylist = [
+            ("no_property", True),
+            ("snapshot", self.snapshot.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+        self.snapshots_mock.get.assert_called_once_with(parsed_args.snapshot)
+        self.assertNotCalled(self.snapshots_mock.reset_state)
+        self.assertNotCalled(self.snapshots_mock.update)
+        self.assertNotCalled(self.snapshots_mock.set_metadata)
+        self.snapshots_mock.delete_metadata.assert_called_with(
+            self.snapshot.id, ["foo"]
+        )
+        self.assertIsNone(result)
+
+    def test_snapshot_set_with_no_property_and_property(self):
+        arglist = [
+            "--no-property",
+            "--property", "foo_1=bar_1",
+            self.snapshot.id,
+        ]
+        verifylist = [
+            ("no_property", True),
+            ("property", {"foo_1": "bar_1"}),
+            ("snapshot", self.snapshot.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+        self.snapshots_mock.get.assert_called_once_with(parsed_args.snapshot)
+        self.assertNotCalled(self.snapshots_mock.reset_state)
+        self.assertNotCalled(self.snapshots_mock.update)
+        self.snapshots_mock.delete_metadata.assert_called_with(
+            self.snapshot.id, ["foo"]
+        )
+        self.snapshots_mock.set_metadata.assert_called_once_with(
+            self.snapshot.id, {"foo_1": "bar_1"})
+        self.assertIsNone(result)
+
     def test_snapshot_set_state_to_error(self):
         arglist = [
             "--state", "error",

openstackclient/volume/v1/volume_snapshot.py

@@ -239,6 +239,15 @@ class SetVolumeSnapshot(command.Command):
             metavar='<description>',
             help=_('New snapshot description')
         )
+        parser.add_argument(
+            "--no-property",
+            dest="no_property",
+            action="store_true",
+            help=_("Remove all properties from <snapshot> "
+                   "(specify both --no-property and --property to "
+                   "remove the current properties before setting "
+                   "new properties.)"),
+        )
         parser.add_argument(
             '--property',
             metavar='<key=value>',
@@ -254,6 +263,17 @@ class SetVolumeSnapshot(command.Command):
                                        parsed_args.snapshot)
 
         result = 0
+        if parsed_args.no_property:
+            try:
+                key_list = snapshot.metadata.keys()
+                volume_client.volume_snapshots.delete_metadata(
+                    snapshot.id,
+                    list(key_list),
+                )
+            except Exception as e:
+                LOG.error(_("Failed to clean snapshot properties: %s"), e)
+                result += 1
+
         if parsed_args.property:
             try:
                 volume_client.volume_snapshots.set_metadata(

openstackclient/volume/v2/volume_snapshot.py

@@ -285,6 +285,15 @@ class SetVolumeSnapshot(command.Command):
             metavar='<description>',
             help=_('New snapshot description')
         )
+        parser.add_argument(
+            "--no-property",
+            dest="no_property",
+            action="store_true",
+            help=_("Remove all properties from <snapshot> "
+                   "(specify both --no-property and --property to "
+                   "remove the current properties before setting "
+                   "new properties.)"),
+        )
         parser.add_argument(
             '--property',
             metavar='<key=value>',
@@ -311,6 +320,17 @@ class SetVolumeSnapshot(command.Command):
                                        parsed_args.snapshot)
 
         result = 0
+        if parsed_args.no_property:
+            try:
+                key_list = snapshot.metadata.keys()
+                volume_client.volume_snapshots.delete_metadata(
+                    snapshot.id,
+                    list(key_list),
+                )
+            except Exception as e:
+                LOG.error(_("Failed to clean snapshot properties: %s"), e)
+                result += 1
+
         if parsed_args.property:
             try:
                 volume_client.volume_snapshots.set_metadata(

releasenotes/notes/support-no-property-in-volume-snapshot-0af3fcb31a3cfc2b.yaml

@@ -0,0 +1,7 @@
+---
+features:
+  - |
+    Add ``--no-property`` option in ``volume snapshot set``.
+    Supporting ``--no-property`` option will apply user a convenient way to
+    clean all properties of volume snapshot in a short command.
+    [ Blueprint  `allow-overwrite-set-options <https://blueprints.launchpad.net/python-openstackclient/+spec/allow-overwrite-set-options>` _]