Log warning when API version is not specified for the OSC plugin

At the Pike PTG, an issue was brought up regarding the use of an old API
version in the ironic OpenStack CLI plugin. [0]

It was suggested that we begin printing a warning whenever the default
OSC API version is used and later default to using the latest API
version when --os-baremetal-api-version is unspecified.

This patch adds the warning discussed above.

[0] https://etherpad.openstack.org/p/ironic-pike-ptg-operations L30

Co-Authored-By: Dmitry Tantsur <dtantsur@redhat.com>
Partial-Bug: #1671145
Change-Id: I0cf4e737595405b7f9beff76a72ffd26e07e6277

5 files changed, 82 insertions, 3 deletions

diff --git a/ironicclient/osc/plugin.py b/ironicclient/osc/plugin.py
index f0b84e0..a1dc94d 100644
--- a/ironicclient/osc/plugin.py
+++ b/ironicclient/osc/plugin.py
@@ -32,10 +32,23 @@ API_VERSIONS = {
     for i in range(1, LAST_KNOWN_API_VERSION + 1)
 }
 API_VERSIONS['1'] = API_VERSIONS[http.DEFAULT_VER]
+OS_BAREMETAL_API_VERSION_SPECIFIED = False
+MISSING_VERSION_WARNING = (
+    "You are using the default API version of the OpenStack CLI baremetal "
+    "(ironic) plugin. This is currently API version %s. In the future, "
+    "the default will be the latest API version understood by both API "
+    "and CLI. You can preserve the current behavior by passing the "
+    "--os-baremetal-api-version argument with the desired version or using "
+    "the OS_BAREMETAL_API_VERSION environment variable."
+)
 
 
 def make_client(instance):
     """Returns a baremetal service client."""
+    if (not OS_BAREMETAL_API_VERSION_SPECIFIED and not
+            utils.env('OS_BAREMETAL_API_VERSION')):
+        LOG.warning(MISSING_VERSION_WARNING, http.DEFAULT_VER)
+
     baremetal_client_class = utils.get_client_class(
         API_NAME,
         instance._api_version[API_NAME],
@@ -80,6 +93,8 @@ def build_option_parser(parser):
 class ReplaceLatestVersion(argparse.Action):
     """Replaces `latest` keyword by last known version."""
     def __call__(self, parser, namespace, values, option_string=None):
+        global OS_BAREMETAL_API_VERSION_SPECIFIED
+        OS_BAREMETAL_API_VERSION_SPECIFIED = True
         latest = values == 'latest'
         if latest:
             values = '1.%d' % LAST_KNOWN_API_VERSION
diff --git a/ironicclient/tests/functional/base.py b/ironicclient/tests/functional/base.py
index f230755..b954d1a 100644
--- a/ironicclient/tests/functional/base.py
+++ b/ironicclient/tests/functional/base.py
@@ -150,7 +150,7 @@ class FunctionalTestBase(base.ClientTestBase):
             return self.client.cmd_with_auth('ironic',
                                              action, flags, params)
 
-    def _ironic_osc(self, action, flags='', params=''):
+    def _ironic_osc(self, action, flags='', params='', merge_stderr=False):
         """Execute baremetal commands via OpenStack Client."""
         config = self._get_config()
         id_api_version = config.get('functional', 'os_identity_api_version')
@@ -164,7 +164,7 @@ class FunctionalTestBase(base.ClientTestBase):
                     'value': getattr(self, domain_attr)
                 }
         return self.client.cmd_with_auth(
-            'openstack', action, flags, params)
+            'openstack', action, flags, params, merge_stderr=merge_stderr)
 
     def ironic(self, action, flags='', params='', parse=True):
         """Return parsed list of dicts with basic item info.
diff --git a/ironicclient/tests/functional/osc/v1/test_baremetal_node_basic.py b/ironicclient/tests/functional/osc/v1/test_baremetal_node_basic.py
index 9f1f0b6..c968f7e 100644
--- a/ironicclient/tests/functional/osc/v1/test_baremetal_node_basic.py
+++ b/ironicclient/tests/functional/osc/v1/test_baremetal_node_basic.py
@@ -26,6 +26,26 @@ class BaremetalNodeTests(base.TestCase):
         super(BaremetalNodeTests, self).setUp()
         self.node = self.node_create()
 
+    def test_warning_version_not_specified(self):
+        """Test API version warning is printed when API version unspecified.
+
+        A warning will appear for any invocation of the baremetal OSC plugin
+        without --os-baremetal-api-version specified. It's tested with a simple
+        node list command here.
+        """
+        output = self.openstack('baremetal node list', merge_stderr=True)
+        self.assertIn('the default will be the latest API version', output)
+
+    def test_no_warning_version_specified(self):
+        """Test API version warning is not printed when API version specified.
+
+        This warning should not appear when a user specifies the ironic API
+        version to use.
+        """
+        output = self.openstack('baremetal --os-baremetal-api-version=1.9 node'
+                                ' list', merge_stderr=True)
+        self.assertNotIn('the default will be the latest API version', output)
+
     def test_create_name_uuid(self):
         """Check baremetal node create command with name and UUID.
 
