Skip to content

Commit

Permalink
Enable YAML 1.2 support for non-ansible files (#3809)
Browse files Browse the repository at this point in the history
  • Loading branch information
ssbarnea authored Nov 3, 2023
1 parent 11828f3 commit 348c385
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 32 deletions.
1 change: 1 addition & 0 deletions .config/dictionary.txt
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ matchtasks
matchvar
matchyaml
maxdepth
maxsplit
minversion
mkdir
mkdocs
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/tox.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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: 834
PYTEST_REQPASS: 845
steps:
- uses: actions/checkout@v4
with:
Expand Down
18 changes: 11 additions & 7 deletions src/ansiblelint/transformer.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@

from ansiblelint.file_utils import Lintable
from ansiblelint.rules import AnsibleLintRule, TransformMixin
from ansiblelint.yaml_utils import FormattedYAML, get_path_to_play, get_path_to_task
from ansiblelint.yaml_utils import (
FormattedYAML,
get_path_to_play,
get_path_to_task,
)

if TYPE_CHECKING:
from ansiblelint.config import Options
Expand Down Expand Up @@ -92,7 +96,12 @@ def run(self) -> None:
# We need a fresh YAML() instance for each load because ruamel.yaml
# stores intermediate state during load which could affect loading
# any other files. (Based on suggestion from ruamel.yaml author)
yaml = FormattedYAML()
yaml = FormattedYAML(
# Ansible only uses YAML 1.1, but others files should use newer 1.2 (ruamel.yaml defaults to 1.2)
version=(1, 1)
if file.is_owned_by_ansible()
else None,
)

ruamel_data = yaml.load(data)
if not isinstance(ruamel_data, (CommentedMap, CommentedSeq)):
Expand All @@ -108,11 +117,6 @@ def run(self) -> None:
if self.write_set != {"none"}:
self._do_transforms(file, ruamel_data or data, file_is_yaml, matches)

if not file.is_owned_by_ansible():
# Do nothing until we enable support for YAML 1.2
# https://github.com/ansible/ansible-lint/issues/3811
continue

if file_is_yaml:
_logger.debug("Dumping %s using YAML (%s)", file, yaml.version)
# noinspection PyUnboundLocalVariable
Expand Down
42 changes: 30 additions & 12 deletions src/ansiblelint/yaml_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
if TYPE_CHECKING:
# noinspection PyProtectedMember
from ruamel.yaml.comments import LineCol
from ruamel.yaml.compat import StreamTextType
from ruamel.yaml.compat import StreamTextType, VersionType
from ruamel.yaml.nodes import ScalarNode
from ruamel.yaml.representer import RoundTripRepresenter
from ruamel.yaml.tokens import CommentToken
Expand All @@ -44,6 +44,7 @@

_logger = logging.getLogger(__name__)


YAMLLINT_CONFIG = """
extends: default
rules:
Expand Down Expand Up @@ -750,13 +751,14 @@ def write_version_directive(self, version_text: Any) -> None:
class FormattedYAML(YAML):
"""A YAML loader/dumper that handles ansible content better by default."""

def __init__(
def __init__( # pylint: disable=too-many-arguments
self,
*,
typ: str | None = None,
pure: bool = False,
output: Any = None,
plug_ins: list[str] | None = None,
version: VersionType | None = None,
):
"""Return a configured ``ruamel.yaml.YAML`` instance.
Expand Down Expand Up @@ -816,10 +818,12 @@ def __init__(
tasks:
- name: Task
"""
# Default to reading/dumping YAML 1.1 (ruamel.yaml defaults to 1.2)
self._yaml_version_default: tuple[int, int] = (1, 1)
self._yaml_version: str | tuple[int, int] = self._yaml_version_default

if version:
if isinstance(version, str):
x, y = version.split(".", maxsplit=1)
version = (int(x), int(y))
self._yaml_version_default: VersionType = version
self._yaml_version: VersionType = self._yaml_version_default
super().__init__(typ=typ, pure=pure, output=output, plug_ins=plug_ins)

# NB: We ignore some mypy issues because ruamel.yaml typehints are not great.
Expand Down Expand Up @@ -920,16 +924,18 @@ def _defaults_from_yamllint_config() -> dict[str, bool | int | str]:

return cast(dict[str, Union[bool, int, str]], config)

@property # type: ignore[override]
def version(self) -> str | tuple[int, int]:
@property
def version(self) -> VersionType | None:
"""Return the YAML version used to parse or dump.
Ansible uses PyYAML which only supports YAML 1.1. ruamel.yaml defaults to 1.2.
So, we have to make sure we dump yaml files using YAML 1.1.
We can relax the version requirement once ansible uses a version of PyYAML
that includes this PR: https://github.com/yaml/pyyaml/pull/555
"""
return self._yaml_version
if hasattr(self, "_yaml_version"):
return self._yaml_version
return None

@version.setter
def version(self, value: str | tuple[int, int] | None) -> None:
Expand All @@ -939,7 +945,11 @@ def version(self, value: str | tuple[int, int] | None) -> None:
So, if a file does not include the directive, it sets this to None.
But, None effectively resets the parsing version to YAML 1.2 (ruamel's default).
"""
self._yaml_version = value if value is not None else self._yaml_version_default
if value is not None:
self._yaml_version = value
elif hasattr(self, "_yaml_version_default"):
self._yaml_version = self._yaml_version_default
# We do nothing if the object did not have a previous default version defined

def load(self, stream: Path | StreamTextType) -> Any:
"""Load YAML content from a string while avoiding known ruamel.yaml issues."""
Expand Down Expand Up @@ -977,7 +987,11 @@ def dumps(self, data: Any) -> str:
stream.write(preamble_comment)
self.dump(data, stream)
text = stream.getvalue()
return self._post_process_yaml(text)
strip_version_directive = hasattr(self, "_yaml_version_default")
return self._post_process_yaml(
text,
strip_version_directive=strip_version_directive,
)

def _prevent_wrapping_flow_style(self, data: Any) -> None:
if not isinstance(data, (CommentedMap, CommentedSeq)):
Expand Down Expand Up @@ -1065,7 +1079,7 @@ def _pre_process_yaml(self, text: str) -> tuple[str, str | None]:
return text, "".join(preamble_comments) or None

@staticmethod
def _post_process_yaml(text: str) -> str:
def _post_process_yaml(text: str, *, strip_version_directive: bool = False) -> str:
"""Handle known issues with ruamel.yaml dumping.
Make sure there's only one newline at the end of the file.
Expand All @@ -1077,6 +1091,10 @@ def _post_process_yaml(text: str) -> str:
Make sure null list items don't end in a space.
"""
# remove YAML directive
if strip_version_directive and text.startswith("%YAML"):
text = text.split("\n", 1)[1]

text = text.rstrip("\n") + "\n"

lines = text.splitlines(keepends=True)
Expand Down
59 changes: 47 additions & 12 deletions test/test_yaml_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

if TYPE_CHECKING:
from ruamel.yaml.comments import CommentedMap, CommentedSeq
from ruamel.yaml.compat import VersionType
from ruamel.yaml.emitter import Emitter

fixtures_dir = Path(__file__).parent / "fixtures"
Expand Down Expand Up @@ -202,8 +203,7 @@ def test_custom_ruamel_yaml_emitter(
assert output == expected_output


@pytest.fixture(name="yaml_formatting_fixtures")
def fixture_yaml_formatting_fixtures(fixture_filename: str) -> tuple[str, str, str]:
def load_yaml_formatting_fixtures(fixture_filename: str) -> tuple[str, str, str]:
"""Get the contents for the formatting fixture files.
To regenerate these fixtures, please run ``pytest --regenerate-formatting-fixtures``.
Expand All @@ -220,26 +220,61 @@ def fixture_yaml_formatting_fixtures(fixture_filename: str) -> tuple[str, str, s


@pytest.mark.parametrize(
"fixture_filename",
("before", "after", "version"),
(
pytest.param("fmt-1.yml", id="1"),
pytest.param("fmt-2.yml", id="2"),
pytest.param("fmt-3.yml", id="3"),
pytest.param("fmt-4.yml", id="4"),
pytest.param("fmt-5.yml", id="5"),
pytest.param("---\nfoo: bar\n", "---\nfoo: bar\n", None, id="1"),
# verify that 'on' is not translated to bool (1.2 behavior)
pytest.param("---\nfoo: on\n", "---\nfoo: on\n", None, id="2"),
# When version is manually mentioned by us, we expect to output without version directive
pytest.param("---\nfoo: on\n", "---\nfoo: on\n", (1, 2), id="3"),
pytest.param("---\nfoo: on\n", "---\nfoo: true\n", (1, 1), id="4"),
pytest.param("%YAML 1.1\n---\nfoo: on\n", "---\nfoo: true\n", (1, 1), id="5"),
# verify that in-line directive takes precedence but dumping strips if we mention a specific version
pytest.param("%YAML 1.1\n---\nfoo: on\n", "---\nfoo: true\n", (1, 2), id="6"),
# verify that version directive are kept if present
pytest.param("%YAML 1.1\n---\nfoo: on\n", "---\nfoo: true\n", None, id="7"),
pytest.param(
"%YAML 1.2\n---\nfoo: on\n",
"%YAML 1.2\n---\nfoo: on\n",
None,
id="8",
),
pytest.param("---\nfoo: YES\n", "---\nfoo: true\n", (1, 1), id="9"),
pytest.param("---\nfoo: YES\n", "---\nfoo: YES\n", (1, 2), id="10"),
pytest.param("---\nfoo: YES\n", "---\nfoo: YES\n", None, id="11"),
),
)
def test_fmt(before: str, after: str, version: VersionType) -> None:
"""Tests behavior of formatter in regards to different YAML versions, specified or not."""
yaml = ansiblelint.yaml_utils.FormattedYAML(version=version)
data = yaml.load(before)
result = yaml.dumps(data)
assert result == after


@pytest.mark.parametrize(
("fixture_filename", "version"),
(
pytest.param("fmt-1.yml", (1, 1), id="1"),
pytest.param("fmt-2.yml", (1, 1), id="2"),
pytest.param("fmt-3.yml", (1, 1), id="3"),
pytest.param("fmt-4.yml", (1, 1), id="4"),
pytest.param("fmt-5.yml", (1, 1), id="5"),
),
)
def test_formatted_yaml_loader_dumper(
yaml_formatting_fixtures: tuple[str, str, str],
fixture_filename: str, # noqa: ARG001
fixture_filename: str,
version: tuple[int, int],
) -> None:
"""Ensure that FormattedYAML loads/dumps formatting fixtures consistently."""
# pylint: disable=unused-argument
before_content, prettier_content, after_content = yaml_formatting_fixtures
before_content, prettier_content, after_content = load_yaml_formatting_fixtures(
fixture_filename,
)
assert before_content != prettier_content
assert before_content != after_content

yaml = ansiblelint.yaml_utils.FormattedYAML()
yaml = ansiblelint.yaml_utils.FormattedYAML(version=version)

data_before = yaml.load(before_content)
dump_from_before = yaml.dumps(data_before)
Expand Down

0 comments on commit 348c385

Please sign in to comment.