From 5fd5b4c8938e76be3850c05bae7ab7c68e0a7467 Mon Sep 17 00:00:00 2001 From: Akihiro Motoki Date: Fri, 6 Sep 2019 17:26:49 +0900 Subject: [PATCH] Fix version handling compatible with python-semanticversion 2.8 It seems the behavior of 'partial' argument of Version class __init__ has been changed between semantic-version 2.6.0 and 2.8.1 (though I could not identify the root cause). 'partial' argument is marked as deprecated in semantic-vesion 2.7.0, so it is a good chance not to depend on 'partial' argument in horizon. This commit uses Version.coerce() [1] instead to convert non-semver version into a valid semver version. We also need to keep the original version information as it is passed when initializing python-*client (cinderclient and keystoneclient). In addition, the previous implementation based on semantic-version 2.6.0 returns True for "Version("3.55") == 3". It depends on the behavior of "partial" argument of semantic_version.Version. It was not documented and it looks tricky to depend on this behavior. "major" and "minor" properties are now introduced and api/cinder.py is updated accordingly. I believe this approach is clearer and stable. Unit test coverage on dict behavior is improved. Variable names in the unit tests are adjusted to more meaningful ones. [1] https://python-semanticversion.readthedocs.io/en/latest/#coercing-an-arbitrary-version-string Change-Id: If0deee9d0289ff91d58d942b9612f7736356ae18 --- openstack_dashboard/api/base.py | 34 ++++++++++--- openstack_dashboard/api/cinder.py | 4 +- .../test/unit/api/test_base.py | 50 +++++++++++++++---- 3 files changed, 70 insertions(+), 18 deletions(-) diff --git a/openstack_dashboard/api/base.py b/openstack_dashboard/api/base.py index 490b030b54..fd56e3d2b1 100644 --- a/openstack_dashboard/api/base.py +++ b/openstack_dashboard/api/base.py @@ -32,23 +32,45 @@ __all__ = ('APIResourceWrapper', 'APIDictWrapper', @functools.total_ordering class Version(object): + """A class to handle API version. + + The current OpenStack APIs use the versioning of ".", + so this class supports this style only. + """ + # NOTE(amotoki): The implementation depends on the semantic_version library + # but we don't care the patch version in this class. + def __init__(self, version): - self.version = semantic_version.Version(str(version), partial=True) + # NOTE(amotoki): + # All comparisons should use self.sem_ver as we would like to + # compare versions in the semantic versioning way. + # self.orig_ver should be used only in __str__ and __repr__ + # to keep the original version information. + self.orig_ver = str(version) + self.sem_ver = semantic_version.Version.coerce(str(version)) + + @property + def major(self): + return self.sem_ver.major + + @property + def minor(self): + return self.sem_ver.minor def __eq__(self, other): - return self.version == Version(other).version + return self.sem_ver == Version(other).sem_ver def __lt__(self, other): - return self.version < Version(other).version + return self.sem_ver < Version(other).sem_ver def __repr__(self): - return "Version('%s')" % self.version + return "Version('%s')" % self.orig_ver def __str__(self): - return str(self.version) + return str(self.orig_ver) def __hash__(self): - return hash(str(self.version)) + return hash(str(self.sem_ver)) class APIVersionManager(object): diff --git a/openstack_dashboard/api/cinder.py b/openstack_dashboard/api/cinder.py index 1cd3c1db7b..b64361ae53 100644 --- a/openstack_dashboard/api/cinder.py +++ b/openstack_dashboard/api/cinder.py @@ -233,9 +233,9 @@ def cinderclient(request, version=None): (username, token_id, tenant_id, cinder_urls, auth_url) = get_auth_params_from_request(request) version = base.Version(version) - if version == 2: + if version.major == 2: service_names = ('volumev2', 'volume') - elif version == 3: + elif version.major == 3: service_names = ('volumev3', 'volume') else: service_names = ('volume',) diff --git a/openstack_dashboard/test/unit/api/test_base.py b/openstack_dashboard/test/unit/api/test_base.py index 8ca4ced95c..cee4801222 100644 --- a/openstack_dashboard/test/unit/api/test_base.py +++ b/openstack_dashboard/test/unit/api/test_base.py @@ -83,25 +83,55 @@ class APIVersionTests(test.TestCase): self.assertEqual(api_base.Version('1.0'), version) def test_greater(self): - version1 = api_base.Version('1.0') + version10 = api_base.Version('1.0') version12 = api_base.Version('1.2') version120 = api_base.Version('1.20') - self.assertGreater(version12, version1) + self.assertGreater(version12, version10) self.assertGreater(version120, version12) - self.assertEqual(version12, 1) # sic! - self.assertGreater(1.2, version1) + self.assertGreater(version12, 1) + self.assertGreater(1.2, version10) self.assertGreater(version120, 1.2) self.assertGreater('1.20', version12) def test_dict(self): - version1 = api_base.Version('1.0') - version1b = api_base.Version('1.0') - self.assertIn(version1, {version1b: 1}) + test_dict = {api_base.Version('1.0'): 1} + + self.assertIn(api_base.Version('1'), test_dict) + self.assertIn(api_base.Version('1.0'), test_dict) + self.assertIn(api_base.Version('1.0.0'), test_dict) + self.assertIn(api_base.Version(1), test_dict) + self.assertIn(api_base.Version(1.0), test_dict) + + self.assertNotIn(api_base.Version('1.2'), test_dict) + self.assertNotIn(api_base.Version('1.20'), test_dict) + + test_dict = {api_base.Version('1.2'): 1} + self.assertIn(api_base.Version('1.2'), test_dict) + self.assertIn(api_base.Version('1.2.0'), test_dict) + self.assertIn(api_base.Version(1.2), test_dict) + + self.assertNotIn(api_base.Version('1'), test_dict) + self.assertNotIn(api_base.Version('1.0'), test_dict) + self.assertNotIn(api_base.Version('1.0.0'), test_dict) + self.assertNotIn(api_base.Version(1), test_dict) + self.assertNotIn(api_base.Version(1.0), test_dict) + self.assertNotIn(api_base.Version('1.20'), test_dict) def test_text(self): - version1 = api_base.Version('1.0') - self.assertEqual("1.0", str(version1)) - self.assertEqual("Version('1.0')", repr(version1)) + version10 = api_base.Version('1.0') + self.assertEqual("1.0", str(version10)) + self.assertEqual("Version('1.0')", repr(version10)) + + def test_major_minor(self): + version1 = api_base.Version('1') + version10 = api_base.Version('1.0') + version120 = api_base.Version('1.20') + self.assertEqual(1, version1.major) + self.assertEqual(0, version1.minor) + self.assertEqual(1, version10.major) + self.assertEqual(0, version10.minor) + self.assertEqual(1, version120.major) + self.assertEqual(20, version120.minor) # Wrapper classes that only define _attrs don't need extra testing.