From ba177ad7386ee124ac9e51c6ea6f9f69fb130c03 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Mon, 28 Jun 2021 16:26:33 +0100 Subject: [PATCH] Check loader modules `__virtual__` and `salt/modules/*.py` with pre-commit Fixes #58537 --- .github/workflows/pre-commit.yml | 2 +- .pre-commit-config.yaml | 34 ++++++++++ cicd/jenkins/pr-pre-commit | 2 +- tasks/loader.py | 104 ++++++++++++++++++++++++++++++- 4 files changed, 138 insertions(+), 4 deletions(-) diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index 03526afe4c1..7b7f8640f9d 100644 --- a/.github/workflows/pre-commit.yml +++ b/.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 + SKIP: lint-salt,lint-tests,pyupgrade,remove-import-headers,rstcheck,loader-check-virtual,loader-check-module-docstrings run: | pre-commit run --show-diff-on-failure --color=always --all-files diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index c388e71b87d..91f4676f804 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1215,6 +1215,40 @@ repos: - jinja2 - msgpack + - repo: https://github.com/saltstack/invoke-pre-commit + rev: v1.5.0 + hooks: + - id: invoke + alias: loader-check-virtual + name: Check modules __virtual__ + files: salt/.*\.py$ + exclude: > + (?x)^( + templates/.*| + salt/ext/.*| + )$ + args: + - loader.check-virtual + additional_dependencies: + - blessings + - pyyaml + - distro + - jinja2 + - msgpack + + - repo: https://github.com/saltstack/invoke-pre-commit + rev: v1.5.0 + hooks: + - id: invoke + alias: loader-check-module-docstrings + name: Check salt/modules/*.py docstrings + files: salt/modules/.*\.py$ + args: + - loader.check-module-docstrings + additional_dependencies: + - blessings + - pyyaml + - repo: https://github.com/saltstack/mirrors-nox rev: v2020.8.22 hooks: diff --git a/cicd/jenkins/pr-pre-commit b/cicd/jenkins/pr-pre-commit index 72daae225d3..7fc43344b20 100644 --- a/cicd/jenkins/pr-pre-commit +++ b/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' + pre_commit_skips = 'pyupgrade,remove-import-headers,rstcheck,loader-check-virtual,loader-check-module-docstrings' } runPreCommit( diff --git a/tasks/loader.py b/tasks/loader.py index cef7c598b6c..3aaacb7c232 100644 --- a/tasks/loader.py +++ b/tasks/loader.py @@ -7,6 +7,7 @@ import ast import pathlib +import re from invoke import task # pylint: disable=3rd-party-module-not-gated from tasks import utils @@ -86,7 +87,7 @@ def check_virtual(ctx, files): exitcode = 0 for path in _files: strpath = str(path) - if strpath.endswith("__init__.py"): + if path.name == "__init__.py": continue for loader in salt_loaders: try: @@ -95,7 +96,7 @@ def check_virtual(ctx, files): except ValueError: # Path doesn't start with the loader path, carry on continue - module = ast.parse(path.read_text(), filename=strpath) + module = ast.parse(path.read_text(), filename=str(path)) found_virtual_func = False for funcdef in [ node for node in module.body if isinstance(node, ast.FunctionDef) @@ -139,3 +140,102 @@ def check_virtual(ctx, files): 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 '*' \n", + funcdef.name, + path.relative_to(CODE_DIR), + ) + continue + + if exitcode: + utils.error("Found {} errors", errors) + utils.exit_invoke(exitcode)