Make discover._version_between more consistent
Partly in response to [1], and partly for the sake of making its API consistent and understandable, discover._version_between now allows un-normalized input, and responds sensibly to unspecified upper & lower bounds. Previously the method treated None as follows: min_version max_version behavior None None Always False not None None ValueError not None not None Compare min to candidate None not None Compare max to candidate That's whack, yo. Took me like ten minutes just to come up with that chart. This change allows us to explain clearly what happens when upper or lower bounds are None: it translates naturally to "there is no bound". Given that, the first line of the chart (None/None) now always returns True, which is a behavior change the callers need to be updated to expect. This is an acceptable sacrifice for making this method sane. [1] https://review.openstack.org/#/c/483604/9/keystoneauth1/discover.py@306 Change-Id: I09b6cf692c8bfa290b73c8d7498bc12a5e91d690
This commit is contained in:
Eric Fried
parent
4b15f5706a
commit
1de528ece2
|
|
@ -276,42 +276,34 @@ def version_to_string(version):
|
|||
def _version_between(min_version, max_version, candidate):
|
||||
"""Determine whether a candidate version is within a specified range.
|
||||
|
||||
:param min_version: Normalized lower bound. May be None. May be
|
||||
(LATEST, LATEST).
|
||||
:param max_version: Normalized upper bound. May be None. May be
|
||||
(LATEST, LATEST).
|
||||
:param candidate: Normalized candidate version to test. May not be None.
|
||||
:param min_version: The minimum version that is acceptable.
|
||||
None/empty indicates no lower bound.
|
||||
:param max_version: The maximum version that is acceptable.
|
||||
None/empty indicates no upper bound.
|
||||
:param candidate: Candidate version to test. May not be None/empty.
|
||||
:return: True if candidate is between min_version and max_version; False
|
||||
otherwise.
|
||||
:raises ValueError: If candidate is None or the input is not properly
|
||||
normalized.
|
||||
:raises ValueError: If candidate is None.
|
||||
:raises TypeError: If any input cannot be normalized.
|
||||
"""
|
||||
def is_normalized(ver):
|
||||
return normalize_version_number(ver) == ver
|
||||
if not candidate:
|
||||
raise ValueError("candidate is required.")
|
||||
candidate = normalize_version_number(candidate)
|
||||
|
||||
# A version can't be between a range that doesn't exist
|
||||
if not min_version and not max_version:
|
||||
# Normalize up front to validate any malformed inputs
|
||||
if min_version:
|
||||
min_version = normalize_version_number(min_version)
|
||||
if max_version:
|
||||
max_version = normalize_version_number(max_version)
|
||||
|
||||
# If the candidate is less than the min_version, it's not a match.
|
||||
# No min_version means no lower bound.
|
||||
if min_version and candidate < min_version:
|
||||
return False
|
||||
|
||||
if candidate is None:
|
||||
raise ValueError("candidate cannot be None.")
|
||||
|
||||
if min_version is not None and not is_normalized(min_version):
|
||||
raise ValueError("min_version is not normalized.")
|
||||
if max_version is not None and not is_normalized(max_version):
|
||||
raise ValueError("max_version is not normalized.")
|
||||
if not is_normalized(candidate):
|
||||
raise ValueError("candidate is not normalized.")
|
||||
# This is only possible if args weren't run through _normalize_version_args
|
||||
if max_version is None and min_version is not None:
|
||||
raise ValueError("Can't use None as an upper bound.")
|
||||
|
||||
# If the candidate is less than the min_version, it's
|
||||
# not a match. None works here.
|
||||
if min_version is not None and candidate < min_version:
|
||||
return False
|
||||
|
||||
if max_version is not None and candidate > max_version:
|
||||
# If the candidate is higher than the max_version, it's not a match.
|
||||
# No max_version means no upper bound.
|
||||
if max_version and candidate > max_version:
|
||||
return False
|
||||
|
||||
return True
|
||||
|
|
@ -633,7 +625,9 @@ class Discover(object):
|
|||
return data
|
||||
if _latest_soft_match(min_version, data['version']):
|
||||
return data
|
||||
if _version_between(min_version, max_version, data['version']):
|
||||
# Only validate version bounds if versions were specified
|
||||
if min_version and max_version and _version_between(
|
||||
min_version, max_version, data['version']):
|
||||
return data
|
||||
|
||||
# If there is no version requested and we could not find a matching
|
||||
|
|
@ -1019,8 +1013,10 @@ class EndpointData(object):
|
|||
except TypeError:
|
||||
pass
|
||||
else:
|
||||
is_between = _version_between(min_version, max_version,
|
||||
url_version)
|
||||
# `is_between` means version bounds were specified *and* the URL
|
||||
# version is between them.
|
||||
is_between = min_version and max_version and _version_between(
|
||||
min_version, max_version, url_version)
|
||||
exact_match = (is_between and max_version and
|
||||
max_version[0] == url_version[0])
|
||||
high_match = (is_between and max_version and
|
||||
|
|
|
|||
|
|
@ -386,30 +386,61 @@ class DiscoverUtils(utils.TestCase):
|
|||
def bad(minver, maxver, cand):
|
||||
self.assertFalse(discover._version_between(minver, maxver, cand))
|
||||
|
||||
def exc(minver, maxver, cand):
|
||||
self.assertRaises(ValueError,
|
||||
def exc(excls, minver, maxver, cand):
|
||||
self.assertRaises(excls,
|
||||
discover._version_between, minver, maxver, cand)
|
||||
|
||||
# candidate required
|
||||
exc(ValueError, (1, 0), (1, 0), None)
|
||||
exc(ValueError, 'v1.0', '1.0', '')
|
||||
# malformed candidate
|
||||
exc(TypeError, None, None, 'bogus')
|
||||
exc(TypeError, None, None, (1, 'two'))
|
||||
# malformed min_version
|
||||
exc(TypeError, 'bogus', None, (1, 0))
|
||||
exc(TypeError, (1, 'two'), None, (1, 0))
|
||||
# malformed max_version
|
||||
exc(TypeError, None, 'bogus', (1, 0))
|
||||
exc(TypeError, None, (1, 'two'), (1, 0))
|
||||
|
||||
# fail on minimum
|
||||
bad((2, 4), None, (1, 55))
|
||||
bad('v2.4', '', '2.3')
|
||||
bad('latest', None, (2, 3000))
|
||||
bad((2, discover.LATEST), '', 'v2.3000')
|
||||
bad((2, 3000), '', (1, discover.LATEST))
|
||||
bad('latest', None, 'v1000.latest')
|
||||
# fail on maximum
|
||||
bad(None, (2, 4), (2, 5))
|
||||
bad('', 'v2.4', '2.5')
|
||||
bad(None, (2, discover.LATEST), (3, 0))
|
||||
bad('', '2000.latest', 'latest')
|
||||
|
||||
# candidate matches a bound
|
||||
good((1, 0), (1, 0), (1, 0))
|
||||
good('1.0', '2.9', '1.0')
|
||||
good('v1.0', 'v2.9', 'v2.9')
|
||||
# properly in between
|
||||
good((1, 0), (1, 10), (1, 2))
|
||||
good(None, (1, 10), (1, 2))
|
||||
good((1, 20), (2, 0), (1, 21))
|
||||
good((1, 0), (2, discover.LATEST), (1, 21))
|
||||
good((1, 0), (2, discover.LATEST), (1, discover.LATEST))
|
||||
good((1, 50), (2, discover.LATEST), (2, discover.LATEST))
|
||||
|
||||
bad((discover.LATEST, discover.LATEST),
|
||||
(discover.LATEST, discover.LATEST), (1, 0))
|
||||
bad(None, None, (1, 0))
|
||||
bad((1, 50), (2, discover.LATEST), (3, 0))
|
||||
bad((1, 50), (2, discover.LATEST), (3, discover.LATEST))
|
||||
bad((1, 50), (2, 5), (2, discover.LATEST))
|
||||
|
||||
exc((1, 0), (1, 0), None)
|
||||
exc('v1.0', (1, 0), (1, 0))
|
||||
exc((1, 0), 'v1.0', (1, 0))
|
||||
exc((1, 0), (1, 0), 'v1.0')
|
||||
exc((1, 0), None, (1, 0))
|
||||
good('1', '2', '1.2')
|
||||
# no lower bound
|
||||
good(None, (2, 5), (2, 3))
|
||||
# no upper bound
|
||||
good('2.5', '', '2.6')
|
||||
# no bounds at all
|
||||
good('', '', 'v1')
|
||||
good(None, None, (999, 999))
|
||||
good(None, None, 'latest')
|
||||
# Various good 'latest' scenarios
|
||||
good((discover.LATEST, discover.LATEST),
|
||||
(discover.LATEST, discover.LATEST),
|
||||
(discover.LATEST, discover.LATEST))
|
||||
good((discover.LATEST, discover.LATEST), None,
|
||||
(discover.LATEST, discover.LATEST))
|
||||
good('', 'latest', 'latest')
|
||||
good('2.latest', '3.latest', '3.0')
|
||||
good('2.latest', None, (55, 66))
|
||||
good(None, '3.latest', '3.9999')
|
||||
|
||||
|
||||
class VersionDataTests(utils.TestCase):
|
||||
|
|
|
|||
Loading…
Reference in New Issue