From 231a72061cf6a7a81deea34bde59bf80939420ab Mon Sep 17 00:00:00 2001 From: Ajinkya Udgirkar Date: Thu, 5 Oct 2023 13:48:57 +0530 Subject: [PATCH 1/3] Add play level autofix for key-order rule --- .github/workflows/tox.yml | 2 +- .../transform-key-order-play.transformed.yml | 10 +++++ .../playbooks/transform-key-order-play.yml | 10 +++++ src/ansiblelint/rules/key_order.py | 37 ++++++++++++++++--- src/ansiblelint/schemas/__store__.json | 2 +- test/test_transformer.py | 6 +++ 6 files changed, 59 insertions(+), 8 deletions(-) create mode 100644 examples/playbooks/transform-key-order-play.transformed.yml create mode 100644 examples/playbooks/transform-key-order-play.yml diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml index 74a036f202..fc64dd5b07 100644 --- a/.github/workflows/tox.yml +++ b/.github/workflows/tox.yml @@ -70,7 +70,7 @@ jobs: env: # Number of expected test passes, safety measure for accidental skip of # tests. Update value if you add/remove tests. - PYTEST_REQPASS: 831 + PYTEST_REQPASS: 832 steps: - uses: actions/checkout@v4 with: diff --git a/examples/playbooks/transform-key-order-play.transformed.yml b/examples/playbooks/transform-key-order-play.transformed.yml new file mode 100644 index 0000000000..030364d8a7 --- /dev/null +++ b/examples/playbooks/transform-key-order-play.transformed.yml @@ -0,0 +1,10 @@ +--- +- name: This is a playbook # <-- name key should be the first one + hosts: localhost + tasks: + - name: A block + when: true + block: + - name: Display a message + ansible.builtin.debug: + msg: Hello world! diff --git a/examples/playbooks/transform-key-order-play.yml b/examples/playbooks/transform-key-order-play.yml new file mode 100644 index 0000000000..e61920dca6 --- /dev/null +++ b/examples/playbooks/transform-key-order-play.yml @@ -0,0 +1,10 @@ +--- +- hosts: localhost + name: This is a playbook # <-- name key should be the first one + tasks: + - name: A block + when: true + block: + - name: Display a message + ansible.builtin.debug: + msg: Hello world! diff --git a/src/ansiblelint/rules/key_order.py b/src/ansiblelint/rules/key_order.py index 16b058914c..4c7aef553e 100644 --- a/src/ansiblelint/rules/key_order.py +++ b/src/ansiblelint/rules/key_order.py @@ -4,15 +4,15 @@ import functools import sys from dataclasses import dataclass -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any -from ansiblelint.errors import RuleMatchTransformMeta +from ansiblelint.constants import ANNOTATION_KEYS, LINE_NUMBER_KEY +from ansiblelint.errors import MatchError, RuleMatchTransformMeta from ansiblelint.rules import AnsibleLintRule, TransformMixin if TYPE_CHECKING: from ruamel.yaml.comments import CommentedMap, CommentedSeq - from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable from ansiblelint.utils import Task @@ -77,6 +77,25 @@ class KeyOrderRule(AnsibleLintRule, TransformMixin): "key-order[task]": "You can improve the task key order", } + def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + """Return matches found for a specific play (entry in playbook).""" + result: list[MatchError] = [] + if file.kind != "playbook": + return result + keys = [str(key) for key, val in data.items() if key not in ANNOTATION_KEYS] + sorted_keys = sorted(keys, key=functools.cmp_to_key(task_property_sorter)) + if keys != sorted_keys: + result.append( + self.create_matcherror( + f"You can improve the play key order to: {', '.join(sorted_keys)}", + filename=file, + tag=f"{self.id}[play]", + lineno=data[LINE_NUMBER_KEY], + transform_meta=KeyOrderTMeta(fixed=tuple(sorted_keys)), + ), + ) + return result + def matchtask( self, task: Task, @@ -103,9 +122,15 @@ def transform( lintable: Lintable, data: CommentedMap | CommentedSeq | str, ) -> None: - if match.tag == "key-order[task]": - if not isinstance(match.transform_meta, KeyOrderTMeta): - return + if not isinstance(match.transform_meta, KeyOrderTMeta): + return + + if match.tag == f"{self.id}[play]": + play = self.seek(match.yaml_path, data) + for key in match.transform_meta.fixed: + play[key] = play.pop(key) + match.fixed = True + if match.tag == f"{self.id}[task]": task = self.seek(match.yaml_path, data) for key in match.transform_meta.fixed: task[key] = task.pop(key) diff --git a/src/ansiblelint/schemas/__store__.json b/src/ansiblelint/schemas/__store__.json index 7d7a01dd0a..d2d0c84585 100644 --- a/src/ansiblelint/schemas/__store__.json +++ b/src/ansiblelint/schemas/__store__.json @@ -24,7 +24,7 @@ "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/inventory.json" }, "meta": { - "etag": "d1f66f20b45ecb814d9be900f01451d8035059b312de4a19edc64e29811a7f39", + "etag": "80d83d5c61044aa67496ea22c74aef08b0216c20e318400d3c939927a2761d51", "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/meta.json" }, "meta-runtime": { diff --git a/test/test_transformer.py b/test/test_transformer.py index f06302484f..fbaf6ae0cb 100644 --- a/test/test_transformer.py +++ b/test/test_transformer.py @@ -125,6 +125,12 @@ def fixture_runner_result( True, id="partial_become", ), + pytest.param( + "examples/playbooks/transform-key-order-play.yml", + 1, + True, + id="key_order_play_transform", + ), ), ) def test_transformer( # pylint: disable=too-many-arguments, too-many-locals From 613fad19866c46df1097551fdee35b0e0157d30f Mon Sep 17 00:00:00 2001 From: Ajinkya Udgirkar Date: Mon, 16 Oct 2023 13:50:35 +0530 Subject: [PATCH 2/3] Hide stacktrace for invalid playbook --- .github/workflows/tox.yml | 2 +- examples/playbooks/invalid-transform.yml | 10 ++++++++++ src/ansiblelint/yaml_utils.py | 4 ++++ test/test_transformer.py | 7 +++++++ 4 files changed, 22 insertions(+), 1 deletion(-) create mode 100644 examples/playbooks/invalid-transform.yml diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml index 45edc4dc32..c82a865eaf 100644 --- a/.github/workflows/tox.yml +++ b/.github/workflows/tox.yml @@ -70,7 +70,7 @@ jobs: env: # Number of expected test passes, safety measure for accidental skip of # tests. Update value if you add/remove tests. - PYTEST_REQPASS: 832 + PYTEST_REQPASS: 833 steps: - uses: actions/checkout@v4 with: diff --git a/examples/playbooks/invalid-transform.yml b/examples/playbooks/invalid-transform.yml new file mode 100644 index 0000000000..92c45c6a38 --- /dev/null +++ b/examples/playbooks/invalid-transform.yml @@ -0,0 +1,10 @@ +--- +- name: Test + hosts: localhost + gather_facts: false + + tasks: + - name: Print hello message + ansible.builtin.debug: + msg: "Hello!" + register: vm_output \ No newline at end of file diff --git a/src/ansiblelint/yaml_utils.py b/src/ansiblelint/yaml_utils.py index 851f96ca13..98318ec94d 100644 --- a/src/ansiblelint/yaml_utils.py +++ b/src/ansiblelint/yaml_utils.py @@ -21,6 +21,7 @@ # Module 'ruamel.yaml' does not explicitly export attribute 'YAML'; implicit reexport disabled # To make the type checkers happy, we import from ruamel.yaml.main instead. from ruamel.yaml.main import YAML +from ruamel.yaml.parser import ParserError from ruamel.yaml.scalarint import ScalarInt from yamllint.config import YamlLintConfig @@ -953,6 +954,9 @@ def loads(self, stream: str) -> Any: data = self.load(stream=text) except ComposerError: data = self.load_all(stream=text) + except ParserError: + data = None + _logger.error("Invalid yaml, verify the file contents and try again.") if preamble_comment is not None and isinstance( data, (CommentedMap, CommentedSeq), diff --git a/test/test_transformer.py b/test/test_transformer.py index 525e37278b..759f9cbf80 100644 --- a/test/test_transformer.py +++ b/test/test_transformer.py @@ -162,6 +162,13 @@ def fixture_runner_result( False, id="github-workflow", ), + pytest.param( + "examples/playbooks/invalid-transform.yml", + 1, + False, + True, + id="invalid_transform", + ), ), ) @mock.patch.dict(os.environ, {"ANSIBLE_LINT_WRITE_TMP": "1"}, clear=True) From df406c9abb40439b05c87c78415b9b21562c2d29 Mon Sep 17 00:00:00 2001 From: Ajinkya Udgirkar Date: Mon, 16 Oct 2023 16:35:50 +0530 Subject: [PATCH 3/3] Fix pre-commit issues --- .pre-commit-config.yaml | 1 + examples/playbooks/invalid-transform.yml | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index ac49de5e31..5f76c92059 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -44,6 +44,7 @@ repos: examples/other/some.j2.yaml| examples/playbooks/collections/.*| examples/playbooks/example.yml| + examples/playbooks/invalid-transform.yml| examples/playbooks/multiline-brackets.*| examples/playbooks/templates/not-valid.yaml| examples/playbooks/vars/empty.transformed.yml| diff --git a/examples/playbooks/invalid-transform.yml b/examples/playbooks/invalid-transform.yml index 92c45c6a38..3a1d50a54f 100644 --- a/examples/playbooks/invalid-transform.yml +++ b/examples/playbooks/invalid-transform.yml @@ -1,3 +1,4 @@ +# yamllint disable-file --- - name: Test hosts: localhost @@ -6,5 +7,5 @@ tasks: - name: Print hello message ansible.builtin.debug: - msg: "Hello!" - register: vm_output \ No newline at end of file + msg: "Hello!" + register: vm_output