Add deprecated removed_in_version and deprecated_aliases version tests (#66920)

pull/51907/merge
Felix Fontein 4 years ago committed by GitHub
parent f0b6f76bc6
commit 0e15375ffe
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

@ -0,0 +1,4 @@
minor_changes:
- "ansible-test - add ``validate-modules`` tests for ``removed_in_version`` and ``deprecated_aliases`` (https://github.com/ansible/ansible/pull/66920/)."
- "ansible-test - enable deprecated version testing for modules and ``module.deprecate()`` calls (https://github.com/ansible/ansible/pull/66920/)."
- "ansible-test - allow sanity tests to check for optional errors by specifying ``--enable-optional-errors`` (https://github.com/ansible/ansible/pull/66920/)."

@ -61,7 +61,11 @@ Codes
============================================================ ================== ==================== =========================================================================================
**Error Code** **Type** **Level** **Sample Message**
------------------------------------------------------------ ------------------ -------------------- -----------------------------------------------------------------------------------------
ansible-deprecated-version Documentation Error A feature is deprecated and supposed to be removed in the current or an earlier Ansible version
ansible-invalid-version Documentation Error The Ansible version at which a feature is supposed to be removed cannot be parsed
ansible-module-not-initialized Syntax Error Execution of the module did not result in initialization of AnsibleModule
collection-deprecated-version Documentation Error A feature is deprecated and supposed to be removed in the current or an earlier collection version
collection-invalid-version Documentation Error The collection version at which a feature is supposed to be removed cannot be parsed (it must be a semantic version, see https://semver.org/)
deprecation-mismatch Documentation Error Module marked as deprecated or removed in at least one of the filename, its metadata, or in DOCUMENTATION (setting DOCUMENTATION.deprecated for deprecation or removing all Documentation for removed) but not in all three places.
doc-choices-do-not-match-spec Documentation Error Value for "choices" from the argument_spec does not match the documentation
doc-choices-incompatible-type Documentation Error Choices value from the documentation is not compatible with type defined in the argument_spec

@ -0,0 +1,95 @@
"""Retrieve collection detail."""
from __future__ import (absolute_import, division, print_function)
__metaclass__ = type
import json
import os
import re
import sys
import yaml
# See semantic versioning specification (https://semver.org/)
NUMERIC_IDENTIFIER = r'(?:0|[1-9][0-9]*)'
ALPHANUMERIC_IDENTIFIER = r'(?:[0-9]*[a-zA-Z-][a-zA-Z0-9-]*)'
PRE_RELEASE_IDENTIFIER = r'(?:' + NUMERIC_IDENTIFIER + r'|' + ALPHANUMERIC_IDENTIFIER + r')'
BUILD_IDENTIFIER = r'[a-zA-Z0-9-]+' # equivalent to r'(?:[0-9]+|' + ALPHANUMERIC_IDENTIFIER + r')'
VERSION_CORE = NUMERIC_IDENTIFIER + r'\.' + NUMERIC_IDENTIFIER + r'\.' + NUMERIC_IDENTIFIER
PRE_RELEASE = r'(?:-' + PRE_RELEASE_IDENTIFIER + r'(?:\.' + PRE_RELEASE_IDENTIFIER + r')*)?'
BUILD = r'(?:\+' + BUILD_IDENTIFIER + r'(?:\.' + BUILD_IDENTIFIER + r')*)?'
SEMVER_REGULAR_EXPRESSION = r'^' + VERSION_CORE + PRE_RELEASE + BUILD + r'$'
def validate_version(version):
"""Raise exception if the provided version is not None or a valid semantic version."""
if version is None:
return
if not re.match(SEMVER_REGULAR_EXPRESSION, version):
raise Exception('Invalid version number "{0}". Collection version numbers must '
'follow semantic versioning (https://semver.org/).'.format(version))
def read_manifest_json(collection_path):
"""Return collection information from the MANIFEST.json file."""
manifest_path = os.path.join(collection_path, 'MANIFEST.json')
if not os.path.exists(manifest_path):
return None
try:
with open(manifest_path) as manifest_file:
manifest = json.load(manifest_file)
collection_info = manifest.get('collection_info') or dict()
result = dict(
version=collection_info.get('version'),
)
validate_version(result['version'])
except Exception as ex: # pylint: disable=broad-except
raise Exception('{0}: {1}'.format(os.path.basename(manifest_path), ex))
return result
def read_galaxy_yml(collection_path):
"""Return collection information from the galaxy.yml file."""
galaxy_path = os.path.join(collection_path, 'galaxy.yml')
if not os.path.exists(galaxy_path):
return None
try:
with open(galaxy_path) as galaxy_file:
galaxy = yaml.safe_load(galaxy_file)
result = dict(
version=galaxy.get('version'),
)
validate_version(result['version'])
except Exception as ex: # pylint: disable=broad-except
raise Exception('{0}: {1}'.format(os.path.basename(galaxy_path), ex))
return result
def main():
"""Retrieve collection detail."""
collection_path = sys.argv[1]
try:
result = read_manifest_json(collection_path) or read_galaxy_yml(collection_path) or dict()
except Exception as ex: # pylint: disable=broad-except
result = dict(
error='{0}'.format(ex),
)
print(json.dumps(result))
if __name__ == '__main__':
main()

@ -3,7 +3,6 @@
disable=
abstract-method,
access-member-before-definition,
ansible-deprecated-version,
arguments-differ,
assignment-from-no-return,
assignment-from-none,

@ -3,7 +3,6 @@
disable=
abstract-method,
access-member-before-definition,
ansible-deprecated-version,
arguments-differ,
assignment-from-no-return,
assignment-from-none,

@ -13,6 +13,7 @@ from pylint.checkers import BaseChecker
from pylint.checkers.utils import check_messages
from ansible.release import __version__ as ansible_version_raw
from ansible.utils.version import SemanticVersion
MSGS = {
'E9501': ("Deprecated version (%r) found in call to Display.deprecated "
@ -30,7 +31,20 @@ MSGS = {
"Display.deprecated or AnsibleModule.deprecate",
"ansible-invalid-deprecated-version",
"Used when a call to Display.deprecated specifies an invalid "
"version number",
"Ansible version number",
{'minversion': (2, 6)}),
'E9504': ("Deprecated version (%r) found in call to Display.deprecated "
"or AnsibleModule.deprecate",
"collection-deprecated-version",
"Used when a call to Display.deprecated specifies a collection "
"version less than or equal to the current version of this "
"collection",
{'minversion': (2, 6)}),
'E9505': ("Invalid deprecated version (%r) found in call to "
"Display.deprecated or AnsibleModule.deprecate",
"collection-invalid-deprecated-version",
"Used when a call to Display.deprecated specifies an invalid "
"collection version number",
{'minversion': (2, 6)}),
}
@ -59,6 +73,35 @@ class AnsibleDeprecatedChecker(BaseChecker):
name = 'deprecated'
msgs = MSGS
options = (
('is-collection', {
'default': False,
'type': 'yn',
'metavar': '<y_or_n>',
'help': 'Whether this is a collection or not.',
}),
('collection-version', {
'default': None,
'type': 'string',
'metavar': '<version>',
'help': 'The collection\'s version number used to check deprecations.',
}),
)
def __init__(self, *args, **kwargs):
self.collection_version = None
self.is_collection = False
self.version_constructor = LooseVersion
super(AnsibleDeprecatedChecker, self).__init__(*args, **kwargs)
def set_option(self, optname, value, action=None, optdict=None):
super(AnsibleDeprecatedChecker, self).set_option(optname, value, action, optdict)
if optname == 'collection-version' and value is not None:
self.version_constructor = SemanticVersion
self.collection_version = self.version_constructor(self.config.collection_version)
if optname == 'is-collection':
self.is_collection = self.config.is_collection
@check_messages(*(MSGS.keys()))
def visit_call(self, node):
version = None
@ -83,10 +126,17 @@ class AnsibleDeprecatedChecker(BaseChecker):
return
try:
if ANSIBLE_VERSION >= LooseVersion(str(version)):
loose_version = self.version_constructor(str(version))
if self.is_collection and self.collection_version is not None:
if self.collection_version >= loose_version:
self.add_message('collection-deprecated-version', node=node, args=(version,))
if not self.is_collection and ANSIBLE_VERSION >= loose_version:
self.add_message('ansible-deprecated-version', node=node, args=(version,))
except ValueError:
self.add_message('ansible-invalid-deprecated-version', node=node, args=(version,))
if self.is_collection:
self.add_message('collection-invalid-deprecated-version', node=node, args=(version,))
else:
self.add_message('ansible-invalid-deprecated-version', node=node, args=(version,))
except AttributeError:
# Not the type of node we are interested in
pass

@ -32,8 +32,9 @@ import traceback
from collections import OrderedDict
from contextlib import contextmanager
from distutils.version import StrictVersion
from distutils.version import StrictVersion, LooseVersion
from fnmatch import fnmatch
import yaml
from ansible import __version__ as ansible_version
@ -43,6 +44,7 @@ from ansible.module_utils._text import to_bytes
from ansible.plugins.loader import fragment_loader
from ansible.utils.collection_loader import AnsibleCollectionLoader
from ansible.utils.plugin_docs import BLACKLIST, add_fragments, get_docstring
from ansible.utils.version import SemanticVersion
from .module_args import AnsibleModuleImportError, AnsibleModuleNotInitialized, get_argument_spec
@ -87,6 +89,9 @@ SUBPROCESS_REGEX = re.compile(r'subprocess\.Po.*')
OS_CALL_REGEX = re.compile(r'os\.call.*')
LOOSE_ANSIBLE_VERSION = LooseVersion('.'.join(ansible_version.split('.')[:3]))
class ReporterEncoder(json.JSONEncoder):
def default(self, o):
if isinstance(o, Exception):
@ -238,7 +243,8 @@ class ModuleValidator(Validator):
WHITELIST_FUTURE_IMPORTS = frozenset(('absolute_import', 'division', 'print_function'))
def __init__(self, path, analyze_arg_spec=False, collection=None, base_branch=None, git_cache=None, reporter=None, routing=None):
def __init__(self, path, analyze_arg_spec=False, collection=None, collection_version=None,
base_branch=None, git_cache=None, reporter=None, routing=None):
super(ModuleValidator, self).__init__(reporter=reporter or Reporter())
self.path = path
@ -247,8 +253,15 @@ class ModuleValidator(Validator):
self.analyze_arg_spec = analyze_arg_spec
self.Version = LooseVersion
self.collection = collection
self.routing = routing
self.collection_version = None
if collection_version is not None:
self.Version = SemanticVersion
self.collection_version_str = collection_version
self.collection_version = self.Version(collection_version)
self.base_branch = base_branch
self.git_cache = git_cache or GitCache()
@ -1450,6 +1463,74 @@ class ModuleValidator(Validator):
msg=msg,
)
continue
if not self.collection or self.collection_version is not None:
if self.collection:
compare_version = self.collection_version
version_of_what = "this collection (%s)" % self.collection_version_str
version_parser_error = "the version number is not a valid semantic version (https://semver.org/)"
code_prefix = 'collection'
else:
compare_version = LOOSE_ANSIBLE_VERSION
version_of_what = "Ansible (%s)" % ansible_version
version_parser_error = "the version number cannot be parsed"
code_prefix = 'ansible'
removed_in_version = data.get('removed_in_version', None)
if removed_in_version is not None:
try:
if compare_version >= self.Version(str(removed_in_version)):
msg = "Argument '%s' in argument_spec" % arg
if context:
msg += " found in %s" % " -> ".join(context)
msg += " has a deprecated removed_in_version '%s'," % removed_in_version
msg += " i.e. the version is less than or equal to the current version of %s" % version_of_what
self.reporter.error(
path=self.object_path,
code=code_prefix + '-deprecated-version',
msg=msg,
)
except ValueError:
msg = "Argument '%s' in argument_spec" % arg
if context:
msg += " found in %s" % " -> ".join(context)
msg += " has an invalid removed_in_version '%s'," % removed_in_version
msg += " i.e. %s" % version_parser_error
self.reporter.error(
path=self.object_path,
code=code_prefix + '-invalid-version',
msg=msg,
)
deprecated_aliases = data.get('deprecated_aliases', None)
if deprecated_aliases is not None:
for deprecated_alias in deprecated_aliases:
try:
if compare_version >= self.Version(str(deprecated_alias['version'])):
msg = "Argument '%s' in argument_spec" % arg
if context:
msg += " found in %s" % " -> ".join(context)
msg += " has deprecated aliases '%s' with removal in version '%s'," % (
deprecated_alias['name'], deprecated_alias['version'])
msg += " i.e. the version is less than or equal to the current version of %s" % version_of_what
self.reporter.error(
path=self.object_path,
code=code_prefix + '-deprecated-version',
msg=msg,
)
except ValueError:
msg = "Argument '%s' in argument_spec" % arg
if context:
msg += " found in %s" % " -> ".join(context)
msg += " has aliases '%s' with removal in invalid version '%s'," % (
deprecated_alias['name'], deprecated_alias['version'])
msg += " i.e. %s" % version_parser_error
self.reporter.error(
path=self.object_path,
code=code_prefix + '-invalid-version',
msg=msg,
)
aliases = data.get('aliases', [])
if arg in aliases:
msg = "Argument '%s' in argument_spec" % arg
@ -2131,6 +2212,9 @@ def run():
'validating files within a collection. Ensure '
'that ANSIBLE_COLLECTIONS_PATHS is set so the '
'contents of the collection can be located')
parser.add_argument('--collection-version',
help='The collection\'s version number used to check '
'deprecations')
args = parser.parse_args()
@ -2162,8 +2246,9 @@ def run():
continue
if ModuleValidator.is_blacklisted(path):
continue
with ModuleValidator(path, collection=args.collection, analyze_arg_spec=args.arg_spec,
base_branch=args.base_branch, git_cache=git_cache, reporter=reporter, routing=routing) as mv1:
with ModuleValidator(path, collection=args.collection, collection_version=args.collection_version,
analyze_arg_spec=args.arg_spec, base_branch=args.base_branch,
git_cache=git_cache, reporter=reporter, routing=routing) as mv1:
mv1.validate()
check_dirs.add(os.path.dirname(path))
@ -2185,8 +2270,9 @@ def run():
continue
if ModuleValidator.is_blacklisted(path):
continue
with ModuleValidator(path, collection=args.collection, analyze_arg_spec=args.arg_spec,
base_branch=args.base_branch, git_cache=git_cache, reporter=reporter, routing=routing) as mv2:
with ModuleValidator(path, collection=args.collection, collection_version=args.collection_version,
analyze_arg_spec=args.arg_spec, base_branch=args.base_branch,
git_cache=git_cache, reporter=reporter, routing=routing) as mv2:
mv2.validate()
if not args.collection:

@ -217,3 +217,36 @@ def check_pyyaml(args, version):
display.warning('PyYAML will be slow due to installation without libyaml support for interpreter: %s' % python)
return result
class CollectionDetail:
"""Collection detail."""
def __init__(self): # type: () -> None
self.version = None # type: t.Optional[str]
class CollectionDetailError(ApplicationError):
"""An error occurred retrieving collection detail."""
def __init__(self, reason): # type: (str) -> None
super(CollectionDetailError, self).__init__('Error collecting collection detail: %s' % reason)
self.reason = reason
def get_collection_detail(args, python): # type: (EnvironmentConfig, str) -> CollectionDetail
"""Return collection detail."""
collection = data_context().content.collection
directory = os.path.join(collection.root, collection.directory)
stdout = run_command(args, [python, os.path.join(ANSIBLE_TEST_DATA_ROOT, 'collection_detail.py'), directory], capture=True, always=True)[0]
result = json.loads(stdout)
error = result.get('error')
if error:
raise CollectionDetailError(error)
version = result.get('version')
detail = CollectionDetail()
detail.version = str(version) if version is not None else None
return detail

@ -530,6 +530,10 @@ def parse_args():
sanity.add_argument('--base-branch',
help=argparse.SUPPRESS)
sanity.add_argument('--enable-optional-errors',
action='store_true',
help='enable optional errors')
add_lint(sanity)
add_extra_docker_options(sanity, integration=False)

@ -241,6 +241,7 @@ class SanityConfig(TestConfig):
self.skip_test = args.skip_test # type: t.List[str]
self.list_tests = args.list_tests # type: bool
self.allow_disabled = args.allow_disabled # type: bool
self.enable_optional_errors = args.enable_optional_errors # type: bool
if args.base_branch:
self.base_branch = args.base_branch # str

@ -396,6 +396,11 @@ class SanityIgnoreParser:
if len(error_codes) > 1:
self.parse_errors.append((line_no, len(path) + len(test_name) + len(error_code) + 3, "Error code cannot contain multiple ':' characters"))
continue
if error_code in test.optional_error_codes:
self.parse_errors.append((line_no, len(path) + len(test_name) + 3, "Optional error code '%s' cannot be ignored" % (
error_code)))
continue
else:
if error_codes:
self.parse_errors.append((line_no, len(path) + len(test_name) + 2, "Sanity test '%s' does not support error codes" % test_name))
@ -470,6 +475,9 @@ class SanityIgnoreProcessor:
filtered = []
for message in messages:
if message.code in self.test.optional_error_codes and not self.args.enable_optional_errors:
continue
path_entry = self.ignore_entries.get(message.path)
if path_entry:
@ -611,6 +619,12 @@ class SanityTest(ABC):
self.name = name
self.enabled = True
# Optional error codes represent errors which spontaneously occur without changes to the content under test, such as those based on the current date.
# Because these errors can be unpredictable they behave differently than normal error codes:
# * They are not reported by default. The `--enable-optional-errors` option must be used to display these errors.
# * They cannot be ignored. This is done to maintain the integrity of the ignore system.
self.optional_error_codes = set()
@property
def error_code(self): # type: () -> t.Optional[str]
"""Error code for ansible-test matching the format used by the underlying test program, or None if the program does not use error codes."""

@ -35,6 +35,9 @@ from ..util_common import (
from ..ansible_util import (
ansible_environment,
get_collection_detail,
CollectionDetail,
CollectionDetailError,
)
from ..config import (
@ -138,6 +141,17 @@ class PylintTest(SanitySingleVersion):
python = find_python(python_version)
collection_detail = None
if data_context().content.collection:
try:
collection_detail = get_collection_detail(args, python)
if not collection_detail.version:
display.warning('Skipping pylint collection version checks since no collection version was found.')
except CollectionDetailError as ex:
display.warning('Skipping pylint collection version checks since collection detail loading failed: %s' % ex.reason)
test_start = datetime.datetime.utcnow()
for context, context_paths in sorted(contexts):
@ -145,7 +159,7 @@ class PylintTest(SanitySingleVersion):
continue
context_start = datetime.datetime.utcnow()
messages += self.pylint(args, context, context_paths, plugin_dir, plugin_names, python)
messages += self.pylint(args, context, context_paths, plugin_dir, plugin_names, python, collection_detail)
context_end = datetime.datetime.utcnow()
context_times.append('%s: %d (%s)' % (context, len(context_paths), context_end - context_start))
@ -184,6 +198,7 @@ class PylintTest(SanitySingleVersion):
plugin_dir, # type: str
plugin_names, # type: t.List[str]
python, # type: str
collection_detail, # type: CollectionDetail
): # type: (...) -> t.List[t.Dict[str, str]]
"""Run pylint using the config specified by the context on the specified paths."""
rcfile = os.path.join(SANITY_ROOT, 'pylint', 'config', context.split('/')[0] + '.cfg')
@ -216,6 +231,14 @@ class PylintTest(SanitySingleVersion):
'--load-plugins', ','.join(load_plugins),
] + paths
if data_context().content.collection:
cmd.extend(['--is-collection', 'yes'])
if collection_detail and collection_detail.version:
cmd.extend(['--collection-version', collection_detail.version])
else:
cmd.extend(['--enable', 'ansible-deprecated-version'])
append_python_path = [plugin_dir]
if data_context().content.collection:

@ -31,6 +31,8 @@ from ..util_common import (
from ..ansible_util import (
ansible_environment,
get_collection_detail,
CollectionDetailError,
)
from ..config import (
@ -66,8 +68,10 @@ class ValidateModulesTest(SanitySingleVersion):
paths = [target.path for target in targets.include]
python = find_python(python_version)
cmd = [
find_python(python_version),
python,
os.path.join(SANITY_ROOT, 'validate-modules', 'validate-modules'),
'--format', 'json',
'--arg-spec',
@ -75,6 +79,16 @@ class ValidateModulesTest(SanitySingleVersion):
if data_context().content.collection:
cmd.extend(['--collection', data_context().content.collection.directory])
try:
collection_detail = get_collection_detail(args, python)
if collection_detail.version:
cmd.extend(['--collection-version', collection_detail.version])
else:
display.warning('Skipping validate-modules collection version checks since no collection version was found.')
except CollectionDetailError as ex:
display.warning('Skipping validate-modules collection version checks since collection detail loading failed: %s' % ex.reason)
else:
if args.base_branch:
cmd.extend([

@ -467,6 +467,7 @@ test/units/module_utils/basic/test__symbolic_mode_to_octal.py future-import-boil
test/units/module_utils/basic/test_deprecate_warn.py future-import-boilerplate
test/units/module_utils/basic/test_deprecate_warn.py metaclass-boilerplate
test/units/module_utils/basic/test_deprecate_warn.py pylint:ansible-deprecated-no-version
test/units/module_utils/basic/test_deprecate_warn.py pylint:ansible-deprecated-version
test/units/module_utils/basic/test_exit_json.py future-import-boilerplate
test/units/module_utils/basic/test_get_file_attributes.py future-import-boilerplate
test/units/module_utils/basic/test_heuristic_log_sanitize.py future-import-boilerplate

Loading…
Cancel
Save