From 084587913aaab238652a7da1449c7f433837a160 Mon Sep 17 00:00:00 2001 From: John R Barker Date: Thu, 7 May 2020 08:11:46 +0100 Subject: [PATCH] validate-modules: deprecated modules in collections (#68646) * validate-modules: deprecated modules in collections In Collections a module is marked as deprecated via meta/routing.yml Use this file, rather than the leading `_` as part of the deprecated test. * Correct variable * review comments * indentation * Read routing.yml only once * pep8 * Apply suggestions from code review Co-authored-by: Matt Clay * review: remove duplicated conditional Co-authored-by: Matt Clay --- ...alidate-modules-deprecated-collections.yml | 2 + .../validate-modules/validate_modules/main.py | 81 ++++++++++++++----- .../validate_modules/schema.py | 3 +- 3 files changed, 64 insertions(+), 22 deletions(-) create mode 100644 changelogs/fragments/validate-modules-deprecated-collections.yml diff --git a/changelogs/fragments/validate-modules-deprecated-collections.yml b/changelogs/fragments/validate-modules-deprecated-collections.yml new file mode 100644 index 00000000000..cbd1366824c --- /dev/null +++ b/changelogs/fragments/validate-modules-deprecated-collections.yml @@ -0,0 +1,2 @@ +minor_changes: + - validate-modules checks for deprecated in collections against meta/routing.yml diff --git a/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py b/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py index 9e82a816f16..6632b03632e 100644 --- a/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py +++ b/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py @@ -34,6 +34,7 @@ from collections import OrderedDict from contextlib import contextmanager from distutils.version import StrictVersion from fnmatch import fnmatch +import yaml from ansible import __version__ as ansible_version from ansible.executor.module_common import REPLACER_WINDOWS @@ -237,7 +238,7 @@ 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): + def __init__(self, path, analyze_arg_spec=False, collection=None, base_branch=None, git_cache=None, reporter=None, routing=None): super(ModuleValidator, self).__init__(reporter=reporter or Reporter()) self.path = path @@ -247,6 +248,7 @@ class ModuleValidator(Validator): self.analyze_arg_spec = analyze_arg_spec self.collection = collection + self.routing = routing self.base_branch = base_branch self.git_cache = git_cache or GitCache() @@ -882,6 +884,7 @@ class ModuleValidator(Validator): deprecated = False removed = False doc_deprecated = None # doc legally might not exist + routing_says_deprecated = False if self.object_name.startswith('_') and not os.path.islink(self.object_path): filename_deprecated_or_removed = True @@ -923,6 +926,12 @@ class ModuleValidator(Validator): code='missing-metadata-status', msg='ANSIBLE_METADATA.status must be exactly one of "deprecated" or "removed"' ) + else: + # We are testing a collection + if self.routing and self.routing.get('plugin_routing', {}).get('modules', {}).get(self.name, {}).get('deprecation', {}): + # meta/routing.yml says this is deprecated + routing_says_deprecated = True + deprecated = True if not removed: if not bool(doc_info['DOCUMENTATION']['value']): @@ -995,7 +1004,8 @@ class ModuleValidator(Validator): doc, doc_schema( os.readlink(self.object_path).split('.')[0], - version_added=not bool(self.collection) + version_added=not bool(self.collection), + deprecated_module=deprecated, ), 'DOCUMENTATION', 'invalid-documentation', @@ -1006,7 +1016,8 @@ class ModuleValidator(Validator): doc, doc_schema( self.object_name.split('.')[0], - version_added=not bool(self.collection) + version_added=not bool(self.collection), + deprecated_module=deprecated, ), 'DOCUMENTATION', 'invalid-documentation', @@ -1070,23 +1081,42 @@ class ModuleValidator(Validator): ) # Check for mismatched deprecation - mismatched_deprecation = True - if not (filename_deprecated_or_removed or removed or deprecated or doc_deprecated): - mismatched_deprecation = False - else: - if (filename_deprecated_or_removed and deprecated and doc_deprecated): - mismatched_deprecation = False - if (filename_deprecated_or_removed and removed and not (documentation_exists or examples_exist or returns_exist)): + if not self.collection: + mismatched_deprecation = True + if not (filename_deprecated_or_removed or removed or deprecated or doc_deprecated): mismatched_deprecation = False + else: + if (filename_deprecated_or_removed and deprecated and doc_deprecated): + mismatched_deprecation = False + if (filename_deprecated_or_removed and removed and not (documentation_exists or examples_exist or returns_exist)): + mismatched_deprecation = False - if mismatched_deprecation: - self.reporter.error( - path=self.object_path, - code='deprecation-mismatch', - msg='Module deprecation/removed must agree in Metadata, by prepending filename with' - ' "_", and setting DOCUMENTATION.deprecated for deprecation or by removing all' - ' documentation for removed' - ) + if mismatched_deprecation: + self.reporter.error( + path=self.object_path, + code='deprecation-mismatch', + msg='Module deprecation/removed must agree in Metadata, by prepending filename with' + ' "_", and setting DOCUMENTATION.deprecated for deprecation or by removing all' + ' documentation for removed' + ) + else: + # We are testing a collection + if self.object_name.startswith('_'): + self.reporter.error( + path=self.object_path, + code='collections-no-underscore-on-deprecation', + msg='Deprecated content in collections MUST NOT start with "_", update meta/routing.yml instead', + ) + + if not (doc_deprecated == routing_says_deprecated): + # DOCUMENTATION.deprecated and meta/routing.yml disagree + self.reporter.error( + path=self.object_path, + code='deprecation-mismatch', + msg='"meta/routing.yml" and DOCUMENTATION.deprecation do not agree.' + ) + + # In the future we should error if ANSIBLE_METADATA exists in a collection return doc_info, doc @@ -2111,8 +2141,19 @@ def run(): check_dirs = set() + routing = None if args.collection: setup_collection_loader() + routing_file = 'meta/routing.yml' + # Load meta/routing.yml if it exists, as it may contain deprecation information + if os.path.isfile(routing_file): + try: + with open(routing_file) as f: + routing = yaml.safe_load(f) + except yaml.error.MarkedYAMLError as ex: + print('%s:%d:%d: YAML load failed: %s' % (routing_file, ex.context_mark.line + 1, ex.context_mark.column + 1, re.sub(r'\s+', ' ', str(ex)))) + except Exception as ex: # pylint: disable=broad-except + print('%s:%d:%d: YAML load failed: %s' % (routing_file, 0, 0, re.sub(r'\s+', ' ', str(ex)))) for module in args.modules: if os.path.isfile(module): @@ -2122,7 +2163,7 @@ def run(): 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) as mv1: + base_branch=args.base_branch, git_cache=git_cache, reporter=reporter, routing=routing) as mv1: mv1.validate() check_dirs.add(os.path.dirname(path)) @@ -2145,7 +2186,7 @@ def run(): 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) as mv2: + base_branch=args.base_branch, git_cache=git_cache, reporter=reporter, routing=routing) as mv2: mv2.validate() if not args.collection: diff --git a/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/schema.py b/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/schema.py index d88f9d7d7be..e25af9a3fa2 100644 --- a/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/schema.py +++ b/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/schema.py @@ -285,8 +285,7 @@ def author(value): raise Invalid("Invalid author") -def doc_schema(module_name, version_added=True): - deprecated_module = False +def doc_schema(module_name, version_added=True, deprecated_module=False): if module_name.startswith('_'): module_name = module_name[1:]