Skip to content

Commit

Permalink
ansible-test: do not accept empty string as valid version number (ans…
Browse files Browse the repository at this point in the history 
…ible#69816)

* Work around strange behavior of StrictVersion and SemanticVersion constructors that they accept an falsy value.

* Do not accept empty strings as versions.
  • Loading branch information
@felixfontein
felixfontein authored Jun 4, 2020
1 parent 3b00c16 commit 2dbd5dc
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 21 deletions.
4 changes: 4 additions & 0 deletions test/lib/ansible_test/_data/sanity/pylint/plugins/deprecated.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,8 @@ def _check_version(self, node, version):
if collection == 'ansible.builtin':
# Ansible-base
try:
if not version_no:
raise ValueError('Version string should not be empty')
loose_version = LooseVersion(str(version_no))
if ANSIBLE_VERSION >= loose_version:
self.add_message('ansible-deprecated-version', node=node, args=(version,))
Expand All @@ -213,6 +215,8 @@ def _check_version(self, node, version):
else:
# Collections
try:
if not version_no:
raise ValueError('Version string should not be empty')
semantic_version = SemanticVersion(version_no)
if collection == self.collection_name and self.collection_version is not None:
if self.collection_version >= semantic_version:
Expand Down
49 changes: 28 additions & 21 deletions test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,21 +254,21 @@ def __init__(self, path, analyze_arg_spec=False, collection=None, collection_ver

self.analyze_arg_spec = analyze_arg_spec

self.Version = LooseVersion
self.StrictVersion = StrictVersion
self._Version = LooseVersion
self._StrictVersion = StrictVersion

self.collection = collection
self.collection_name = 'ansible.builtin'
if self.collection:
self.Version = SemanticVersion
self.StrictVersion = SemanticVersion
self._Version = SemanticVersion
self._StrictVersion = SemanticVersion
collection_namespace_path, collection_name = os.path.split(self.collection)
self.collection_name = '%s.%s' % (os.path.basename(collection_namespace_path), collection_name)
self.routing = routing
self.collection_version = None
if collection_version is not None:
self.collection_version_str = collection_version
self.collection_version = self.Version(collection_version)
self.collection_version = SemanticVersion(collection_version)

self.base_branch = base_branch
self.git_cache = git_cache or GitCache()
Expand All @@ -288,6 +288,16 @@ def __init__(self, path, analyze_arg_spec=False, collection=None, collection_ver
else:
self.base_module = None

def _create_version(self, v):
if not v:
raise ValueError('Empty string is not a valid version')
return self._Version(v)

def _create_strict_version(self, v):
if not v:
raise ValueError('Empty string is not a valid version')
return self._StrictVersion(v)

def __enter__(self):
return self

Expand Down Expand Up @@ -1193,7 +1203,8 @@ def _validate_docs(self):
def _check_version_added(self, doc, existing_doc):
version_added_raw = doc.get('version_added')
try:
version_added = self.StrictVersion(self._extract_version_from_tag_for_msg(str(doc.get('version_added', '0.0') or '0.0')))
version_added = self._create_strict_version(
self._extract_version_from_tag_for_msg(str(doc.get('version_added', '0.0') or '0.0')))
except ValueError:
version_added = doc.get('version_added', '0.0')
if self._is_new_module() or version_added != 'ansible.builtin:historical':
Expand All @@ -1219,7 +1230,7 @@ def _check_version_added(self, doc, existing_doc):
return

should_be = '.'.join(ansible_version.split('.')[:2])
strict_ansible_version = self.StrictVersion(should_be)
strict_ansible_version = self._create_strict_version(should_be)

if (version_added < strict_ansible_version or
strict_ansible_version < version_added):
Expand Down Expand Up @@ -1585,7 +1596,7 @@ def _validate_argument_spec(self, docs, spec, kwargs, context=None, last_context
if removed_in_version is not None:
try:
collection_name, removed_in_version = self._split_tagged_version(removed_in_version)
if collection_name == self.collection_name and compare_version >= self.Version(str(removed_in_version)):
if collection_name == self.collection_name and compare_version >= self._create_version(str(removed_in_version)):
msg = "Argument '%s' in argument_spec" % arg
if context:
msg += " found in %s" % " -> ".join(context)
Expand All @@ -1605,7 +1616,7 @@ def _validate_argument_spec(self, docs, spec, kwargs, context=None, last_context
if 'name' in deprecated_alias and 'version' in deprecated_alias:
try:
collection_name, version = self._split_tagged_version(deprecated_alias['version'])
if collection_name == self.collection_name and compare_version >= self.Version(str(version)):
if collection_name == self.collection_name and compare_version >= self._create_version(str(version)):
msg = "Argument '%s' in argument_spec" % arg
if context:
msg += " found in %s" % " -> ".join(context)
Expand Down Expand Up @@ -2022,12 +2033,10 @@ def _check_for_new_args(self, doc, metadata):
return

try:
mod_version_added = self.StrictVersion()
mod_version_added.parse(
self._extract_version_from_tag_for_msg(str(existing_doc.get('version_added', '0.0')))
)
mod_version_added = self._create_strict_version(
self._extract_version_from_tag_for_msg(str(existing_doc.get('version_added', '0.0'))))
except ValueError:
mod_version_added = self.StrictVersion('0.0')
mod_version_added = self._create_strict_version('0.0')

if self.base_branch and 'stable-' in self.base_branch:
metadata.pop('metadata_version', None)
Expand All @@ -2042,7 +2051,7 @@ def _check_for_new_args(self, doc, metadata):
options = doc.get('options', {}) or {}

should_be = '.'.join(ansible_version.split('.')[:2])
strict_ansible_version = self.StrictVersion(should_be)
strict_ansible_version = self._create_strict_version(should_be)

for option, details in options.items():
try:
Expand All @@ -2069,10 +2078,8 @@ def _check_for_new_args(self, doc, metadata):
continue

try:
version_added = self.StrictVersion()
version_added.parse(
self._extract_version_from_tag_for_msg(str(details.get('version_added', '0.0')))
)
version_added = self._create_strict_version(
self._extract_version_from_tag_for_msg(str(details.get('version_added', '0.0'))))
except ValueError:
# already reported during schema validation
continue
Expand Down Expand Up @@ -2167,12 +2174,12 @@ def validate(self):
)
# Treat the module as not to be removed:
raise ValueError('')
removed_in = self.StrictVersion(str(version))
removed_in = self._create_strict_version(str(version))
except ValueError:
end_of_deprecation_should_be_removed_only = False
else:
if not self.collection:
strict_ansible_version = self.StrictVersion('.'.join(ansible_version.split('.')[:2]))
strict_ansible_version = self._create_strict_version('.'.join(ansible_version.split('.')[:2]))
end_of_deprecation_should_be_removed_only = strict_ansible_version >= removed_in
elif self.collection_version:
strict_ansible_version = self.collection_version
Expand Down
4 changes: 4 additions & 0 deletions test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/schema.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ def _add_ansible_error_code(exception, error_code):
def semantic_version(v, error_code=None):
if not isinstance(v, string_types):
raise _add_ansible_error_code(Invalid('Semantic version must be a string'), error_code or 'collection-invalid-version')
if not v:
raise _add_ansible_error_code(
Invalid('Empty string is not a valid semantic version'),
error_code or 'collection-invalid-version')
try:
SemanticVersion(v)
except ValueError as e:
Expand Down

0 comments on commit 2dbd5dc

Please sign in to comment.