From ba69870d86b5840dec06c6c30c8ddf50398bdb44 Mon Sep 17 00:00:00 2001
From: Stephen Finucane <sfinucan@redhat.com>
Date: Wed, 15 Dec 2021 17:32:10 +0000
Subject: [PATCH] compute: Fix weird option definition for 'server ssh'

argparse allows you to specify multiple options for a given argument
when declaring the argument. For some reason, we weren't doing this for
the 'server ssh' command. There's no apparent reason for doing things
this way and it's been that way since the beginning (2013) so let's not
do that.

We also add unit tests since they were missing and should exist.

Change-Id: I67a9e6516d7057266210cd4083e9ddeb1cfaa5de
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>
---
 openstackclient/compute/v2/server.py          | 33 +-------
 .../tests/unit/compute/v2/test_server.py      | 77 +++++++++++++++++++
 2 files changed, 81 insertions(+), 29 deletions(-)

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index a18ce81012..5c603d04da 100644
--- a/openstackclient/compute/v2/server.py
+++ b/openstackclient/compute/v2/server.py
@@ -4462,51 +4462,26 @@ class SshServer(command.Command):
             help=_('Server (name or ID)'),
         )
         parser.add_argument(
-            '--login',
+            '--login', '-l',
             metavar='<login-name>',
             help=_('Login name (ssh -l option)'),
         )
         parser.add_argument(
-            '-l',
-            dest='login',
-            metavar='<login-name>',
-            help=argparse.SUPPRESS,
-        )
-        parser.add_argument(
-            '--port',
+            '--port', '-p',
             metavar='<port>',
             type=int,
             help=_('Destination port (ssh -p option)'),
         )
         parser.add_argument(
-            '-p',
-            metavar='<port>',
-            dest='port',
-            type=int,
-            help=argparse.SUPPRESS,
-        )
-        parser.add_argument(
-            '--identity',
+            '--identity', '-i',
             metavar='<keyfile>',
             help=_('Private key file (ssh -i option)'),
         )
         parser.add_argument(
-            '-i',
-            metavar='<filename>',
-            dest='identity',
-            help=argparse.SUPPRESS,
-        )
-        parser.add_argument(
-            '--option',
+            '--option', '-o',
             metavar='<config-options>',
             help=_('Options in ssh_config(5) format (ssh -o option)'),
         )
-        parser.add_argument(
-            '-o',
-            metavar='<option>',
-            dest='option',
-            help=argparse.SUPPRESS,
-        )
         ip_group = parser.add_mutually_exclusive_group()
         ip_group.add_argument(
             '-4',
diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py
index 15be07fc6c..acff61e6ec 100644
--- a/openstackclient/tests/unit/compute/v2/test_server.py
+++ b/openstackclient/tests/unit/compute/v2/test_server.py
@@ -8265,6 +8265,83 @@ class TestServerShow(TestServer):
             exceptions.CommandError, self.cmd.take_action, parsed_args)
 
 
+@mock.patch('openstackclient.compute.v2.server.os.system')
+class TestServerSsh(TestServer):
+
+    def setUp(self):
+        super().setUp()
+
+        self.cmd = server.SshServer(self.app, None)
+
+        self.app.client_manager.auth_ref = mock.Mock()
+        self.app.client_manager.auth_ref.username = 'cloud'
+
+        self.attrs = {
+            'addresses': {
+                'public': [
+                    {
+                        'addr': '192.168.1.30',
+                        'OS-EXT-IPS-MAC:mac_addr': '00:0c:29:0d:11:74',
+                        'OS-EXT-IPS:type': 'fixed',
+                        'version': 4,
+                    },
+                ],
+            },
+        }
+        self.server = compute_fakes.FakeServer.create_one_server(
+            attrs=self.attrs, methods=self.methods,
+        )
+        self.servers_mock.get.return_value = self.server
+
+    def test_server_ssh_no_opts(self, mock_exec):
+        arglist = [
+            self.server.name,
+        ]
+        verifylist = [
+            ('server', self.server.name),
+            ('login', None),
+            ('port', None),
+            ('identity', None),
+            ('option', None),
+            ('ipv4', False),
+            ('ipv6', False),
+            ('address_type', 'public'),
+            ('verbose', False),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+
+        self.assertIsNone(result)
+        mock_exec.assert_called_once_with('ssh cloud@192.168.1.30')
+
+    def test_server_ssh_opts(self, mock_exec):
+        arglist = [
+            self.server.name,
+            '-l', 'username',
+            '-p', '2222',
+        ]
+        verifylist = [
+            ('server', self.server.name),
+            ('login', 'username'),
+            ('port', 2222),
+            ('identity', None),
+            ('option', None),
+            ('ipv4', False),
+            ('ipv6', False),
+            ('address_type', 'public'),
+            ('verbose', False),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        result = self.cmd.take_action(parsed_args)
+
+        self.assertIsNone(result)
+        mock_exec.assert_called_once_with(
+            'ssh -p 2222 username@192.168.1.30'
+        )
+
+
 class TestServerStart(TestServer):
 
     def setUp(self):