Merge "Set the default API version of OSC CLI to "latest""

6 files changed, 148 insertions, 139 deletions

diff --git a/ironicclient/osc/plugin.py b/ironicclient/osc/plugin.py
index c4c2fd4..5723672 100644
--- a/ironicclient/osc/plugin.py
+++ b/ironicclient/osc/plugin.py
@@ -19,41 +19,28 @@
 import argparse
 import logging
 
-from ironicclient.common import http
 from osc_lib import utils
 
 LOG = logging.getLogger(__name__)
 
+CLIENT_CLASS = 'ironicclient.v1.client.Client'
 API_VERSION_OPTION = 'os_baremetal_api_version'
 API_NAME = 'baremetal'
 LAST_KNOWN_API_VERSION = 34
 LATEST_VERSION = "1.{}".format(LAST_KNOWN_API_VERSION)
 API_VERSIONS = {
-    '1.%d' % i: 'ironicclient.v1.client.Client'
+    '1.%d' % i: CLIENT_CLASS
     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."
-)
+API_VERSIONS['1'] = CLIENT_CLASS
 # NOTE(dtantsur): flag to indicate that the requested version was "latest".
 # Due to how OSC works we cannot just add "latest" to the list of supported
 # versions - it breaks the major version detection.
-OS_BAREMETAL_API_LATEST = False
+OS_BAREMETAL_API_LATEST = True
 
 
 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)
-
     requested_api_version = instance._api_version[API_NAME]
 
     baremetal_client_class = utils.get_client_class(
@@ -65,11 +52,19 @@ def make_client(instance):
               requested_api_version if not OS_BAREMETAL_API_LATEST
               else "latest")
 
+    if requested_api_version == '1':
+        # NOTE(dtantsur): '1' means 'the latest v1 API version'. Since we don't
+        # have other major versions, it's identical to 'latest'.
+        requested_api_version = LATEST_VERSION
+        allow_api_version_downgrade = True
+    else:
+        allow_api_version_downgrade = OS_BAREMETAL_API_LATEST
+
     client = baremetal_client_class(
         os_ironic_api_version=requested_api_version,
         # NOTE(dtantsur): enable re-negotiation of the latest version, if CLI
         # latest is too high for the server we're talking to.
-        allow_api_version_downgrade=OS_BAREMETAL_API_LATEST,
+        allow_api_version_downgrade=allow_api_version_downgrade,
         session=instance.session,
         region_name=instance._region_name,
         # NOTE(vdrok): This will be set as endpoint_override, and the Client
@@ -87,17 +82,14 @@ def build_option_parser(parser):
     parser.add_argument(
         '--os-baremetal-api-version',
         metavar='<baremetal-api-version>',
-        default=_get_environment_version(http.DEFAULT_VER),
+        default=_get_environment_version("latest"),
         choices=sorted(
             API_VERSIONS,
             key=lambda k: [int(x) for x in k.split('.')]) + ['latest'],
         action=ReplaceLatestVersion,
-        help='Baremetal API version, default=' +
-             http.DEFAULT_VER +
-             ' (Env: OS_BAREMETAL_API_VERSION). '
-             'Use "latest" for the latest known API version. '
-             'The default value will change to "latest" in the Queens '
-             'release.',
+        help='Bare metal API version, default="latest" (the maximum version '
+             'supported by both the client and the server). '
+             '(Env: OS_BAREMETAL_API_VERSION)',
     )
     return parser
 
@@ -109,7 +101,8 @@ def _get_environment_version(default):
         env_value = default
     if env_value == 'latest':
         env_value = LATEST_VERSION
-        OS_BAREMETAL_API_LATEST = True
+    else:
+        OS_BAREMETAL_API_LATEST = False
     return env_value
 
 
@@ -120,12 +113,16 @@ class ReplaceLatestVersion(argparse.Action):
     breaks the major version detection (OSC tries to load configuration options
     from setuptools entrypoint openstack.baremetal.vlatest). This action
     replaces "latest" with the latest known version, and sets the global
-    OS_BAREMETAL_API_LATEST flag to True.
+    OS_BAREMETAL_API_LATEST flag appropriately.
     """
     def __call__(self, parser, namespace, values, option_string=None):
-        global OS_BAREMETAL_API_VERSION_SPECIFIED, OS_BAREMETAL_API_LATEST
-        OS_BAREMETAL_API_VERSION_SPECIFIED = True
+        global OS_BAREMETAL_API_LATEST
         if values == 'latest':
             values = LATEST_VERSION
+            # The default value of "True" may have been overriden due to
+            # non-empty OS_BAREMETAL_API_VERSION env variable. If a user
+            # explicitly requests "latest", we need to correct it.
             OS_BAREMETAL_API_LATEST = True
+        else:
+            OS_BAREMETAL_API_LATEST = False
         setattr(namespace, self.dest, values)
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 d0729ca..2d1be7c 100644
--- a/ironicclient/tests/functional/osc/v1/test_baremetal_node_basic.py
+++ b/ironicclient/tests/functional/osc/v1/test_baremetal_node_basic.py
@@ -27,26 +27,6 @@ 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.
 
@@ -64,10 +44,25 @@ class BaremetalNodeTests(base.TestCase):
         self.assertEqual(node_info['name'], name)
         self.assertEqual(node_info['driver'], 'fake')
         self.assertEqual(node_info['maintenance'], False)
+        self.assertEqual(node_info['provision_state'], 'enroll')
         node_list = self.node_list()
         self.assertIn(uuid, [x['UUID'] for x in node_list])
         self.assertIn(name, [x['Name'] for x in node_list])
 
+    def test_create_old_api_version(self):
+        """Check baremetal node create command with name and UUID.
+
+        Test steps:
+        1) Create baremetal node in setUp.
+        2) Create one more baremetal node explicitly with old API version
+        3) Check that node successfully created.
+        """
+        node_info = self.node_create(
+            params='--os-baremetal-api-version 1.5')
+        self.assertEqual(node_info['driver'], 'fake')
+        self.assertEqual(node_info['maintenance'], False)
+        self.assertEqual(node_info['provision_state'], 'available')
+
     @ddt.data('name', 'uuid')
     def test_delete(self, key):
         """Check baremetal node delete command with name/UUID argument.
