From 1aeb6f4728300bea8667a3ca7ba38a727c5757dc Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Sat, 22 Apr 2023 13:07:34 +0100 Subject: [PATCH] Capture python warnings and report them as matches This change introduces a new way to generate warnings in linter, one that uses the warnings support in Python. The benefit is that this allows use to generate warnings from anywhere inside the code without having to pass them from function to function. For start we will be using this for internal rules, like ability to report outdated noqa comments. --- .github/workflows/tox.yml | 2 +- .pre-commit-config.yaml | 4 +-- examples/playbooks/capture-warning.yml | 8 +++++ src/ansiblelint/errors.py | 14 ++++++++ src/ansiblelint/rules/__init__.py | 5 ++- src/ansiblelint/rules/jinja.py | 5 ++- src/ansiblelint/rules/var_naming.py | 10 ++++-- src/ansiblelint/runner.py | 50 ++++++++++++++++++++++++-- src/ansiblelint/skip_utils.py | 31 ++++++++++++---- test/test_skiputils.py | 20 ++++++++++- 10 files changed, 131 insertions(+), 18 deletions(-) create mode 100644 examples/playbooks/capture-warning.yml diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml index 5ea30eb2947..6c9be827715 100644 --- a/.github/workflows/tox.yml +++ b/.github/workflows/tox.yml @@ -59,7 +59,7 @@ jobs: WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:GITHUB_STEP_SUMMARY # Number of expected test passes, safety measure for accidental skip of # tests. Update value if you add/remove tests. - PYTEST_REQPASS: 794 + PYTEST_REQPASS: 795 steps: - name: Activate WSL1 if: "contains(matrix.shell, 'wsl')" diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index dc85c9dae47..84fc1e36672 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -66,7 +66,7 @@ repos: args: [--relative, --no-progress, --no-summary] name: Spell check with cspell - repo: https://github.com/python-jsonschema/check-jsonschema - rev: 0.22.0 + rev: 0.23.0 hooks: - id: check-github-workflows - repo: https://github.com/pre-commit/pre-commit-hooks.git @@ -126,7 +126,7 @@ repos: types: [file, yaml] entry: yamllint --strict - repo: https://github.com/charliermarsh/ruff-pre-commit - rev: "v0.0.264" + rev: "v0.0.265" hooks: - id: ruff args: [--fix, --exit-non-zero-on-fix] diff --git a/examples/playbooks/capture-warning.yml b/examples/playbooks/capture-warning.yml new file mode 100644 index 00000000000..ee206901cd3 --- /dev/null +++ b/examples/playbooks/capture-warning.yml @@ -0,0 +1,8 @@ +--- +- name: Fixture to generate a warning + hosts: localhost + tasks: + - name: Generate a warning + ansible.builtin.debug: + msg: "This is a warning" + when: "{{ false }}" # noqa: 102 jinja[spacing] diff --git a/src/ansiblelint/errors.py b/src/ansiblelint/errors.py index 99af2b302f2..fd19055423a 100644 --- a/src/ansiblelint/errors.py +++ b/src/ansiblelint/errors.py @@ -13,6 +13,20 @@ from ansiblelint.utils import Task +class LintWarning(Warning): + """Used by linter.""" + + +@dataclass +class WarnSource: + """Container for warning information, so we can later create a MatchError from it.""" + + filename: Lintable + lineno: int + tag: str + message: str | None = None + + class StrictModeError(RuntimeError): """Raise when we encounter a warning in strict mode.""" diff --git a/src/ansiblelint/rules/__init__.py b/src/ansiblelint/rules/__init__.py index 9083dbd749b..7f65628eaa2 100644 --- a/src/ansiblelint/rules/__init__.py +++ b/src/ansiblelint/rules/__init__.py @@ -119,7 +119,10 @@ def matchlines(self, file: Lintable) -> list[MatchError]: if line.lstrip().startswith("#"): continue - rule_id_list = ansiblelint.skip_utils.get_rule_skips_from_line(line) + rule_id_list = ansiblelint.skip_utils.get_rule_skips_from_line( + line, + lintable=file, + ) if self.id in rule_id_list: continue diff --git a/src/ansiblelint/rules/jinja.py b/src/ansiblelint/rules/jinja.py index cf0c01b3aae..7aa6e7b1eef 100644 --- a/src/ansiblelint/rules/jinja.py +++ b/src/ansiblelint/rules/jinja.py @@ -203,7 +203,10 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: lines = file.content.splitlines() for match in raw_results: # lineno starts with 1, not zero - skip_list = get_rule_skips_from_line(lines[match.lineno - 1]) + skip_list = get_rule_skips_from_line( + line=lines[match.lineno - 1], + lintable=file, + ) if match.rule.id not in skip_list and match.tag not in skip_list: results.append(match) else: diff --git a/src/ansiblelint/rules/var_naming.py b/src/ansiblelint/rules/var_naming.py index ec7cbd04250..f9f969c40f6 100644 --- a/src/ansiblelint/rules/var_naming.py +++ b/src/ansiblelint/rules/var_naming.py @@ -95,7 +95,10 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: lines = file.content.splitlines() for match in raw_results: # lineno starts with 1, not zero - skip_list = get_rule_skips_from_line(lines[match.lineno - 1]) + skip_list = get_rule_skips_from_line( + line=lines[match.lineno - 1], + lintable=file, + ) if match.rule.id not in skip_list and match.tag not in skip_list: results.append(match) @@ -175,7 +178,10 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: lines = file.content.splitlines() for match in raw_results: # lineno starts with 1, not zero - skip_list = get_rule_skips_from_line(lines[match.lineno - 1]) + skip_list = get_rule_skips_from_line( + line=lines[match.lineno - 1], + lintable=file, + ) if match.rule.id not in skip_list and match.tag not in skip_list: results.append(match) else: diff --git a/src/ansiblelint/runner.py b/src/ansiblelint/runner.py index 8851ebd6de7..74caa93a247 100644 --- a/src/ansiblelint/runner.py +++ b/src/ansiblelint/runner.py @@ -5,15 +5,16 @@ import multiprocessing import multiprocessing.pool import os +import warnings from dataclasses import dataclass from fnmatch import fnmatch from typing import TYPE_CHECKING, Any import ansiblelint.skip_utils import ansiblelint.utils -from ansiblelint._internal.rules import LoadingFailureRule +from ansiblelint._internal.rules import LoadingFailureRule, WarningRule from ansiblelint.constants import States -from ansiblelint.errors import MatchError +from ansiblelint.errors import LintWarning, MatchError, WarnSource from ansiblelint.file_utils import Lintable, expand_dirs_in_lintables from ansiblelint.rules.syntax_check import AnsibleSyntaxCheckRule @@ -112,8 +113,51 @@ def is_excluded(self, lintable: Lintable) -> bool: for path in self.exclude_paths ) - def run(self) -> list[MatchError]: # noqa: C901 + def run(self) -> list[MatchError]: """Execute the linting process.""" + matches: list[MatchError] = [] + with warnings.catch_warnings(record=True) as captured_warnings: + warnings.simplefilter("always") + matches = self._run() + for warn in captured_warnings: + # For the moment we are ignoring deprecation warnings as Ansible + # modules outside current content can generate them and user + # might not be able to do anything about them. + if warn.category is DeprecationWarning: + continue + if warn.category is LintWarning: + filename: None | Lintable = None + if isinstance(warn.source, WarnSource): + match = MatchError( + message=warn.source.message or warn.category.__name__, + rule=WarningRule(), + filename=warn.source.filename.filename, + tag=warn.source.tag, + lineno=warn.source.lineno, + ) + else: + filename = warn.source + # breakpoint() + match = MatchError( + message=warn.message + if isinstance(warn.message, str) + else "?", + rule=WarningRule(), + filename=str(filename), + ) + matches.append(match) + continue + _logger.warning( + "%s:%s %s %s", + warn.filename, + warn.lineno or 1, + warn.category.__name__, + warn.message, + ) + return matches + + def _run(self) -> list[MatchError]: # noqa: C901 + """Run the linting (inner loop).""" files: list[Lintable] = [] matches: list[MatchError] = [] diff --git a/src/ansiblelint/skip_utils.py b/src/ansiblelint/skip_utils.py index 006e5980ba5..826e38ad9f0 100644 --- a/src/ansiblelint/skip_utils.py +++ b/src/ansiblelint/skip_utils.py @@ -24,6 +24,7 @@ import collections.abc import logging import re +import warnings from functools import cache from itertools import product from typing import TYPE_CHECKING, Any @@ -41,6 +42,7 @@ RENAMED_TAGS, SKIPPED_RULES_KEY, ) +from ansiblelint.errors import LintWarning, WarnSource if TYPE_CHECKING: from collections.abc import Generator, Sequence @@ -60,7 +62,11 @@ # ansible.parsing.yaml.objects.AnsibleSequence -def get_rule_skips_from_line(line: str) -> list[str]: +def get_rule_skips_from_line( + line: str, + lintable: Lintable, + lineno: int = 1, +) -> list[str]: """Return list of rule ids skipped via comment on the line of yaml.""" _before_noqa, _noqa_marker, noqa_text = line.partition("# noqa") @@ -69,10 +75,17 @@ def get_rule_skips_from_line(line: str) -> list[str]: if v in RENAMED_TAGS: tag = RENAMED_TAGS[v] if v not in _found_deprecated_tags: - _logger.warning( - "Replaced outdated tag '%s' with '%s', replace it to avoid future regressions", - v, - tag, + msg = f"Replaced outdated tag '{v}' with '{tag}', replace it to avoid future errors" + warnings.warn( + message=msg, + category=LintWarning, + source=WarnSource( + filename=lintable, + lineno=lineno, + tag="warning[outdated-tag]", + message=msg, + ), + stacklevel=0, ) _found_deprecated_tags.add(v) v = tag @@ -257,7 +270,11 @@ def traverse_yaml(obj: Any) -> None: # noqa: C901 if _noqa_comment_re.match(comment_str): line = v.start_mark.line + 1 # ruamel line numbers start at 0 lintable.line_skips[line].update( - get_rule_skips_from_line(comment_str.strip()), + get_rule_skips_from_line( + comment_str.strip(), + lintable=lintable, + lineno=line, + ), ) yaml_comment_obj_strings.append(str(obj.ca.items)) if isinstance(obj, dict): @@ -277,7 +294,7 @@ def traverse_yaml(obj: Any) -> None: # noqa: C901 rule_id_list = [] for comment_obj_str in yaml_comment_obj_strings: for line in comment_obj_str.split(r"\n"): - rule_id_list.extend(get_rule_skips_from_line(line)) + rule_id_list.extend(get_rule_skips_from_line(line, lintable=lintable)) return [normalize_tag(tag) for tag in rule_id_list] diff --git a/test/test_skiputils.py b/test/test_skiputils.py index eab7c68e21e..7e736e73a43 100644 --- a/test/test_skiputils.py +++ b/test/test_skiputils.py @@ -8,6 +8,7 @@ from ansiblelint.constants import SKIPPED_RULES_KEY from ansiblelint.file_utils import Lintable +from ansiblelint.runner import Runner from ansiblelint.skip_utils import ( append_skipped_rules, get_rule_skips_from_line, @@ -17,6 +18,7 @@ if TYPE_CHECKING: from ansible.parsing.yaml.objects import AnsibleBaseYAMLObject + from ansiblelint.rules import RulesCollection from ansiblelint.testing import RunFromText PLAYBOOK_WITH_NOQA = """\ @@ -43,7 +45,7 @@ ) def test_get_rule_skips_from_line(line: str, expected: str) -> None: """Validate get_rule_skips_from_line.""" - v = get_rule_skips_from_line(line) + v = get_rule_skips_from_line(line, lintable=Lintable("")) assert v == [expected] @@ -232,3 +234,19 @@ def test_append_skipped_rules( def test_is_nested_task(task: dict[str, Any], expected: bool) -> None: """Test is_nested_task() returns expected bool.""" assert is_nested_task(task) == expected + + +def test_capture_warning_outdated_tag( + default_rules_collection: RulesCollection, +) -> None: + """Test that exclude paths do work.""" + runner = Runner( + "examples/playbooks/capture-warning.yml", + rules=default_rules_collection, + ) + + matches = runner.run() + assert len(matches) == 1 + assert matches[0].rule.id == "warning" + assert matches[0].tag == "warning[outdated-tag]" + assert matches[0].lineno == 8