volume: Add more missing 'volume backup *' options

Add an additional '--no-property' option to the 'volume backup set'
command, along with a brand spanking new 'volume backup unset' command.

Change-Id: Id7ca925e0ada03e259f0ecaf3e02af11c900641e
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>

Author: stephenfin

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

@@ -17,6 +17,7 @@
 from unittest import mock
 import uuid
 
+from cinderclient import api_versions
 from osc_lib.cli import format_columns
 
 from openstackclient.tests.unit import fakes
@@ -292,6 +293,8 @@ class FakeVolumeClient(object):
     def __init__(self, **kwargs):
         self.auth_token = kwargs['token']
         self.management_url = kwargs['endpoint']
+        self.api_version = api_versions.APIVersion('2.0')
+
         self.availability_zones = mock.Mock()
         self.availability_zones.resource_class = fakes.FakeResource(None, {})
         self.backups = mock.Mock()

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

@@ -490,7 +490,9 @@ def test_backup_restore(self):
 
 class TestBackupSet(TestBackup):
 
-    backup = volume_fakes.FakeBackup.create_one_backup()
+    backup = volume_fakes.FakeBackup.create_one_backup(
+        attrs={'metadata': {'wow': 'cool'}},
+    )
 
     def setUp(self):
         super(TestBackupSet, self).setUp()
@@ -627,6 +629,159 @@ def test_backup_set_state_failed(self):
         self.backups_mock.reset_state.assert_called_with(
             self.backup.id, 'error')
 
