From 1eb9e1cb754280541383f8d6d885ce5ba4d71a63 Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Fri, 2 Oct 2020 13:52:02 +0100 Subject: [PATCH] Enable progressive mode Adds a new option --progressive that ease adoption of the linter as it will only report failure if it detects an increase number of violations since previous commit. --- .gitignore | 3 + README.rst | 19 ++++++ lib/ansiblelint/__main__.py | 115 ++++++++++++++++++++++++++++------ lib/ansiblelint/cli.py | 6 ++ lib/ansiblelint/errors.py | 1 + lib/ansiblelint/file_utils.py | 12 ++++ 6 files changed, 137 insertions(+), 19 deletions(-) diff --git a/.gitignore b/.gitignore index 5601b98522..c542d2d6c7 100644 --- a/.gitignore +++ b/.gitignore @@ -41,3 +41,6 @@ pip-wheel-metadata # mypy .mypy_cache + +# .cache is used by progressive mode +.cache diff --git a/README.rst b/README.rst index d0868bb8b1..0e061fa9c8 100644 --- a/README.rst +++ b/README.rst @@ -94,6 +94,8 @@ The following is the output from ``ansible-lint --help``, providing an overview -q quieter, although not silent output -p parseable output in the format of pep8 --parseable-severity parseable output including severity of rule + --progressive Return success if it detects a reduction in number of violations compared with + previous git commit. This feature works only on git repository clones. -r RULESDIR Specify custom rule directories. Add -R to keep using embedded rules from /usr/local/lib/python3.8/site-packages/ansiblelint/rules -R Keep default rules when using -r @@ -111,6 +113,23 @@ The following is the output from ``ansible-lint --help``, providing an overview -c CONFIG_FILE Specify configuration file to use. Defaults to ".ansible-lint" --version show program's version number and exit +Progressive mode +---------------- + +In order to ease tool adoption, git users can enable the progressive mode using +``--progressive`` option. This makes the linter return a success even if +some failures are found, as long the total number of violations did not +increase since the previous commit. + +As expected, this mode makes the linter run twice if it finds any violations. +The second run is performed against a temporary git working copy that contains +the previous commit. All the violations that were already present are removed +from the list and the final result is displayed. + +The most notable benefit introduced by this mode it does not prevent merging +new code while allowing developer to address historical violation at his own +speed. + CI/CD ----- diff --git a/lib/ansiblelint/__main__.py b/lib/ansiblelint/__main__.py index cc0d5c3848..ff8d4772fa 100755 --- a/lib/ansiblelint/__main__.py +++ b/lib/ansiblelint/__main__.py @@ -24,13 +24,16 @@ import logging import os import pathlib +import subprocess import sys -from typing import TYPE_CHECKING, List, Set, Type +from contextlib import contextmanager +from typing import TYPE_CHECKING, Any, List, Set, Type, Union from rich.markdown import Markdown from ansiblelint import cli, formatters from ansiblelint.color import console, console_stderr +from ansiblelint.file_utils import cwd from ansiblelint.generate_docs import rules_as_rich, rules_as_rst from ansiblelint.rules import RulesCollection from ansiblelint.runner import Runner @@ -96,8 +99,9 @@ def report_outcome(matches: List["MatchError"], options) -> int: # .ansible-lint warn_list: # or 'skip_list' to silence them completely """ + matches_unignored = [match for match in matches if not match.ignored] - matched_rules = {match.rule.id: match.rule for match in matches} + matched_rules = {match.rule.id: match.rule for match in matches_unignored} for id in sorted(matched_rules.keys()): if {id, *matched_rules[id].tags}.isdisjoint(options.warn_list): msg += f" - '{id}' # {matched_rules[id].shortdesc}\n" @@ -151,6 +155,76 @@ def main() -> int: skip.update(str(s).split(',')) options.skip_list = frozenset(skip) + matches = _get_matches(rules, options) + + # Assure we do not print duplicates and the order is consistent + matches = sorted(set(matches)) + + mark_as_success = False + if matches and options.progressive: + _logger.info( + "Matches found, running again on previous revision in order to detect regressions") + with _previous_revision(): + old_matches = _get_matches(rules, options) + # remove old matches from current list + matches_delta = list(set(matches) - set(old_matches)) + if len(matches_delta) == 0: + _logger.warning( + "Total violations not increased since previous " + "commit, will mark result as success. (%s -> %s)", + len(old_matches), len(matches_delta)) + mark_as_success = True + + ignored = 0 + for match in matches: + # if match is not new, mark is as ignored + if match not in matches_delta: + match.ignored = True + ignored += 1 + if ignored: + _logger.warning( + "Marked %s previously known violation(s) as ignored due to" + " progressive mode.", ignored) + + _render_matches(matches, options, formatter, cwd) + + if matches and not mark_as_success: + return report_outcome(matches, options=options) + else: + return 0 + + +def _render_matches( + matches: List, + options: "Namespace", + formatter: Any, + cwd: Union[str, pathlib.Path]): + + ignored_matches = [match for match in matches if match.ignored] + fatal_matches = [match for match in matches if not match.ignored] + # Displayed ignored matches first + if ignored_matches: + _logger.warning( + "Listing %s violation(s) marked as ignored, likely already known", + len(ignored_matches)) + for match in ignored_matches: + if match.ignored: + print(formatter.format(match, options.colored)) + if fatal_matches: + _logger.warning("Listing %s violation(s) that are fatal", len(fatal_matches)) + for match in fatal_matches: + if not match.ignored: + print(formatter.format(match, options.colored)) + + # If run under GitHub Actions we also want to emit output recognized by it. + if os.getenv('GITHUB_ACTIONS') == 'true' and os.getenv('GITHUB_WORKFLOW'): + formatter = formatters.AnnotationsFormatter(cwd, True) + for match in matches: + print(formatter.format(match)) + + +def _get_matches(rules: RulesCollection, options: "Namespace") -> list: + if not options.playbook: # no args triggers auto-detection mode playbooks = get_playbooks_and_roles(options=options) @@ -164,23 +238,26 @@ def main() -> int: options.skip_list, options.exclude_paths, options.verbosity, checked_files) matches.extend(runner.run()) - - # Assure we do not print duplicates and the order is consistent - matches = sorted(set(matches)) - - for match in matches: - print(formatter.format(match, options.colored)) - - # If run under GitHub Actions we also want to emit output recognized by it. - if os.getenv('GITHUB_ACTIONS') == 'true' and os.getenv('GITHUB_WORKFLOW'): - formatter = formatters.AnnotationsFormatter(cwd, True) - for match in matches: - print(formatter.format(match)) - - if matches: - return report_outcome(matches, options=options) - else: - return 0 + return matches + + +@contextmanager +def _previous_revision(): + """Create or update a temporary workdir containing the previous revision.""" + worktree_dir = ".cache/old-rev" + revision = subprocess.run( + ["git", "rev-parse", "HEAD^1"], + check=True, + universal_newlines=True, + stdout=subprocess.PIPE, + stderr=subprocess.DEVNULL, + ).stdout + p = pathlib.Path(worktree_dir) + p.mkdir(parents=True, exist_ok=True) + os.system(f"git worktree add -f {worktree_dir} 2>/dev/null") + with cwd(worktree_dir): + os.system(f"git checkout {revision}") + yield if __name__ == "__main__": diff --git a/lib/ansiblelint/cli.py b/lib/ansiblelint/cli.py index 6886605992..6b395615dc 100644 --- a/lib/ansiblelint/cli.py +++ b/lib/ansiblelint/cli.py @@ -111,6 +111,12 @@ def get_cli_parser() -> argparse.ArgumentParser: default=False, action='store_true', help="parseable output including severity of rule") + parser.add_argument('--progressive', dest='progressive', + default=False, + action='store_true', + help="Return success if it detects a reduction in number" + " of violations compared with previous git commit. This " + "feature works only in git repositories.") parser.add_argument('-r', action=AbspathArgAction, dest='rulesdir', default=[], type=Path, help="Specify custom rule directories. Add -R " diff --git a/lib/ansiblelint/errors.py b/lib/ansiblelint/errors.py index 540fe4e68a..8569dca09c 100644 --- a/lib/ansiblelint/errors.py +++ b/lib/ansiblelint/errors.py @@ -41,6 +41,7 @@ def __init__( self.details = details self.filename = normpath(filename) if filename else None self.rule = rule + self.ignored = False # If set it will be displayed but not counted as failure def __repr__(self): """Return a MatchError instance representation.""" diff --git a/lib/ansiblelint/file_utils.py b/lib/ansiblelint/file_utils.py index c464191069..f25382f3ff 100644 --- a/lib/ansiblelint/file_utils.py +++ b/lib/ansiblelint/file_utils.py @@ -1,5 +1,6 @@ """Utility functions related to file operations.""" import os +from contextlib import contextmanager def normpath(path) -> str: @@ -11,3 +12,14 @@ def normpath(path) -> str: """ # convertion to string in order to allow receiving non string objects return os.path.relpath(str(path)) + + +@contextmanager +def cwd(path): + """Context manager for temporary changing current working directory.""" + old_pwd = os.getcwd() + os.chdir(path) + try: + yield + finally: + os.chdir(old_pwd)