diff --git a/ironicclient/tests/unit/osc/test_plugin.py b/ironicclient/tests/unit/osc/test_plugin.py
index a86f5fe..a687d47 100644
--- a/ironicclient/tests/unit/osc/test_plugin.py
+++ b/ironicclient/tests/unit/osc/test_plugin.py
@@ -23,8 +23,10 @@ from ironicclient.v1 import client
 
 class MakeClientTest(testtools.TestCase):
 
+    @mock.patch.object(plugin, 'OS_BAREMETAL_API_VERSION_SPECIFIED', new=True)
+    @mock.patch.object(plugin.LOG, 'warning', autospec=True)
     @mock.patch.object(client, 'Client', autospec=True)
-    def test_make_client(self, mock_client):
+    def test_make_client(self, mock_client, mock_warning):
         instance = fakes.FakeClientManager()
         instance.get_endpoint_for_service_type = mock.Mock(
             return_value='endpoint')
@@ -33,10 +35,43 @@ class MakeClientTest(testtools.TestCase):
                                             session=instance.session,
                                             region_name=instance._region_name,
                                             endpoint='endpoint')
+        self.assertFalse(mock_warning.called)
         instance.get_endpoint_for_service_type.assert_called_once_with(
             'baremetal', region_name=instance._region_name,
             interface=instance.interface)
 
+    @mock.patch.object(plugin, 'OS_BAREMETAL_API_VERSION_SPECIFIED', new=False)
+    @mock.patch.object(plugin.LOG, 'warning', autospec=True)
+    @mock.patch.object(client, 'Client', autospec=True)
+    def test_make_client_log_warning_no_version_specified(self, mock_client,
+                                                          mock_warning):
+        instance = fakes.FakeClientManager()
+        instance.get_endpoint_for_service_type = mock.Mock(
+            return_value='endpoint')
+        instance._api_version = {'baremetal': http.DEFAULT_VER}
+        plugin.make_client(instance)
+        mock_client.assert_called_once_with(
+            os_ironic_api_version=http.DEFAULT_VER,
+            session=instance.session,
+            region_name=instance._region_name,
+            endpoint='endpoint')
+        self.assertTrue(mock_warning.called)
+        instance.get_endpoint_for_service_type.assert_called_once_with(
+            'baremetal', region_name=instance._region_name,
+            interface=instance.interface)
+
+    @mock.patch.object(plugin.utils, 'env', lambda x: '1.29')
+    @mock.patch.object(plugin, 'OS_BAREMETAL_API_VERSION_SPECIFIED', new=False)
+    @mock.patch.object(plugin.LOG, 'warning', autospec=True)
+    @mock.patch.object(client, 'Client', autospec=True)
+    def test_make_client_version_from_env_no_warning(self, mock_client,
+                                                     mock_warning):
+        instance = fakes.FakeClientManager()
+        instance.get_endpoint_for_service_type = mock.Mock(
+            return_value='endpoint')
+        plugin.make_client(instance)
+        self.assertFalse(mock_warning.called)
+
 
 class BuildOptionParserTest(testtools.TestCase):
 
@@ -63,6 +98,7 @@ class ReplaceLatestVersionTest(testtools.TestCase):
                                 namespace)
         self.assertEqual('1.%d' % plugin.LAST_KNOWN_API_VERSION,
                          namespace.os_baremetal_api_version)
+        self.assertTrue(plugin.OS_BAREMETAL_API_VERSION_SPECIFIED)
 
     def test___call___specific_version(self):
         parser = argparse.ArgumentParser()
@@ -71,3 +107,4 @@ class ReplaceLatestVersionTest(testtools.TestCase):
         parser.parse_known_args(['--os-baremetal-api-version', '1.4'],
                                 namespace)
         self.assertEqual('1.4', namespace.os_baremetal_api_version)
+        self.assertTrue(plugin.OS_BAREMETAL_API_VERSION_SPECIFIED)
diff --git a/releasenotes/notes/implicit-version-warning-d34b99727b50d519.yaml b/releasenotes/notes/implicit-version-warning-d34b99727b50d519.yaml
new file mode 100644
index 0000000..3431415
--- /dev/null
+++ b/releasenotes/notes/implicit-version-warning-d34b99727b50d519.yaml
@@ -0,0 +1,7 @@
+---
+deprecations:
+  - |
+    Currently, the default API version for the OSC plugin is fixed to be 1.9.
+    In the Queens release, it will be changed to the latest version understood
+    by both the client and the server. In this release a warning is logged, if
+    no explicit version is provided.