Skip to content

Commit

Permalink
Capture python warnings and report them as matches
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ssbarnea committed May 8, 2023
1 parent 546332e commit 9782c0c
Show file tree
Hide file tree
Showing 9 changed files with 129 additions and 16 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/tox.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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')"
Expand Down
8 changes: 8 additions & 0 deletions examples/playbooks/capture-warning.yml
Original file line number Diff line number Diff line change
@@ -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]
14 changes: 14 additions & 0 deletions src/ansiblelint/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."""

Expand Down
5 changes: 4 additions & 1 deletion src/ansiblelint/rules/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
5 changes: 4 additions & 1 deletion src/ansiblelint/rules/jinja.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
10 changes: 8 additions & 2 deletions src/ansiblelint/rules/var_naming.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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:
Expand Down
50 changes: 47 additions & 3 deletions src/ansiblelint/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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] = []

Expand Down
31 changes: 24 additions & 7 deletions src/ansiblelint/skip_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -41,6 +42,7 @@
RENAMED_TAGS,
SKIPPED_RULES_KEY,
)
from ansiblelint.errors import LintWarning, WarnSource

if TYPE_CHECKING:
from collections.abc import Generator, Sequence
Expand All @@ -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")

Expand All @@ -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
Expand Down Expand Up @@ -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):
Expand All @@ -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]

Expand Down
20 changes: 19 additions & 1 deletion test/test_skiputils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 = """\
Expand All @@ -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]


Expand Down Expand Up @@ -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

0 comments on commit 9782c0c

Please sign in to comment.