diff --git a/openstackclient/tests/unit/volume/v2/fakes.py b/openstackclient/tests/unit/volume/v2/fakes.py index 86778698d..b5f66d4b1 100644 --- a/openstackclient/tests/unit/volume/v2/fakes.py +++ b/openstackclient/tests/unit/volume/v2/fakes.py @@ -17,6 +17,7 @@ import random 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() diff --git a/openstackclient/tests/unit/volume/v2/test_volume_backup.py b/openstackclient/tests/unit/volume/v2/test_volume_backup.py index 7b5a965e0..5d57bf609 100644 --- a/openstackclient/tests/unit/volume/v2/test_volume_backup.py +++ b/openstackclient/tests/unit/volume/v2/test_volume_backup.py @@ -490,7 +490,9 @@ class TestBackupRestore(TestBackup): 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 @@ class TestBackupSet(TestBackup): 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): diff --git a/openstackclient/volume/v2/volume_backup.py b/openstackclient/volume/v2/volume_backup.py index a171164e3..96b22a681 100644 --- a/openstackclient/volume/v2/volume_backup.py +++ b/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 @@ class SetVolumeBackup(command.Command): '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='', action=parseractions.KeyValueAction, dest='properties', + default={}, help=_( 'Set a property on this backup ' '(repeat option to set multiple values) ' @@ -454,6 +465,14 @@ class SetVolumeBackup(command.Command): 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,13 +481,20 @@ class SetVolumeBackup(command.Command): ) 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: @@ -476,6 +502,63 @@ class SetVolumeBackup(command.Command): 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='', + help=_('Backup to modify (name or ID)') + ) + parser.add_argument( + '--property', + metavar='', + 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") diff --git a/releasenotes/notes/add-missing-volume-backup-opts-b9246aded87427ce.yaml b/releasenotes/notes/add-missing-volume-backup-opts-b9246aded87427ce.yaml index 883cb0d56..f3b8bbc38 100644 --- a/releasenotes/notes/add-missing-volume-backup-opts-b9246aded87427ce.yaml +++ b/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`` diff --git a/setup.cfg b/setup.cfg index b86f4fcb9..b39061622 100644 --- a/setup.cfg +++ b/setup.cfg @@ -715,6 +715,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