diff --git a/ironicclient/tests/functional/osc/v1/test_baremetal_node_provision_states.py b/ironicclient/tests/functional/osc/v1/test_baremetal_node_provision_states.py
index acc24a2..350582c 100644
--- a/ironicclient/tests/functional/osc/v1/test_baremetal_node_provision_states.py
+++ b/ironicclient/tests/functional/osc/v1/test_baremetal_node_provision_states.py
@@ -22,20 +22,38 @@ class ProvisionStateTests(base.TestCase):
         super(ProvisionStateTests, self).setUp()
         self.node = self.node_create()
 
-    def test_deploy_rebuild_undeploy(self):
+    def test_deploy_rebuild_undeploy_manage(self):
         """Deploy, rebuild and undeploy node.
 
         Test steps:
         1) Create baremetal node in setUp.
-        2) Check initial "available" provision state.
-        3) Set baremetal node "deploy" provision state.
-        4) Check baremetal node provision_state field value is "active".
-        5) Set baremetal node "rebuild" provision state.
-        6) Check baremetal node provision_state field value is "active".
-        7) Set baremetal node "undeploy" provision state.
-        8) Check baremetal node provision_state field value is "available".
+        2) Check initial "enroll" provision state.
+        3) Set baremetal node "manage" provision state.
+        4) Check baremetal node provision_state field value is "manageable".
+        5) Set baremetal node "provide" provision state.
+        6) Check baremetal node provision_state field value is "available".
+        7) Set baremetal node "deploy" provision state.
+        8) Check baremetal node provision_state field value is "active".
+        9) Set baremetal node "rebuild" provision state.
+        10) Check baremetal node provision_state field value is "active".
+        11) Set baremetal node "undeploy" provision state.
+        12) Check baremetal node provision_state field value is "available".
+        13) Set baremetal node "manage" provision state.
+        14) Check baremetal node provision_state field value is "manageable".
+        15) Set baremetal node "provide" provision state.
+        16) Check baremetal node provision_state field value is "available".
         """
         show_prop = self.node_show(self.node['uuid'], ["provision_state"])
