From d77f3ef36b2437003eb23c3c6987ee0d8c9527b5 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Wed, 4 Aug 2021 07:35:34 +0100 Subject: [PATCH] Also check for proper `versionadded` versions Refs #59077 --- .github/workflows/pre-commit.yml | 2 +- .pre-commit-config.yaml | 18 +++- cicd/jenkins/pr-pre-commit | 2 +- noxfile.py | 2 + tasks/__init__.py | 5 +- tasks/docstrings.py | 162 +++++++++++++++++++++++++++++++ tasks/loader.py | 143 +-------------------------- 7 files changed, 185 insertions(+), 149 deletions(-) create mode 100644 tasks/docstrings.py diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index 4498ee721b9..6766ecef7a3 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,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 diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 91f4676f804..4fb45ee7a8d 100644 --- a/.pre-commit-config.yaml +++ b/.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 - name: Check salt/modules/*.py docstrings - files: salt/modules/.*\.py$ + alias: check-docstrings + name: Check docstrings + 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 diff --git a/cicd/jenkins/pr-pre-commit b/cicd/jenkins/pr-pre-commit index f3d8101db39..31450adf29c 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,loader-check-module-docstrings' + pre_commit_skips = 'pyupgrade,remove-import-headers,rstcheck,check-docstrings' } runPreCommit( diff --git a/noxfile.py b/noxfile.py index 7914f582a83..5da5986fca7 100644 --- a/noxfile.py +++ b/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 = [] diff --git a/tasks/__init__.py b/tasks/__init__.py index 000fe255d8c..5f5aac88cb8 100644 --- a/tasks/__init__.py +++ b/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") diff --git a/tasks/docstrings.py b/tasks/docstrings.py new file mode 100644 index 00000000000..801d2a2e8ea --- /dev/null +++ b/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 '*' \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.*)") + 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 diff --git a/tasks/loader.py b/tasks/loader.py index 6a720f6ab86..58b4cf61731 100644 --- a/tasks/loader.py +++ b/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 '*' \n", - funcdef.name, - path.relative_to(CODE_DIR), - ) - continue - - if exitcode: - utils.error("Found {} errors", errors) - utils.exit_invoke(exitcode)