Fix NoneType error for volume snapshot create command

In volume snapshot command, <volume> is the same
as <snapshot-name> when --volume is not specified,
but <volume> cannot be None, so when <snapshot-name>
is not specified (<snapshot-name> is None), a NoneType
error appears.
So make <snapshot-name> no longer optional, it should
be always present.

Change-Id: I3d9f10753a8ef601e70816421c160598e2cc811f
Closes-bug: #1659894

Author: Huanxuan Ao

doc/source/backwards-incompatible.rst

@@ -27,6 +27,21 @@ Backwards Incompatible Changes
 ..   * Remove in: <5.0>
 ..   * Commit: <tbd>
 
+Release 3.10
+------------
+
+1. The positional argument ``<snapshot-name>`` of the ``volume snapshot create``
+   command is no longer optional.
+
+   Previously when the ``--volume`` option was
+   present ``<snapshot-name>`` defaulted to the ``--volume`` value.  When the
+   ``--volume`` option is not present now it defaults to the value of
+   ``<snapshot-name>``.
+
+  * As of: 3.10
+  * Bug: 1659894
+  * Commit: https://review.openstack.org/440497
+
 Release 3.0
 -----------
 

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

@@ -49,7 +49,7 @@ Create new volume snapshot
 .. _volume_snapshot_create-snapshot-name:
 .. describe:: <snapshot-name>
 
-    Name of the new snapshot (default to None)
+    Name of the new snapshot
 
 volume snapshot delete
 ----------------------

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

@@ -18,6 +18,7 @@
 from osc_lib import exceptions
 from osc_lib import utils
 
+from openstackclient.tests.unit import utils as tests_utils
 from openstackclient.tests.unit.volume.v1 import fakes as volume_fakes
 from openstackclient.volume.v1 import volume_snapshot
 
@@ -98,26 +99,17 @@ def test_snapshot_create(self):
     def test_snapshot_create_without_name(self):
         arglist = [
             "--volume", self.new_snapshot.volume_id,
-            "--description", self.new_snapshot.display_description,
-            "--force"
         ]
         verifylist = [
             ("volume", self.new_snapshot.volume_id),
-            ("description", self.new_snapshot.display_description),
-            ("force", True)
         ]
-        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-
-        columns, data = self.cmd.take_action(parsed_args)
-
-        self.snapshots_mock.create.assert_called_with(
-            self.new_snapshot.volume_id,
-            True,
-            None,
-            self.new_snapshot.display_description,
+        self.assertRaises(
+            tests_utils.ParserException,
+            self.check_parser,
+            self.cmd,
+            arglist,
+            verifylist,
         )
-        self.assertEqual(self.columns, columns)
-        self.assertEqual(self.data, data)
 
     def test_snapshot_create_without_volume(self):
         arglist = [

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

@@ -20,6 +20,7 @@
 from osc_lib import utils
 
 from openstackclient.tests.unit.identity.v3 import fakes as project_fakes
+from openstackclient.tests.unit import utils as tests_utils
 from openstackclient.tests.unit.volume.v2 import fakes as volume_fakes
 from openstackclient.volume.v2 import volume_snapshot
 
@@ -107,27 +108,17 @@ def test_snapshot_create(self):
     def test_snapshot_create_without_name(self):
         arglist = [
             "--volume", self.new_snapshot.volume_id,
-            "--description", self.new_snapshot.description,
-            "--force"
         ]
         verifylist = [
             ("volume", self.new_snapshot.volume_id),
-            ("description", self.new_snapshot.description),
-            ("force", True)
         ]
-        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-
-        columns, data = self.cmd.take_action(parsed_args)
-
-        self.snapshots_mock.create.assert_called_with(
-            self.new_snapshot.volume_id,
-            force=True,
-            name=None,
-            description=self.new_snapshot.description,
-            metadata=None,
+        self.assertRaises(
+            tests_utils.ParserException,
+            self.check_parser,
+            self.cmd,
+            arglist,
+            verifylist,
         )
-        self.assertEqual(self.columns, columns)
-        self.assertEqual(self.data, data)
 
     def test_snapshot_create_without_volume(self):
         arglist = [
@@ -156,17 +147,19 @@ def test_snapshot_create_without_volume(self):
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.data, data)
 
-    def test_snapshot_create_without_remote_source(self):
+    def test_snapshot_create_with_remote_source(self):
         arglist = [
             '--remote-source', 'source-name=test_source_name',
             '--remote-source', 'source-id=test_source_id',
             '--volume', self.new_snapshot.volume_id,
+            self.new_snapshot.name,
         ]
         ref_dict = {'source-name': 'test_source_name',
                     'source-id': 'test_source_id'}
         verifylist = [
             ('remote_source', ref_dict),
             ('volume', self.new_snapshot.volume_id),
+            ("snapshot_name", self.new_snapshot.name),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
@@ -175,7 +168,7 @@ def test_snapshot_create_without_remote_source(self):
         self.snapshots_mock.manage.assert_called_with(
             volume_id=self.new_snapshot.volume_id,
             ref=ref_dict,
-            name=None,
+            name=self.new_snapshot.name,
             description=None,
             metadata=None,
         )

openstackclient/volume/v1/volume_snapshot.py

@@ -38,8 +38,7 @@ def get_parser(self, prog_name):
         parser.add_argument(
             'snapshot_name',
             metavar='<snapshot-name>',
-            nargs="?",
-            help=_('Name of the snapshot (default to None)'),
+            help=_('Name of the new snapshot'),
         )
         parser.add_argument(
             '--volume',

openstackclient/volume/v2/volume_snapshot.py

@@ -38,8 +38,7 @@ def get_parser(self, prog_name):
         parser.add_argument(
             "snapshot_name",
             metavar="<snapshot-name>",
-            nargs="?",
-            help=_("Name of the new snapshot (default to None)")
+            help=_("Name of the new snapshot"),
         )
         parser.add_argument(
             "--volume",

releasenotes/notes/bug-1659894-4518b10615498ba9.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Now the positional parameter ``<snapshot-name>`` of ``volume snapshot create``
+    command is no longer optional, it should be always present.
+    [Bug `1659894 <https://bugs.launchpad.net/bugs/1659894>`_]