+        self.assertEqual("enroll", show_prop["provision_state"])
+
+        # manage
+        self.openstack('baremetal node manage {0}'.format(self.node['uuid']))
+        show_prop = self.node_show(self.node['uuid'], ["provision_state"])
+        self.assertEqual("manageable", show_prop["provision_state"])
+
+        # provide
+        self.openstack('baremetal node provide {0}'.format(self.node['uuid']))
+        show_prop = self.node_show(self.node['uuid'], ["provision_state"])
         self.assertEqual("available", show_prop["provision_state"])
 
         # deploy
@@ -55,21 +73,6 @@ class ProvisionStateTests(base.TestCase):
         show_prop = self.node_show(self.node['uuid'], ["provision_state"])
         self.assertEqual("available", show_prop["provision_state"])
 
-    def test_manage_provide(self):
-        """Manage and provide node back.
-
-        Steps:
-        1) Create baremetal node in setUp.
-        2) Check initial "available" provision state.
-        3) Set baremetal node "manage" provision state.
-        4) Check baremetal node provision_state field value is "manageable".
-        5) Set baremetal node "provide" provision state.
-        6) Check baremetal node provision_state field value is "available".
-        """
-
-        show_prop = self.node_show(self.node['uuid'], ["provision_state"])
-        self.assertEqual("available", show_prop["provision_state"])
-
         # manage
         self.openstack('baremetal node manage {0}'.format(self.node['uuid']))
         show_prop = self.node_show(self.node['uuid'], ["provision_state"])
diff --git a/ironicclient/tests/unit/osc/fakes.py b/ironicclient/tests/unit/osc/fakes.py
index cd9731b..76d77e6 100644
--- a/ironicclient/tests/unit/osc/fakes.py
+++ b/ironicclient/tests/unit/osc/fakes.py
@@ -19,6 +19,7 @@ import sys
 
 AUTH_TOKEN = "foobar"
 AUTH_URL = "http://0.0.0.0"
+API_VERSION = '1.6'
 
 
 class FakeApp(object):
@@ -38,7 +39,7 @@ class FakeClientManager(object):
         self.interface = 'public'
         self._region_name = 'RegionOne'
         self.session = 'fake session'
-        self._api_version = {'baremetal': '1.6'}
+        self._api_version = {'baremetal': API_VERSION}
 
 
 class FakeResource(object):
diff --git a/ironicclient/tests/unit/osc/test_plugin.py b/ironicclient/tests/unit/osc/test_plugin.py
index 266b3a2..f4cff99 100644
--- a/ironicclient/tests/unit/osc/test_plugin.py
+++ b/ironicclient/tests/unit/osc/test_plugin.py
@@ -15,7 +15,6 @@ import argparse
 import mock
 import testtools
 
-from ironicclient.common import http
 from ironicclient.osc import plugin
 from ironicclient.tests.unit.osc import fakes
 from ironicclient.v1 import client
@@ -23,87 +22,67 @@ from ironicclient.v1 import client
 
 class MakeClientTest(testtools.TestCase):
 
-    @mock.patch.object(plugin.utils, 'env', lambda x: None)
     @mock.patch.object(plugin, 'OS_BAREMETAL_API_LATEST', new=False)
-    @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, mock_warning):
+    def test_make_client_explicit_version(self, mock_client):
         instance = fakes.FakeClientManager()
         instance.get_endpoint_for_service_type = mock.Mock(
             return_value='endpoint')
         plugin.make_client(instance)
-        mock_client.assert_called_once_with(os_ironic_api_version='1.6',
-                                            allow_api_version_downgrade=False,
-                                            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.utils, 'env', lambda x: None)
-    @mock.patch.object(plugin, 'OS_BAREMETAL_API_LATEST', new=False)
-    @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,
+            os_ironic_api_version=fakes.API_VERSION,
             allow_api_version_downgrade=False,
             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: None)
     @mock.patch.object(plugin, 'OS_BAREMETAL_API_LATEST', new=True)