+    def test_backup_set_no_property(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.43')
+
+        arglist = [
+            '--no-property',
+            self.backup.id,
+        ]
+        verifylist = [
+            ('no_property', True),
+            ('backup', self.backup.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'metadata': {},
+        }
+        self.backups_mock.update.assert_called_once_with(
+            self.backup.id,
+            **kwargs
+        )
+        self.assertIsNone(result)
+
+    def test_backup_set_no_property_pre_v343(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.42')
+
+        arglist = [
+            '--no-property',
+            self.backup.id,
+        ]
+        verifylist = [
+            ('no_property', True),
+            ('backup', self.backup.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        exc = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn("--os-volume-api-version 3.43 or greater", str(exc))
+
+    def test_backup_set_property(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.43')
+
+        arglist = [
+            '--property', 'foo=bar',
+            self.backup.id,
+        ]
+        verifylist = [
+            ('properties', {'foo': 'bar'}),
+            ('backup', self.backup.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'metadata': {'wow': 'cool', 'foo': 'bar'},
+        }
+        self.backups_mock.update.assert_called_once_with(
+            self.backup.id,
+            **kwargs
+        )
+        self.assertIsNone(result)
+
+    def test_backup_set_property_pre_v343(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.42')
+
+        arglist = [
+            '--property', 'foo=bar',
+            self.backup.id,
+        ]
+        verifylist = [
+            ('properties', {'foo': 'bar'}),
+            ('backup', self.backup.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        exc = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn("--os-volume-api-version 3.43 or greater", str(exc))
+
+
+class TestBackupUnset(TestBackup):
+
+    backup = volume_fakes.FakeBackup.create_one_backup(
+        attrs={'metadata': {'foo': 'bar'}},
+    )
+
+    def setUp(self):
+        super().setUp()
+
+        self.backups_mock.get.return_value = self.backup
+
+        # Get the command object to test
+        self.cmd = volume_backup.UnsetVolumeBackup(self.app, None)
+
+    def test_backup_unset_property(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.43')
+
+        arglist = [
+            '--property', 'foo',
+            self.backup.id,
+        ]
+        verifylist = [
+            ('properties', ['foo']),
+            ('backup', self.backup.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = {
+            'metadata': {},
+        }
+        self.backups_mock.update.assert_called_once_with(
+            self.backup.id,
+            **kwargs
+        )
+        self.assertIsNone(result)
+
+    def test_backup_unset_property_pre_v343(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.42')
+
+        arglist = [
+            '--property', 'foo',
+            self.backup.id,
+        ]
+        verifylist = [
+            ('properties', ['foo']),
+            ('backup', self.backup.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        exc = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action,
+            parsed_args)
+        self.assertIn("--os-volume-api-version 3.43 or greater", str(exc))
+
 
 class TestBackupShow(TestBackup):
 

openstackclient/volume/v2/volume_backup.py

@@ -14,6 +14,7 @@
 
 """Volume v2 Backup action implementations"""
 
+import copy
 import functools
 import logging
 
@@ -405,11 +406,21 @@ def get_parser(self, prog_name):
                 'exercise caution when using)'
             ),
         )
+        parser.add_argument(
+            '--no-property',
+            action='store_true',
+            help=_(
+                'Remove all properties from this backup '
+                '(specify both --no-property and --property to remove the '
+                'current properties before setting new properties)'
+            ),
+        )
         parser.add_argument(
             '--property',
             metavar='<key=value>',
             action=parseractions.KeyValueAction,
             dest='properties',
+            default={},
             help=_(
                 'Set a property on this backup '
                 '(repeat option to set multiple values) '
@@ -454,6 +465,14 @@ def take_action(self, parsed_args):
 
             kwargs['description'] = parsed_args.description
 
+        if parsed_args.no_property:
+            if volume_client.api_version < api_versions.APIVersion('3.43'):
+                msg = _(
+                    '--os-volume-api-version 3.43 or greater is required to '
+                    'support the --no-property option'
+                )
+                raise exceptions.CommandError(msg)
+
         if parsed_args.properties:
             if volume_client.api_version < api_versions.APIVersion('3.43'):
                 msg = _(
@@ -462,20 +481,84 @@ def take_action(self, parsed_args):
                 )
                 raise exceptions.CommandError(msg)
 
-            kwargs['metadata'] = parsed_args.properties
+        if volume_client.api_version >= api_versions.APIVersion('3.43'):
+            metadata = copy.deepcopy(backup.metadata)
+
+            if parsed_args.no_property:
+                metadata = {}
+
+            metadata.update(parsed_args.properties)
+            kwargs['metadata'] = metadata
 
         if kwargs:
             try:
                 volume_client.backups.update(backup.id, **kwargs)
             except Exception as e:
-                LOG.error("Failed to update backup name or description: %s", e)
+                LOG.error("Failed to update backup: %s", e)
                 result += 1
 
         if result > 0:
             msg = _("One or more of the set operations failed")
             raise exceptions.CommandError(msg)
 
 
+class UnsetVolumeBackup(command.Command):
+    """Unset volume backup properties.
+
+    This command requires ``--os-volume-api-version`` 3.43 or greater.
+    """
+
+    def get_parser(self, prog_name):
+        parser = super().get_parser(prog_name)
+        parser.add_argument(
+            'backup',
+            metavar='<backup>',
+            help=_('Backup to modify (name or ID)')
+        )
+        parser.add_argument(
+            '--property',
+            metavar='<key>',
+            action='append',
+            dest='properties',
+            help=_(
+                'Property to remove from this backup '
+                '(repeat option to unset multiple values) '
+            ),
+        )
+        return parser
+
+    def take_action(self, parsed_args):
+        volume_client = self.app.client_manager.volume
+
+        if volume_client.api_version < api_versions.APIVersion('3.43'):
+            msg = _(
+                '--os-volume-api-version 3.43 or greater is required to '
+                'support the --property option'
+            )
+            raise exceptions.CommandError(msg)
+
+        backup = utils.find_resource(
+            volume_client.backups, parsed_args.backup)
+        metadata = copy.deepcopy(backup.metadata)
+
+        for key in parsed_args.properties:
+            if key not in metadata:
+                # ignore invalid properties but continue
+                LOG.warning(
+                    "'%s' is not a valid property for backup '%s'",
+                    key, parsed_args.backup,
+                )
+                continue
+
+            del metadata[key]
+
+        kwargs = {
+            'metadata': metadata,
+        }
+
+        volume_client.backups.update(backup.id, **kwargs)
+
+
 class ShowVolumeBackup(command.ShowOne):
     _description = _("Display volume backup details")
 

releasenotes/notes/add-missing-volume-backup-opts-b9246aded87427ce.yaml

@@ -6,8 +6,12 @@ features:
     non-incremental backup, set a metadata property on the created backup, and
     set an availability zone on the created backup, respectively.
   - |
-    Add ``--property`` option the ``volume backup set`` command to set a
-    metadata property on an existing backup.
+    Add ``--property`` and ``--no-property`` options to the
+    ``volume backup set`` command to set a metadata property or remove all
+    metadata properties from an existing backup.
+  - |
+    Add new ``volume backup unset`` command to allow unsetting of properties
+    from an existing volume backup.
 fixes:
   - |
     The ``--name`` and ``--description`` options of the ``volume backup set``

setup.cfg

@@ -709,6 +709,7 @@ openstack.volume.v3 =
     volume_backup_list = openstackclient.volume.v2.volume_backup:ListVolumeBackup
     volume_backup_restore = openstackclient.volume.v2.volume_backup:RestoreVolumeBackup
     volume_backup_set = openstackclient.volume.v2.volume_backup:SetVolumeBackup
+    volume_backup_unset = openstackclient.volume.v2.volume_backup:UnsetVolumeBackup
     volume_backup_show = openstackclient.volume.v2.volume_backup:ShowVolumeBackup
 
     volume_backup_record_export = openstackclient.volume.v2.backup_record:ExportBackupRecord