volume: Add missing 'volume backup *' options

Add a couple of missing options to each command:

  volume backup create
    --no-incremental
    --property
    --availability-zone

  volume backup set
    --property

Most of these are version dependent so we add the relevant version
checks as part of this work. While we're here, we also make the
formatting a little easier on the eye in places.

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

Author: stephenfin

doc/source/cli/data/cinder.csv

@@ -15,7 +15,7 @@ backup-list,volume backup list,Lists all backups.
 backup-reset-state,volume backup set --state,Explicitly updates the backup state.
 backup-restore,volume backup restore,Restores a backup.
 backup-show,volume backup show,Show backup details.
-backup-update,,Updates a backup. (Supported by API versions 3.9 - 3.latest)
+backup-update,volume backup set,Updates a backup. (Supported by API versions 3.9 - 3.latest)
 cgsnapshot-create,consistency group snapshot create,Creates a cgsnapshot.
 cgsnapshot-delete,consistency group snapshot delete,Removes one or more cgsnapshots.
 cgsnapshot-list,consistency group snapshot list,Lists all cgsnapshots.

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

@@ -15,6 +15,7 @@
 from unittest import mock
 from unittest.mock import call
 
+from cinderclient import api_versions
 from osc_lib import exceptions
 from osc_lib import utils
 
@@ -114,6 +115,104 @@ def test_backup_create(self):
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.data, data)
 
+    def test_backup_create_with_properties(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.43')
+
+        arglist = [
+            "--property", "foo=bar",
+            "--property", "wow=much-cool",
+            self.new_backup.volume_id,
+        ]
+        verifylist = [
+            ("properties", {"foo": "bar", "wow": "much-cool"}),
+            ("volume", self.new_backup.volume_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        columns, data = self.cmd.take_action(parsed_args)
+
+        self.backups_mock.create.assert_called_with(
+            self.new_backup.volume_id,
+            container=None,
+            name=None,
+            description=None,
+            force=False,
+            incremental=False,
+            metadata={"foo": "bar", "wow": "much-cool"},
+        )
+        self.assertEqual(self.columns, columns)
+        self.assertEqual(self.data, data)
+
+    def test_backup_create_with_properties_pre_v343(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.42')
+
+        arglist = [
+            "--property", "foo=bar",
+            "--property", "wow=much-cool",
+            self.new_backup.volume_id,
+        ]
+        verifylist = [
+            ("properties", {"foo": "bar", "wow": "much-cool"}),
+            ("volume", self.new_backup.volume_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_create_with_availability_zone(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.51')
+
+        arglist = [
+            "--availability-zone", "my-az",
+            self.new_backup.volume_id,
+        ]
+        verifylist = [
+            ("availability_zone", "my-az"),
+            ("volume", self.new_backup.volume_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        columns, data = self.cmd.take_action(parsed_args)
+
+        self.backups_mock.create.assert_called_with(
+            self.new_backup.volume_id,
+            container=None,
+            name=None,
+            description=None,
+            force=False,
+            incremental=False,
+            availability_zone="my-az",
+        )
+        self.assertEqual(self.columns, columns)
+        self.assertEqual(self.data, data)
+
+    def test_backup_create_with_availability_zone_pre_v351(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.50')
+
+        arglist = [
+            "--availability-zone", "my-az",
+            self.new_backup.volume_id,
+        ]
+        verifylist = [
+            ("availability_zone", "my-az"),
+            ("volume", self.new_backup.volume_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.51 or greater", str(exc))
+
     def test_backup_create_without_name(self):
         arglist = [
             "--description", self.new_backup.description,
@@ -136,7 +235,6 @@ def test_backup_create_without_name(self):
             description=self.new_backup.description,
             force=False,
             incremental=False,
-            snapshot_id=None,
         )
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.data, data)
@@ -240,18 +338,18 @@ class TestBackupList(TestBackup):
     backups = volume_fakes.FakeBackup.create_backups(
         attrs={'volume_id': volume.name}, count=3)
 
-    columns = [
+    columns = (
         'ID',
         'Name',
         'Description',
         'Status',
         'Size',
-    ]
-    columns_long = columns + [
+    )
+    columns_long = columns + (
         'Availability Zone',
         'Volume',
         'Container',
-    ]
+    )
 
     data = []
     for b in backups:
@@ -403,6 +501,9 @@ def setUp(self):
         self.cmd = volume_backup.SetVolumeBackup(self.app, None)
 
     def test_backup_set_name(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.9')
+
         arglist = [
             '--name', 'new_name',
             self.backup.id,
@@ -420,7 +521,30 @@ def test_backup_set_name(self):
             self.backup.id, **{'name': 'new_name'})
         self.assertIsNone(result)
 
+    def test_backup_set_name_pre_v39(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.8')
+
+        arglist = [
+            '--name', 'new_name',
+            self.backup.id,
+        ]
+        verifylist = [
+            ('name', 'new_name'),
+            ('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.9 or greater", str(exc))
+
     def test_backup_set_description(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.9')
+
         arglist = [
             '--description', 'new_description',
             self.backup.id,
@@ -444,6 +568,27 @@ def test_backup_set_description(self):
         )
         self.assertIsNone(result)
 
+    def test_backup_set_description_pre_v39(self):
+        self.app.client_manager.volume.api_version = \
+            api_versions.APIVersion('3.8')
+
+        arglist = [
+            '--description', 'new_description',
+            self.backup.id,
+        ]
+        verifylist = [
+            ('name', None),
+            ('description', 'new_description'),
+            ('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.9 or greater", str(exc))
+
     def test_backup_set_state(self):
         arglist = [
             '--state', 'error',