From fba36acf33f15a8b9547719e1a7c6ad2b7a0ad7d Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Thu, 6 Oct 2022 17:57:07 +0100 Subject: [PATCH] Refactor the rendering of errors and warnings (#2566) - Display warnings different than errors on all output formats - Document YAML specific errors - Display warning using yellow AND adding (warning) suffix - Corrected cases where warning was determined incorrectly - Display only the detailed rule tag on matches (reduce clutter) - Capitalize YAML error messages Fixes: #1670 --- src/ansiblelint/color.py | 5 +- src/ansiblelint/errors.py | 8 ++++ src/ansiblelint/formatters/__init__.py | 63 +++++++++++--------------- src/ansiblelint/rules/yaml.md | 16 +++++++ src/ansiblelint/rules/yaml_rule.py | 9 ++-- test/test_cli_role_paths.py | 2 +- test/test_utils.py | 2 +- 7 files changed, 59 insertions(+), 46 deletions(-) diff --git a/src/ansiblelint/color.py b/src/ansiblelint/color.py index 8b3847fa14..6670e500e3 100644 --- a/src/ansiblelint/color.py +++ b/src/ansiblelint/color.py @@ -54,11 +54,10 @@ _theme = Theme( { "info": "cyan", - "warning": "dim yellow", + "warning": "yellow", "danger": "bold red", "title": "yellow", - "error_code": "bright_red", - "error_title": "red", + "error": "bright_red", "filename": "blue", } ) diff --git a/src/ansiblelint/errors.py b/src/ansiblelint/errors.py index 29e9b17f8e..a50915a105 100644 --- a/src/ansiblelint/errors.py +++ b/src/ansiblelint/errors.py @@ -5,6 +5,7 @@ from typing import Any from ansiblelint._internal.rules import BaseRule, RuntimeErrorRule +from ansiblelint.config import options from ansiblelint.file_utils import Lintable, normpath @@ -87,6 +88,13 @@ def __init__( # True when a transform has resolved this MatchError self.fixed = False + @functools.cached_property + def level(self) -> str: + """Return the level of the rule: error, warning or notice.""" + if {self.tag, self.rule.id, *self.rule.tags}.isdisjoint(options.warn_list): + return "error" + return "warning" + def __repr__(self) -> str: """Return a MatchError instance representation.""" formatstr = "[{0}] ({1}) matched {2}:{3} {4}" diff --git a/src/ansiblelint/formatters/__init__.py b/src/ansiblelint/formatters/__init__.py index 96a6f9dd7c..e0a595a03f 100644 --- a/src/ansiblelint/formatters/__init__.py +++ b/src/ansiblelint/formatters/__init__.py @@ -58,9 +58,9 @@ class Formatter(BaseFormatter): # type: ignore def format(self, match: MatchError) -> str: _id = getattr(match.rule, "id", "000") - result = f"[error_code][link={match.rule.url}]{_id}[/link][/][dim]:[/] [error_title]{self.escape(match.message)}[/]" - if match.tag: - result += f" [dim][error_code]({self.escape(match.tag)})[/][/]" + result = f"[{match.level}][bold][link={match.rule.url}]{self.escape(match.tag)}[/link][/][/][dim]:[/] [{match.level}]{self.escape(match.message)}[/]" + if match.level != "error": + result += f" [dim][{match.level}]({match.level})[/][/]" result += ( "\n" f"[filename]{self._format_path(match.filename or '')}[/]:{match.position}" @@ -76,7 +76,7 @@ class QuietFormatter(BaseFormatter[Any]): def format(self, match: MatchError) -> str: return ( - f"[error_code]{match.rule.id}[/] " + f"[{match.level}]{match.rule.id}[/] " f"[filename]{self._format_path(match.filename or '')}[/]:{match.position}" ) @@ -86,20 +86,20 @@ class ParseableFormatter(BaseFormatter[Any]): def format(self, match: MatchError) -> str: result = ( - f"[filename]{self._format_path(match.filename or '')}[/]:{match.position}: " - f"[error_code]{match.rule.id}[/]" + f"[filename]{self._format_path(match.filename or '')}[/][dim]:{match.position}:[/] " + f"[{match.level}][bold]{self.escape(match.tag)}[/bold]" + f": {match.message}" + if not options.quiet + else "[/]" ) + if match.level != "error": + result += f" [dim][{match.level}]({match.level})[/][/]" - if not options.quiet: - result += f": [dim]{match.message}[/]" - - if match.tag: - result += f" [dim][error_code]({self.escape(match.tag)})[/][/]" return result class AnnotationsFormatter(BaseFormatter): # type: ignore - # https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#setting-a-warning-message + # https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-a-warning-message """Formatter for emitting violations as GitHub Workflow Commands. These commands trigger the GHA Workflow runners platform to post violations @@ -115,10 +115,8 @@ class AnnotationsFormatter(BaseFormatter): # type: ignore def format(self, match: MatchError) -> str: """Prepare a match instance for reporting as a GitHub Actions annotation.""" - level = self._severity_to_level(match.rule.severity) file_path = self._format_path(match.filename or "") line_num = match.linenumber - rule_id = match.rule.id severity = match.rule.severity violation_details = self.escape(match.message) if match.column: @@ -126,19 +124,10 @@ def format(self, match: MatchError) -> str: else: col = "" return ( - f"::{level} file={file_path},line={line_num}{col},severity={severity}" - f"::{rule_id} {violation_details}" + f"::{match.level} file={file_path},line={line_num}{col},severity={severity},title={match.tag}" + f"::{violation_details}" ) - @staticmethod - def _severity_to_level(severity: str) -> str: - if severity in ["VERY_LOW", "LOW"]: - return "warning" - if severity in ["INFO"]: - return "debug" - # ['MEDIUM', 'HIGH', 'VERY_HIGH'] or anything else - return "error" - class CodeclimateJSONFormatter(BaseFormatter[Any]): """Formatter for emitting violations in Codeclimate JSON report format. @@ -161,7 +150,7 @@ def format_result(self, matches: list[MatchError]) -> str: issue["type"] = "issue" issue["check_name"] = match.tag or match.rule.id # rule-id[subrule-id] issue["categories"] = match.rule.tags - issue["severity"] = self._severity_to_level(match.rule.severity) + issue["severity"] = self._severity_to_level(match) issue["description"] = self.escape(str(match.message)) issue["fingerprint"] = hashlib.sha256( repr(match).encode("utf-8") @@ -184,7 +173,11 @@ def format_result(self, matches: list[MatchError]) -> str: return json.dumps(result) @staticmethod - def _severity_to_level(severity: str) -> str: + def _severity_to_level(match: MatchError) -> str: + if match.level != "error": + return "info" + severity = match.rule.severity + if severity in ["LOW"]: return "minor" if severity in ["MEDIUM"]: @@ -205,7 +198,7 @@ class SarifFormatter(BaseFormatter[Any]): """ BASE_URI_ID = "SRCROOT" - TOOL_NAME = "Ansible-lint" + TOOL_NAME = "ansible-lint" TOOL_URL = "https://github.com/ansible/ansible-lint" SARIF_SCHEMA_VERSION = "2.1.0" SARIF_SCHEMA = ( @@ -267,12 +260,12 @@ def _extract_results( def _to_sarif_rule(self, match: MatchError) -> dict[str, Any]: rule: dict[str, Any] = { "id": match.rule.id, - "name": match.rule.id, + "name": match.tag, "shortDescription": { "text": self.escape(str(match.message)), }, "defaultConfiguration": { - "level": self._to_sarif_level(match.rule.severity), + "level": self._to_sarif_level(match), }, "help": { "text": str(match.rule.description), @@ -311,10 +304,6 @@ def _to_sarif_result(self, match: MatchError) -> dict[str, Any]: return result @staticmethod - def _to_sarif_level(severity: str) -> str: - if severity in ["VERY_HIGH", "HIGH", "MEDIUM"]: - return "error" - if severity in ["LOW"]: - return "warning" - # VERY_LOW, INFO or anything else - return "note" + def _to_sarif_level(match: MatchError) -> str: + # sarif accepts only 4 levels: error, warning, note, none + return match.level diff --git a/src/ansiblelint/rules/yaml.md b/src/ansiblelint/rules/yaml.md index 8f65876999..d53cb7dc3c 100644 --- a/src/ansiblelint/rules/yaml.md +++ b/src/ansiblelint/rules/yaml.md @@ -29,6 +29,22 @@ warn_list: See the [list of yamllint rules](https://yamllint.readthedocs.io/en/stable/rules.html) for more information. +Some of the detailed error codes that you might see are: + +- `yaml[brackets]` - _too few spaces inside empty brackets_, or _too many spaces inside brackets_ +- `yaml[colons]` - _too many spaces before colon_, or _too many spaces after colon_ +- `yaml[commas]` - _too many spaces before comma_, or _too few spaces after comma_ +- `yaml[comments-indentation]` - _Comment not indented like content_ +- `yaml[comments]` - _Too few spaces before comment_, or _Missing starting space in comment_ +- `yaml[document-start]` - _missing document start "---"_ or _found forbidden document start "---"_ +- `yaml[empty-lines]` - _too many blank lines (...> ...)_ +- `yaml[indentation]` - _Wrong indentation: expected ... but found ..._ +- `yaml[key-duplicates]` - _Duplication of key "..." in mapping_ +- `yaml[new-line-at-end-of-file]` - _No new line character at the end of file_ +- `yaml[syntax]` - YAML syntax is broken +- `yaml[trailing-spaces]` - Spaces are found at the end of lines +- `yaml[truthy]` - _Truthy value should be one of ..._ + ## Problematic code ```yaml diff --git a/src/ansiblelint/rules/yaml_rule.py b/src/ansiblelint/rules/yaml_rule.py index 40e264f232..8f35ca8280 100644 --- a/src/ansiblelint/rules/yaml_rule.py +++ b/src/ansiblelint/rules/yaml_rule.py @@ -50,7 +50,8 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: self.severity = "VERY_HIGH" matches.append( self.create_matcherror( - message=problem.desc, + # yamllint does return lower-case sentences + message=problem.desc.capitalize(), linenumber=problem.line, details="", filename=file, @@ -129,9 +130,9 @@ def _fetch_skips(data: Any, collector: dict[int, set[str]]) -> dict[int, set[str "examples/yamllint/invalid.yml", "yaml", [ - 'missing document start "---"', - 'duplication of key "foo" in mapping', - "trailing spaces", + 'Missing document start "---"', + 'Duplication of key "foo" in mapping', + "Trailing spaces", ], ), ( diff --git a/test/test_cli_role_paths.py b/test/test_cli_role_paths.py index df89aa7647..555a2ba81b 100644 --- a/test/test_cli_role_paths.py +++ b/test/test_cli_role_paths.py @@ -169,7 +169,7 @@ def test_run_playbook_github(result: bool, env: dict[str, str]) -> None: result_gh = run_ansible_lint(role_path, cwd=cwd, env=env) expected = ( - "::warning file=examples/playbooks/example.yml,line=44,severity=VERY_LOW::package-latest " + "::error file=examples/playbooks/example.yml,line=44,severity=VERY_LOW,title=package-latest::" "Package installs should not use latest" ) assert (expected in result_gh.stdout) is result diff --git a/test/test_utils.py b/test/test_utils.py index 00e0639f84..4ef5ac6ef4 100644 --- a/test/test_utils.py +++ b/test/test_utils.py @@ -320,7 +320,7 @@ def test_cli_auto_detect(capfd: CaptureFixture[str]) -> None: # An expected rule match from our examples assert ( "examples/playbooks/empty_playbook.yml:1:1: " - "warning: Empty playbook, nothing to do" in out + "warning[empty-playbook]: Empty playbook, nothing to do" in out ) # assures that our ansible-lint config exclude was effective in excluding github files assert "Identified: .github/" not in out