-    @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_latest(self, mock_client, mock_warning):
+    def test_make_client_latest(self, mock_client):
         instance = fakes.FakeClientManager()
         instance.get_endpoint_for_service_type = mock.Mock(
             return_value='endpoint')
+        instance._api_version = {'baremetal': plugin.LATEST_VERSION}
         plugin.make_client(instance)
-        mock_client.assert_called_once_with(os_ironic_api_version='1.6',
-                                            allow_api_version_downgrade=True,
-                                            session=instance.session,
-                                            region_name=instance._region_name,
-                                            endpoint='endpoint')
-        self.assertFalse(mock_warning.called)
+        mock_client.assert_called_once_with(
+            # NOTE(dtantsur): "latest" is changed to an actual version before
+            # make_client is called.
+            os_ironic_api_version=plugin.LATEST_VERSION,
+            allow_api_version_downgrade=True,
+            session=instance.session,
+            region_name=instance._region_name,
+            endpoint='endpoint')
         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_LATEST', new=False)
-    @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):
+    def test_make_client_v1(self, mock_client):
         instance = fakes.FakeClientManager()
         instance.get_endpoint_for_service_type = mock.Mock(
             return_value='endpoint')
+        instance._api_version = {'baremetal': '1'}
         plugin.make_client(instance)
-        self.assertFalse(mock_warning.called)
+        mock_client.assert_called_once_with(
+            os_ironic_api_version=plugin.LATEST_VERSION,
+            allow_api_version_downgrade=True,
+            session=instance.session,
+            region_name=instance._region_name,
+            endpoint='endpoint')
+        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_LATEST', new=True)
+@mock.patch.object(argparse.ArgumentParser, 'add_argument', autospec=True)
 class BuildOptionParserTest(testtools.TestCase):
 
     @mock.patch.object(plugin.utils, 'env', lambda x: None)
-    @mock.patch.object(argparse.ArgumentParser, 'add_argument', autospec=True)
     def test_build_option_parser(self, mock_add_argument):
         parser = argparse.ArgumentParser()
         mock_add_argument.reset_mock()
@@ -113,11 +92,11 @@ class BuildOptionParserTest(testtools.TestCase):
         mock_add_argument.assert_called_once_with(
             mock.ANY, '--os-baremetal-api-version',
             action=plugin.ReplaceLatestVersion, choices=version_list,
-            default=http.DEFAULT_VER, help=mock.ANY,
+            default=plugin.LATEST_VERSION, help=mock.ANY,
             metavar='<baremetal-api-version>')
+        self.assertTrue(plugin.OS_BAREMETAL_API_LATEST)
 
     @mock.patch.object(plugin.utils, 'env', lambda x: "latest")
-    @mock.patch.object(argparse.ArgumentParser, 'add_argument', autospec=True)
     def test_build_option_parser_env_latest(self, mock_add_argument):
         parser = argparse.ArgumentParser()
         mock_add_argument.reset_mock()
@@ -129,26 +108,27 @@ class BuildOptionParserTest(testtools.TestCase):
             action=plugin.ReplaceLatestVersion, choices=version_list,
             default=plugin.LATEST_VERSION, help=mock.ANY,
             metavar='<baremetal-api-version>')
+        self.assertTrue(plugin.OS_BAREMETAL_API_LATEST)
 
