From 12f1e56ebf2ea3999c57246410501c09fced2c4c Mon Sep 17 00:00:00 2001
From: Stephen Finucane <sfinucan@redhat.com>
Date: Wed, 22 Jul 2020 10:39:47 +0100
Subject: [PATCH] Add 'openstack server create --use-config-drive'

Despite what the help text for this options says, the nova API only
accepts boolean values for this value and has done so since at least the
introduction of the 2.1 microversioned API. While it would be nice to
convert '--config-drive' to a boolean flag, we'd need to be able to
retain temporary support for people passing arguments. 'nargs=?' [1]
looks promising but it has an annoying tendency to swallow a positional
argument following it [2]. Since that is not an option, we have to live
with a new config option, '--use-config-drive' and a '--no-config-drive'
counterpart.

[1] https://docs.python.org/3/library/argparse.html#nargs
[2] https://bugs.python.org/issue9338

Change-Id: If9cce0ad4094cc9cef1c9136b80c3b0f35a82c7a
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>
Story: #2005468
Task: #30547
---
 openstackclient/compute/v2/server.py          | 45 ++++++++++++++-----
 .../tests/unit/compute/v2/test_server.py      |  5 ++-
 ...ver-use-config-drive-9fc68552365cfefa.yaml |  8 ++++
 3 files changed, 44 insertions(+), 14 deletions(-)
 create mode 100644 releasenotes/notes/story-2005468-server-use-config-drive-9fc68552365cfefa.yaml

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index 93e9f966ae..54cfe770f2 100644
--- a/openstackclient/compute/v2/server.py
+++ b/openstackclient/compute/v2/server.py
@@ -693,12 +693,30 @@ class CreateServer(command.ShowOne):
             default={},
             help=_('Hints for the scheduler (optional extension)'),
         )
-        parser.add_argument(
+        config_drive_group = parser.add_mutually_exclusive_group()
+        config_drive_group.add_argument(
+            '--use-config-drive',
+            action='store_true',
+            dest='config_drive',
+            help=_("Enable config drive."),
+        )
+        config_drive_group.add_argument(
+            '--no-config-drive',
+            action='store_false',
+            dest='config_drive',
+            help=_("Disable config drive."),
+        )
+        # TODO(stephenfin): Drop support in the next major version bump after
+        # Victoria
+        config_drive_group.add_argument(
             '--config-drive',
             metavar='<config-drive-volume>|True',
             default=False,
-            help=_('Use specified volume as the config drive, '
-                   'or \'True\' to use an ephemeral drive'),
+            help=_(
+                "**Deprecated** Use specified volume as the config drive, "
+                "or 'True' to use an ephemeral drive. Replaced by "
+                "'--use-config-drive'."
+            ),
         )
         parser.add_argument(
             '--min',
@@ -991,16 +1009,19 @@ class CreateServer(command.ShowOne):
             else:
                 hints[key] = values
 
-        # What does a non-boolean value for config-drive do?
-        # --config-drive argument is either a volume id or
-        # 'True' (or '1') to use an ephemeral volume
-        if str(parsed_args.config_drive).lower() in ("true", "1"):
-            config_drive = True
-        elif str(parsed_args.config_drive).lower() in ("false", "0",
-                                                       "", "none"):
-            config_drive = None
+        if isinstance(parsed_args.config_drive, bool):
+            # NOTE(stephenfin): The API doesn't accept False as a value :'(
+            config_drive = parsed_args.config_drive or None
         else:
-            config_drive = parsed_args.config_drive
+            # TODO(stephenfin): Remove when we drop support for
+            # '--config-drive'
+            if str(parsed_args.config_drive).lower() in ("true", "1"):
+                config_drive = True
+            elif str(parsed_args.config_drive).lower() in ("false", "0",
+                                                           "", "none"):
+                config_drive = None
+            else:
+                config_drive = parsed_args.config_drive
 
         boot_kwargs = dict(
             meta=parsed_args.property,
diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py
index 7e4c71c50c..dc699c0f43 100644
--- a/openstackclient/tests/unit/compute/v2/test_server.py
+++ b/openstackclient/tests/unit/compute/v2/test_server.py
@@ -857,6 +857,7 @@ class TestServerCreate(TestServer):
             '--key-name', 'keyname',
             '--property', 'Beta=b',
             '--security-group', 'securitygroup',
+            '--use-config-drive',
             '--hint', 'a=b',
             '--hint', 'a=c',
             self.new_server.name,
@@ -868,7 +869,7 @@ class TestServerCreate(TestServer):
             ('property', {'Beta': 'b'}),
             ('security_group', ['securitygroup']),
             ('hint', {'a': ['b', 'c']}),
-            ('config_drive', False),
+            ('config_drive', True),
             ('server_name', self.new_server.name),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@@ -900,7 +901,7 @@ class TestServerCreate(TestServer):
             block_device_mapping_v2=[],
             nics=[],
             scheduler_hints={'a': ['b', 'c']},
-            config_drive=None,
+            config_drive=True,
         )
         # ServerManager.create(name, image, flavor, **kwargs)
         self.servers_mock.create.assert_called_with(
diff --git a/releasenotes/notes/story-2005468-server-use-config-drive-9fc68552365cfefa.yaml b/releasenotes/notes/story-2005468-server-use-config-drive-9fc68552365cfefa.yaml
new file mode 100644
index 0000000000..786ede4f78
--- /dev/null
+++ b/releasenotes/notes/story-2005468-server-use-config-drive-9fc68552365cfefa.yaml
@@ -0,0 +1,8 @@
+---
+deprecations:
+  - |
+    The ``--config-drive`` option on the ``openstack server create`` command
+    has been deprecated in favour of the ``--use-config-drive`` and
+    ``--no-config-drive`` arguments. The ``--config-drive`` option expected
+    either a string or bool-like argument, but the nova API has only supported
+    boolean values since API v2.1 was introduced.