From c534b9465d4c7cea8944d282665c960285c3e162 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 18 Oct 2017 11:50:29 +0200 Subject: [PATCH] Set the default API version of OSC CLI to "latest" Now the default of OSC is the negotiated maximum version understood by both the server and the client. The value of "1" is now equivalent to "latest" as well. This change also cleans up unit tests for the OSC plugin. Change-Id: I489fee937a356b523eb35379dce3631195132fe5 Closes-Bug: #1671145 --- ironicclient/osc/plugin.py | 55 ++++---- .../osc/v1/test_baremetal_node_basic.py | 35 +++--- .../test_baremetal_node_provision_states.py | 49 ++++---- ironicclient/tests/unit/osc/fakes.py | 3 +- ironicclient/tests/unit/osc/test_plugin.py | 119 ++++++++---------- .../latest-default-41fdcc49701c4d70.yaml | 26 ++++ 6 files changed, 148 insertions(+), 139 deletions(-) create mode 100644 releasenotes/notes/latest-default-41fdcc49701c4d70.yaml diff --git a/ironicclient/osc/plugin.py b/ironicclient/osc/plugin.py index c4c2fd42f..5723672f5 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='', - 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 d0729caea..2d1be7c81 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 acc24a219..350582c34 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 cd9731bb9..76d77e60d 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 266b3a2f4..f4cff9952 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='') + 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='') + 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='') + 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 000000000..40a10a5ca --- /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.