Also check for proper versionadded versions

Refs #59077

.github/workflows/pre-commit.yml

@@ -47,7 +47,7 @@ jobs:
     - name: Check ALL Files On Branch
       if: github.event_name != 'pull_request'
       env:
-        SKIP: lint-salt,lint-tests,pyupgrade,remove-import-headers,rstcheck,loader-check-module-docstrings
+        SKIP: lint-salt,lint-tests,pyupgrade,remove-import-headers,rstcheck,check-docstrings
       run: |
         pre-commit run --show-diff-on-failure --color=always --all-files
 

.pre-commit-config.yaml

@@ -1220,7 +1220,7 @@ repos:
     hooks:
       - id: invoke
         alias: loader-check-virtual
-        name: Check modules __virtual__
+        name: Check loader modules __virtual__
         files: salt/.*\.py$
         exclude: >
             (?x)^(
@@ -1240,14 +1240,22 @@ repos:
     rev: v1.5.0
     hooks:
       - id: invoke
-        alias: loader-check-module-docstrings
+        alias: check-docstrings
-        name: Check salt/modules/*.py docstrings
+        name: Check docstrings
-        files: salt/modules/.*\.py$
+        files: salt/.*\.py$
+        exclude: >
+            (?x)^(
+                templates/.*|
+                salt/ext/.*|
+            )$
         args:
-          - loader.check-module-docstrings
+          - docstrings.check
         additional_dependencies:
           - blessings
           - pyyaml
+          - distro
+          - jinja2
+          - msgpack
 
   - repo: https://github.com/saltstack/mirrors-nox
     rev: v2020.8.22

cicd/jenkins/pr-pre-commit

@@ -5,7 +5,7 @@ if (env.CHANGE_ID) {
     pre_commit_skips = ''
 } else {
     // This is a branch build
-    pre_commit_skips = 'pyupgrade,remove-import-headers,rstcheck,loader-check-module-docstrings'
+    pre_commit_skips = 'pyupgrade,remove-import-headers,rstcheck,check-docstrings'
 }
 
 runPreCommit(

noxfile.py

@@ -1060,11 +1060,13 @@ def invoke(session):
     Run invoke tasks
     """
     if _upgrade_pip_setuptools_and_wheel(session):
+        _install_requirements(session, "zeromq")
         requirements_file = os.path.join(
             "requirements", "static", "ci", _get_pydir(session), "invoke.txt"
         )
         install_command = ["--progress-bar=off", "-r", requirements_file]
         session.install(*install_command, silent=PIP_INSTALL_SILENT)
+
     cmd = ["inv"]
     files = []
 

tasks/__init__.py

@@ -1,8 +1,11 @@
 from invoke import Collection  # pylint: disable=3rd-party-module-not-gated
 
-from . import docs, filemap, loader
+from . import docs, docstrings, filemap, loader
 
 ns = Collection()
 ns.add_collection(Collection.from_module(docs, name="docs"), name="docs")
+ns.add_collection(
+    Collection.from_module(docstrings, name="docstrings"), name="docstrings"
+)
 ns.add_collection(Collection.from_module(loader, name="loader"), name="loader")
 ns.add_collection(Collection.from_module(filemap, name="filemap"), name="filemap")

tasks/docstrings.py

@@ -0,0 +1,162 @@
+"""
+    tasks.docstrings
+    ~~~~~~~~~~~~~~~~
+
+    Docstrings related tasks
+"""
+
+import ast
+import pathlib
+import re
+
+from invoke import task  # pylint: disable=3rd-party-module-not-gated
+from salt.loader import SALT_INTERNAL_LOADERS_PATHS
+from salt.version import SaltStackVersion
+from tasks import utils
+
+CODE_DIR = pathlib.Path(__file__).resolve().parent.parent
+SALT_CODE_DIR = CODE_DIR / "salt"
+SALT_MODULES_PATH = SALT_CODE_DIR / "modules"
+
+
+@task(iterable=["files"], positional=["files"])
+def check(ctx, files, check_proper_formatting=False):
+    """
+    Check salt's docstrings
+    """
+    # CD into Salt's repo root directory
+    ctx.cd(CODE_DIR)
+
+    # Unfortunately invoke does not support nargs.
+    # We migth have been passed --files="foo.py bar.py"
+    # Turn that into a list of paths
+    _files = []
+    for path in files:
+        if not path:
+            continue
+        _files.extend(path.split())
+    if not _files:
+        _files = SALT_CODE_DIR.rglob("*.py")
+    else:
+        _files = [pathlib.Path(fname) for fname in _files]
+
+    _files = [path.resolve() for path in _files]
+
+    errors = 0
+    exitcode = 0
+    for path in _files:
+        module = ast.parse(path.read_text(), filename=str(path))
+        module_docstring = ast.get_docstring(module)
+        if module_docstring:
+            error = _check_valid_versionadded(module_docstring)
+            if error:
+                errors += 1
+                exitcode = 1
+                utils.error(
+                    "The module '{}' does not provide a proper `versionadded` version: {!r} is not valid.",
+                    path.relative_to(CODE_DIR),
+                    error,
+                )
+
+        for funcdef in [
+            node for node in module.body if isinstance(node, ast.FunctionDef)
+        ]:
+            docstring = ast.get_docstring(funcdef)
+            if docstring:
+                error = _check_valid_versionadded(docstring)
+                if error:
+                    errors += 1
+                    exitcode = 1
+                    utils.error(
+                        "The module '{}' does not provide a proper `versionadded` version: {!r} is not valid.",
+                        path.relative_to(CODE_DIR),
+                        error,
+                    )
+
+            if not str(path).startswith(SALT_INTERNAL_LOADERS_PATHS):
+                # No further docstrings checks are needed
+                continue
+
+            # We're dealing with a salt loader module
+            if funcdef.name.startswith("_"):
+                # We're not interested in internal functions
+                continue
+
+            if not docstring:
+                errors += 1
+                exitcode = 1
+                utils.error(
+                    "The function {!r} on '{}' does not have a docstring",
+                    funcdef.name,
+                    path.relative_to(CODE_DIR),
+                )
+                continue
+
+            try:
+                relpath = path.relative_to(SALT_MODULES_PATH)
+                if str(relpath.parent) != ".":
+                    # We don't want to check nested packages
+                    continue
+                # But this is a module under salt/modules, let's check
+                # the CLI examples
+            except ValueError:
+                # We're not checking CLI examples in any other salt loader modules
+                continue
+
+            if _check_cli_example_present(docstring) is False:
+                errors += 1
+                exitcode = 1
+                utils.error(
+                    "The function {!r} on '{}' does not have a 'CLI Example:' in it's docstring",
+                    funcdef.name,
+                    path.relative_to(CODE_DIR),
+                )
+                continue
+
+            if check_proper_formatting is False:
+                continue
+
+            # By now we now this function has a docstring and it has a CLI Example section
+            # Let's now check if it's properly formatted
+            if _check_cli_example_proper_formatting(docstring) is False:
+                errors += 1
+                exitcode = 1
+                utils.error(
+                    "The function {!r} on '{}' does not have a proper 'CLI Example:' section in "
+                    "it's docstring. The proper format is:\n"
+                    "CLI Example:\n"
+                    "\n"
+                    ".. code-block:: bash\n"
+                    "\n"
+                    "    salt '*' <insert example here>\n",
+                    funcdef.name,
+                    path.relative_to(CODE_DIR),
+                )
+                continue
+
+    if exitcode:
+        utils.error("Found {} errors", errors)
+    utils.exit_invoke(exitcode)
+
+
+def _check_valid_versionadded(docstring):
+    versionadded_regex = re.compile("versionadded::(?P<version>.*)")
+    for match in versionadded_regex.finditer(docstring):
+        version = match.group("version")
+        try:
+            parsed = SaltStackVersion.parse(version.strip())
+        except ValueError:
+            return version.strip()
+    return False
+
+
+def _check_cli_example_present(docstring):
+    cli_example_regex = re.compile(r"CLI Example(?:s)?:")
+    return cli_example_regex.search(docstring) is not None
+
+
+def _check_cli_example_proper_formatting(docstring):
+    good_cli_example_regex = re.compile(
+        r"CLI Example(?:s)?:\n\n.. code-block:: bash\n\n    salt (.*) '*'", re.MULTILINE
+    )
+    return good_cli_example_regex.search(docstring) is not None

tasks/loader.py

@@ -7,9 +7,9 @@
 
 import ast
 import pathlib
-import re
 
 from invoke import task  # pylint: disable=3rd-party-module-not-gated
+from salt.loader import SALT_INTERNAL_LOADERS_PATHS
 from tasks import utils
 
 CODE_DIR = pathlib.Path(__file__).resolve().parent.parent
@@ -43,53 +43,13 @@ def check_virtual(ctx, files, enforce_virtualname=False):
 
     _files = [path.resolve() for path in _files]
 
-    salt_loaders = (
-        CODE_DIR / "salt" / "modules",
-        CODE_DIR / "salt" / "metaproxy",
-        CODE_DIR / "salt" / "matchers",
-        CODE_DIR / "salt" / "engines",
-        CODE_DIR / "salt" / "proxy",
-        CODE_DIR / "salt" / "returners",
-        CODE_DIR / "salt" / "utils",
-        CODE_DIR / "salt" / "pillar",
-        CODE_DIR / "salt" / "tops",
-        CODE_DIR / "salt" / "wheel",
-        CODE_DIR / "salt" / "output",
-        CODE_DIR / "salt" / "serializers",
-        CODE_DIR / "salt" / "tokens",
-        CODE_DIR / "salt" / "auth",
-        CODE_DIR / "salt" / "fileserver",
-        CODE_DIR / "salt" / "roster",
-        CODE_DIR / "salt" / "thorium",
-        CODE_DIR / "salt" / "states",
-        CODE_DIR / "salt" / "beacons",
-        CODE_DIR / "salt" / "log" / "handlers",
-        CODE_DIR / "salt" / "client" / "ssh",
-        CODE_DIR / "salt" / "renderers",
-        CODE_DIR / "salt" / "grains",
-        CODE_DIR / "salt" / "runners",
-        CODE_DIR / "salt" / "queues",
-        CODE_DIR / "salt" / "sdb",
-        CODE_DIR / "salt" / "spm" / "pkgdb",
-        CODE_DIR / "salt" / "spm" / "pkgfiles",
-        CODE_DIR / "salt" / "cloud" / "clouds",
-        CODE_DIR / "salt" / "netapi",
-        CODE_DIR / "salt" / "executors",
-        CODE_DIR / "salt" / "cache",
-    )
-
-    # This is just internal task checking
-    for loader in salt_loaders:
-        if not pathlib.Path(loader).is_dir():
-            utils.error("The {} path is not a directory", loader)
-
     errors = 0
     exitcode = 0
     for path in _files:
         strpath = str(path)
         if path.name == "__init__.py":
             continue
-        for loader in salt_loaders:
+        for loader in SALT_INTERNAL_LOADERS_PATHS:
             try:
                 path.relative_to(loader)
                 break
@@ -140,102 +100,3 @@ def check_virtual(ctx, files, enforce_virtualname=False):
     if exitcode:
         utils.error("Found {} errors", errors)
     utils.exit_invoke(exitcode)
-
-
-@task(iterable=["files"], positional=["files"])
-def check_module_docstrings(ctx, files, check_proper_formatting=False):
-    # CD into Salt's repo root directory
-    ctx.cd(CODE_DIR)
-
-    # Unfortunately invoke does not support nargs.
-    # We migth have been passed --files="foo.py bar.py"
-    # Turn that into a list of paths
-    _files = []
-    for path in files:
-        if not path:
-            continue
-        _files.extend(path.split())
-    if not _files:
-        _files = (SALT_CODE_DIR / "modules").rglob("*.py")
-    else:
-        _files = [pathlib.Path(fname) for fname in _files]
-
-    _files = [path.resolve() for path in _files]
-
-    _files_to_check = []
-    salt_modules_path = SALT_CODE_DIR / "modules"
-    for path in _files:
-        try:
-            relpath = path.relative_to(salt_modules_path)
-            if str(relpath.parent) != ".":
-                # We don't want to check nested packages
-                continue
-            _files_to_check.append(path)
-        except ValueError:
-            # This is not a salt/modules/*.py module. Carry on.
-            continue
-
-    errors = 0
-    exitcode = 0
-    cli_example_regex = re.compile(r"CLI Example(?:s)?:")
-    good_cli_example_regex = re.compile(
-        r"CLI Example(?:s)?:\n\n.. code-block:: bash\n\n    salt (.*) '*'", re.MULTILINE
-    )
-    for path in _files_to_check:
-        if path.name == "__init__.py":
-            continue
-        module = ast.parse(path.read_text(), filename=str(path))
-        for funcdef in [
-            node for node in module.body if isinstance(node, ast.FunctionDef)
-        ]:
-            if funcdef.name in ("__virtual__", "__init__"):
-                # We're not interested in these
-                continue
-            if funcdef.name.startswith("_"):
-                # We're not interested in private functions
-                continue
-            docstring = ast.get_docstring(funcdef)
-            if not docstring:
-                errors += 1
-                exitcode = 1
-                utils.error(
-                    "The function {!r} on {} does not have a docstring",
-                    funcdef.name,
-                    path.relative_to(CODE_DIR),
-                )
-                continue
-
-            if not cli_example_regex.search(docstring):
-                errors += 1
-                exitcode = 1
-                utils.error(
-                    "The function {!r} on {} does not have a 'CLI Example:' in it's docstring",
-                    funcdef.name,
-                    path.relative_to(CODE_DIR),
-                )
-                continue
-
-            if check_proper_formatting is False:
-                continue
-
-            # By now we now this function has a docstring and it has a CLI Example section
-            # Let's now check if it's properly formatted
-            if not good_cli_example_regex.search(docstring):
-                errors += 1
-                exitcode = 1
-                utils.error(
-                    "The function {!r} on {} does not have a proper 'CLI Example:' section in "
-                    "it's docstring. The proper format is:\n"
-                    "CLI Example:\n"
-                    "\n"
-                    ".. code-block:: bash\n"
-                    "\n"
-                    "    salt '*' <insert example here>\n",
-                    funcdef.name,
-                    path.relative_to(CODE_DIR),
-                )
-                continue
-
-    if exitcode:
-        utils.error("Found {} errors", errors)
-    utils.exit_invoke(exitcode)