-    @mock.patch.object(plugin.utils, 'env', autospec=True)
-    def test__get_environment_version(self, mock_utils_env):
-        mock_utils_env.return_value = 'latest'
-        result = plugin._get_environment_version(None)
-        self.assertEqual(plugin.LATEST_VERSION, result)
-
-        mock_utils_env.return_value = None
-        result = plugin._get_environment_version('1.22')
-        self.assertEqual("1.22", result)
-
-        mock_utils_env.return_value = "1.23"
-        result = plugin._get_environment_version('1.9')
-        self.assertEqual("1.23", result)
+    @mock.patch.object(plugin.utils, 'env', lambda x: "1.1")
+    def test_build_option_parser_env(self, mock_add_argument):
+        parser = argparse.ArgumentParser()
+        mock_add_argument.reset_mock()
+        plugin.build_option_parser(parser)
+        version_list = ['1'] + ['1.%d' % i for i in range(
+            1, plugin.LAST_KNOWN_API_VERSION + 1)] + ['latest']
+        mock_add_argument.assert_called_once_with(
+            mock.ANY, '--os-baremetal-api-version',
+            action=plugin.ReplaceLatestVersion, choices=version_list,
+            default='1.1', help=mock.ANY,
+            metavar='<baremetal-api-version>')
+        self.assertFalse(plugin.OS_BAREMETAL_API_LATEST)
 
 
+@mock.patch.object(plugin.utils, 'env', lambda x: None)
 class ReplaceLatestVersionTest(testtools.TestCase):
 
     @mock.patch.object(plugin, 'OS_BAREMETAL_API_LATEST', new=False)
-    @mock.patch.object(plugin.utils, 'env', lambda x: None)
     def test___call___latest(self):
         parser = argparse.ArgumentParser()
         plugin.build_option_parser(parser)
@@ -157,11 +137,9 @@ class ReplaceLatestVersionTest(testtools.TestCase):
                                 namespace)
         self.assertEqual(plugin.LATEST_VERSION,
                          namespace.os_baremetal_api_version)
-        self.assertTrue(plugin.OS_BAREMETAL_API_VERSION_SPECIFIED)
         self.assertTrue(plugin.OS_BAREMETAL_API_LATEST)
 
-    @mock.patch.object(plugin, 'OS_BAREMETAL_API_LATEST', new=False)
-    @mock.patch.object(plugin.utils, 'env', lambda x: None)
+    @mock.patch.object(plugin, 'OS_BAREMETAL_API_LATEST', new=True)
     def test___call___specific_version(self):
         parser = argparse.ArgumentParser()
         plugin.build_option_parser(parser)
@@ -169,5 +147,14 @@ 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)
         self.assertFalse(plugin.OS_BAREMETAL_API_LATEST)
+
+    @mock.patch.object(plugin, 'OS_BAREMETAL_API_LATEST', new=True)
+    def test___call___default(self):
+        parser = argparse.ArgumentParser()
+        plugin.build_option_parser(parser)
+        namespace = argparse.Namespace()
+        parser.parse_known_args([], namespace)
+        self.assertEqual(plugin.LATEST_VERSION,
+                         namespace.os_baremetal_api_version)
+        self.assertTrue(plugin.OS_BAREMETAL_API_LATEST)
diff --git a/releasenotes/notes/latest-default-41fdcc49701c4d70.yaml b/releasenotes/notes/latest-default-41fdcc49701c4d70.yaml
new file mode 100644
index 0000000..40a10a5
--- /dev/null
+++ b/releasenotes/notes/latest-default-41fdcc49701c4d70.yaml
@@ -0,0 +1,26 @@
+---
+upgrade:
+  - |
+    The default API version for the bare metal OSC client (``openstack
+    baremetal`` commands) is now "latest", which is the maximum version
+    understood by both the client and the server.  This change makes the CLI
+    automatically pull in new features and changes (including potentially
+    breaking), when talking to new servers.
+
+    Scripts that rely on some specific API behavior should set the
+    ``OS_BAREMETAL_API_VERSION`` environment variable or use the
+    ``--os-baremetal-api-version`` CLI argument.
+
+    .. note:: This change does not affect the Python API.
+features:
+  - |
+    The bare metal OSC client (``openstack baremetal`` commands) now supports
+    the specification of API version ``1``. The actual version used will be
+    the maximum 1.x version understood by both the client and the server.
+    Thus, it is currently identical to the ``latest`` value.
+fixes:
+  - |
+    Users of the ``openstack baremetal`` commands no longer have to specify
+    an explicit API version to use the latest features. The default API version
+    is now "latest", which is the maximum version understood by both the client
+    and the server.