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 <matt@mystile.com>

* review: remove duplicated conditional

Co-authored-by: Matt Clay <matt@mystile.com>

changelogs/fragments/validate-modules-deprecated-collections.yml

@@ -0,0 +1,2 @@
+minor_changes:
+    - validate-modules checks for deprecated in collections against meta/routing.yml

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 self.collection:
-        if not (filename_deprecated_or_removed or removed or deprecated or doc_deprecated):
+            mismatched_deprecation = True
-            mismatched_deprecation = False
+            if not (filename_deprecated_or_removed or removed or deprecated or doc_deprecated):
-        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
+            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:
+            if mismatched_deprecation:
-            self.reporter.error(
+                self.reporter.error(
-                path=self.object_path,
+                    path=self.object_path,
-                code='deprecation-mismatch',
+                    code='deprecation-mismatch',
-                msg='Module deprecation/removed must agree in Metadata, by prepending filename with'
+                    msg='Module deprecation/removed must agree in Metadata, by prepending filename with'
-                    ' "_", and setting DOCUMENTATION.deprecated for deprecation or by removing all'
+                        ' "_", and setting DOCUMENTATION.deprecated for deprecation or by removing all'
-                    ' documentation for removed'
+                        ' 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:

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):
+def doc_schema(module_name, version_added=True, deprecated_module=False):
-    deprecated_module = False
 
     if module_name.startswith('_'):
         module_name = module_name[1:]