From bac30743aff6e2c01de6f0683e02f9a380124b82 Mon Sep 17 00:00:00 2001 From: Jacob Callahan Date: Fri, 27 Sep 2024 17:09:33 -0400 Subject: [PATCH] Switch to ruamel to keep comments in yaml files PyYaml is a good library, but had the unfortunate side-effect of stripping comments from yaml files on read/write. Switching to ruamel now let's us retain the comments, and arguably improves usage in some ways. There may be some issues with awxkit objects due to the way the representers are added, but I can't say for certain that is the case yet. --- broker/config_manager.py | 33 +++--- broker/helpers.py | 25 ++--- broker/providers/ansible_tower.py | 13 ++- pyproject.toml | 164 ++++++++++++++---------------- tests/test_config_manager.py | 8 +- 5 files changed, 114 insertions(+), 129 deletions(-) diff --git a/broker/config_manager.py b/broker/config_manager.py index 6a6d6bf..44136d6 100644 --- a/broker/config_manager.py +++ b/broker/config_manager.py @@ -12,10 +12,14 @@ import click from logzero import logger from packaging.version import Version -import yaml +from ruamel.yaml import YAML, YAMLError from broker import exceptions +yaml = YAML() +yaml.default_flow_style = False +yaml.sort_keys = False + C_SEP = "." # chunk separator GH_CFG = "https://raw.githubusercontent.com/SatelliteQE/broker/master/broker_settings.yaml.example" @@ -25,14 +29,6 @@ def file_name_to_ver(file_name): return Version(file_name[1:].replace("_", ".")) -def yaml_format(data): - """Format the data as yaml. - - Duplicating here to avoid circular imports. - """ - return yaml.dump(data, default_flow_style=False, sort_keys=False) - - class ConfigManager: """Class to interact with Broker's configuration file. @@ -52,7 +48,7 @@ def __init__(self, settings_path=None): self._settings_path = settings_path if settings_path: if settings_path.exists(): - self._cfg = yaml.safe_load(self._settings_path.read_text()) + self._cfg = yaml.load(self._settings_path) else: click.secho( f"Broker settings file not found at {settings_path.absolute()}.", fg="red" @@ -74,15 +70,14 @@ def interactive_mode(self): def _interactive_edit(self, chunk): """Write the chunk data to a temporary file and open it in an editor.""" with NamedTemporaryFile(mode="w+", suffix=".yaml") as tmp: - tmp.write(yaml_format(chunk)) - tmp.seek(0) + yaml.dump(chunk, tmp) click.edit(filename=tmp.name) tmp.seek(0) new_data = tmp.read() # first try to load it as yaml try: - return yaml.safe_load(new_data) - except yaml.YAMLError: # then try json + return yaml.load(new_data) + except YAMLError: # then try json try: return json.loads(new_data) except json.JSONDecodeError: # finally, just return the raw data @@ -187,9 +182,7 @@ def update(self, chunk, new_val, curr_chunk=None): # update the config file if it exists if self._settings_path.exists(): self.backup() - self._settings_path.write_text( - yaml.dump(self._cfg, default_flow_style=False, sort_keys=False) - ) + yaml.dump(self._cfg, self._settings_path) else: # we're not at the top level, so keep going down if C_SEP in chunk: curr, chunk = chunk.split(C_SEP, 1) @@ -237,7 +230,7 @@ def init_config_file(self, chunk=None, _from=None): raise exceptions.ConfigurationError( f"Broker settings file not found at {self._settings_path.absolute()}." ) - chunk_data = self.get(chunk, yaml.safe_load(raw_data)) + chunk_data = self.get(chunk, yaml.load(raw_data)) if self.interactive_mode: chunk_data = self._interactive_edit(chunk_data) self.update(chunk, chunk_data) @@ -253,9 +246,7 @@ def migrate(self, force_version=None): for migration in sorted(migrations, key=lambda m: m.TO_VERSION): working_config = migration.run_migrations(working_config) self.backup() - self._settings_path.write_text( - yaml.dump(working_config, default_flow_style=False, sort_keys=False) - ) + yaml.dump(working_config, self._settings_path) logger.info("Config migration complete.") def validate(self, chunk, providers=None): diff --git a/broker/helpers.py b/broker/helpers.py index 835ca63..b65ae55 100644 --- a/broker/helpers.py +++ b/broker/helpers.py @@ -7,6 +7,7 @@ from copy import deepcopy import getpass import inspect +from io import BytesIO import json import os from pathlib import Path @@ -18,13 +19,17 @@ import click from logzero import logger -import yaml +from ruamel.yaml import YAML from broker import exceptions, logger as b_log, settings FilterTest = namedtuple("FilterTest", "haystack needle test") INVENTORY_LOCK = threading.Lock() +yaml = YAML() +yaml.default_flow_style = False +yaml.sort_keys = False + def clean_dict(in_dict): """Remove entries from a dict where value is None.""" @@ -167,15 +172,10 @@ def load_file(file, warn=True): if warn: logger.warning(f"File {file.absolute()} is invalid or does not exist.") return [] - loader_args = {} if file.suffix == ".json": - loader = json + return json.loads(file.read_text()) elif file.suffix in (".yaml", ".yml"): - loader = yaml - loader_args = {"Loader": yaml.FullLoader} - with file.open() as f: - data = loader.load(f, **loader_args) or [] - return data + return yaml.load(file) def resolve_file_args(broker_args): @@ -251,8 +251,7 @@ def update_inventory(add=None, remove=None): inv_data.extend(add) settings.inventory_path.touch() - with settings.inventory_path.open("w") as inv_file: - yaml.dump(inv_data, inv_file) + yaml.dump(inv_data, settings.inventory_path) def yaml_format(in_struct): @@ -263,8 +262,10 @@ def yaml_format(in_struct): :return: yaml-formatted string """ if isinstance(in_struct, str): - in_struct = yaml.load(in_struct, Loader=yaml.FullLoader) - return yaml.dump(in_struct, default_flow_style=False, sort_keys=False) + in_struct = yaml.load(in_struct) + output = BytesIO() # ruamel doesn't natively allow for string output + yaml.dump(in_struct, output) + return output.getvalue().decode("utf-8") def flip_provider_actions(provider_actions): diff --git a/broker/providers/ansible_tower.py b/broker/providers/ansible_tower.py index 4973b03..5b3dec8 100644 --- a/broker/providers/ansible_tower.py +++ b/broker/providers/ansible_tower.py @@ -1,4 +1,5 @@ """Ansible Tower provider implementation.""" + from functools import cache, cached_property import inspect import json @@ -7,7 +8,7 @@ import click from dynaconf import Validator from logzero import logger -import yaml +from ruamel.yaml import YAML from broker import exceptions from broker.helpers import eval_filter, find_origin @@ -21,6 +22,8 @@ from broker import helpers from broker.providers import Provider +YAML().register_class(awxkit.utils.PseudoNamespace) + class JobExecutionError(exceptions.ProviderError): """Raised when a job execution fails.""" @@ -778,9 +781,9 @@ def release(self, name, broker_args=None): ) -def awxkit_representer(dumper, data): - """In order to resolve awxkit objects, a custom representer is needed.""" - return dumper.represent_dict(dict(data)) +# def awxkit_representer(dumper, data): +# """In order to resolve awxkit objects, a custom representer is needed.""" +# return dumper.represent_dict(dict(data)) -yaml.add_representer(awxkit.utils.PseudoNamespace, awxkit_representer) +# yaml.add_representer(awxkit.utils.PseudoNamespace, awxkit_representer) diff --git a/pyproject.toml b/pyproject.toml index 7e9c1dd..1734af1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -8,9 +8,7 @@ description = "The infrastructure middleman." readme = "README.md" requires-python = ">=3.10" keywords = ["broker", "AnsibleTower", "docker", "podman", "beaker"] -authors = [ - {name = "Jacob J Callahan", email = "jacob.callahan05@gmail.com"} -] +authors = [{ name = "Jacob J Callahan", email = "jacob.callahan05@gmail.com" }] classifiers = [ "Development Status :: 4 - Beta", "Intended Audience :: Developers", @@ -27,33 +25,25 @@ dependencies = [ "dynaconf<4.0.0", "logzero", "packaging", - "pyyaml", "rich", "rich_click", + "ruamel.yaml", "setuptools", "ssh2-python312", ] -dynamic = ["version"] # dynamic fields to update on build - version via setuptools_scm +dynamic = [ + "version", +] # dynamic fields to update on build - version via setuptools_scm [project.urls] Repository = "https://github.com/SatelliteQE/broker" [project.optional-dependencies] beaker = ["beaker-client"] -dev = [ - "pre-commit", - "pytest", - "ruff" -] -docker = [ - "docker", - "paramiko" -] +dev = ["pre-commit", "pytest", "ruff"] +docker = ["docker", "paramiko"] podman = ["podman>=5.2"] -setup = [ - "build", - "twine", -] +setup = ["build", "twine"] ssh2_py311 = ["ssh2-python"] ssh2_python = ["ssh2-python"] @@ -72,7 +62,7 @@ include-package-data = true [tool.setuptools.packages.find] include = ["broker"] -[tool.setuptools_scm] # same as use_scm_version=True in setup.py +[tool.setuptools_scm] # same as use_scm_version=True in setup.py [tool.pytest.ini_options] testpaths = ["tests"] @@ -85,82 +75,82 @@ target-version = "py311" fixable = ["ALL"] select = [ - "B002", # Python does not support the unary prefix increment - "B007", # Loop control variable {name} not used within loop body - "B009", # Do not call getattr with a constant attribute value - "B010", # Do not call setattr with a constant attribute value - "B011", # Do not `assert False`, raise `AssertionError` instead - "B013", # Redundant tuple in exception handler - "B014", # Exception handler with duplicate exception - "B023", # Function definition does not bind loop variable {name} - "B026", # Star-arg unpacking after a keyword argument is strongly discouraged - "BLE001", # Using bare except clauses is prohibited - "C", # complexity - "C4", # flake8-comprehensions - "COM818", # Trailing comma on bare tuple prohibited - "D", # docstrings - "E", # pycodestyle - "F", # pyflakes/autoflake - "G", # flake8-logging-format - "I", # isort - "ISC001", # Implicitly concatenated string literals on one line - "N804", # First argument of a class method should be named cls - "N805", # First argument of a method should be named self - "N815", # Variable {name} in class scope should not be mixedCase - "N999", # Invalid module name: '{name}' - "PERF", # Perflint rules - "PGH004", # Use specific rule codes when using noqa + "B002", # Python does not support the unary prefix increment + "B007", # Loop control variable {name} not used within loop body + "B009", # Do not call getattr with a constant attribute value + "B010", # Do not call setattr with a constant attribute value + "B011", # Do not `assert False`, raise `AssertionError` instead + "B013", # Redundant tuple in exception handler + "B014", # Exception handler with duplicate exception + "B023", # Function definition does not bind loop variable {name} + "B026", # Star-arg unpacking after a keyword argument is strongly discouraged + "BLE001", # Using bare except clauses is prohibited + "C", # complexity + "C4", # flake8-comprehensions + "COM818", # Trailing comma on bare tuple prohibited + "D", # docstrings + "E", # pycodestyle + "F", # pyflakes/autoflake + "G", # flake8-logging-format + "I", # isort + "ISC001", # Implicitly concatenated string literals on one line + "N804", # First argument of a class method should be named cls + "N805", # First argument of a method should be named self + "N815", # Variable {name} in class scope should not be mixedCase + "N999", # Invalid module name: '{name}' + "PERF", # Perflint rules + "PGH004", # Use specific rule codes when using noqa "PLC0414", # Useless import alias. Import alias does not rename original package. - "PLC", # pylint - "PLE", # pylint - "PLR", # pylint - "PLW", # pylint - "PTH", # Use pathlib - "RUF", # Ruff-specific rules - "S103", # bad-file-permissions - "S108", # hardcoded-temp-file - "S110", # try-except-pass - "S112", # try-except-continue - "S113", # Probable use of requests call without timeout - "S306", # suspicious-mktemp-usage - "S307", # suspicious-eval-usage - "S601", # paramiko-call - "S602", # subprocess-popen-with-shell-equals-true - "S604", # call-with-shell-equals-true - "S609", # unix-command-wildcard-injection - "SIM105", # Use contextlib.suppress({exception}) instead of try-except-pass - "SIM117", # Merge with-statements that use the same scope - "SIM118", # Use {key} in {dict} instead of {key} in {dict}.keys() - "SIM201", # Use {left} != {right} instead of not {left} == {right} - "SIM208", # Use {expr} instead of not (not {expr}) - "SIM212", # Use {a} if {a} else {b} instead of {b} if not {a} else {a} - "SIM300", # Yoda conditions. Use 'age == 42' instead of '42 == age'. - "SIM401", # Use get from dict with default instead of an if block - "T100", # Trace found: {name} used - "T20", # flake8-print - "TRY004", # Prefer TypeError exception for invalid type - "TRY302", # Remove exception handler; error is immediately re-raised + "PLC", # pylint + "PLE", # pylint + "PLR", # pylint + "PLW", # pylint + "PTH", # Use pathlib + "RUF", # Ruff-specific rules + "S103", # bad-file-permissions + "S108", # hardcoded-temp-file + "S110", # try-except-pass + "S112", # try-except-continue + "S113", # Probable use of requests call without timeout + "S306", # suspicious-mktemp-usage + "S307", # suspicious-eval-usage + "S601", # paramiko-call + "S602", # subprocess-popen-with-shell-equals-true + "S604", # call-with-shell-equals-true + "S609", # unix-command-wildcard-injection + "SIM105", # Use contextlib.suppress({exception}) instead of try-except-pass + "SIM117", # Merge with-statements that use the same scope + "SIM118", # Use {key} in {dict} instead of {key} in {dict}.keys() + "SIM201", # Use {left} != {right} instead of not {left} == {right} + "SIM208", # Use {expr} instead of not (not {expr}) + "SIM212", # Use {a} if {a} else {b} instead of {b} if not {a} else {a} + "SIM300", # Yoda conditions. Use 'age == 42' instead of '42 == age'. + "SIM401", # Use get from dict with default instead of an if block + "T100", # Trace found: {name} used + "T20", # flake8-print + "TRY004", # Prefer TypeError exception for invalid type + "TRY302", # Remove exception handler; error is immediately re-raised "PLR0911", # Too many return statements ({returns} > {max_returns}) "PLR0912", # Too many branches ({branches} > {max_branches}) "PLR0915", # Too many statements ({statements} > {max_statements}) "PLR2004", # Magic value used in comparison, consider replacing {value} with a constant variable "PLW2901", # Outer {outer_kind} variable {name} overwritten by inner {inner_kind} target - "UP", # pyupgrade - "W", # pycodestyle + "UP", # pyupgrade + "W", # pycodestyle ] ignore = [ - "ANN", # flake8-annotations - "D203", # 1 blank line required before class docstring - "D213", # Multi-line docstring summary should start at the second line - "D406", # Section name should end with a newline - "D407", # Section name underlining - "D413", # Missing blank line after last section - "E501", # line too long - "E731", # do not assign a lambda expression, use a def + "ANN", # flake8-annotations + "D203", # 1 blank line required before class docstring + "D213", # Multi-line docstring summary should start at the second line + "D406", # Section name should end with a newline + "D407", # Section name underlining + "D413", # Missing blank line after last section + "E501", # line too long + "E731", # do not assign a lambda expression, use a def "PLR0913", # Too many arguments to function call ({c_args} > {max_args}) - "RUF012", # Mutable class attributes should be annotated with typing.ClassVar - "D107", # Missing docstring in __init__ + "RUF012", # Mutable class attributes should be annotated with typing.ClassVar + "D107", # Missing docstring in __init__ ] [tool.ruff.flake8-pytest-style] @@ -168,9 +158,7 @@ fixture-parentheses = false [tool.ruff.isort] force-sort-within-sections = true -known-first-party = [ - "broker", -] +known-first-party = ["broker"] combine-as-imports = true [tool.ruff.per-file-ignores] diff --git a/tests/test_config_manager.py b/tests/test_config_manager.py index 798c53a..ddc6ce2 100644 --- a/tests/test_config_manager.py +++ b/tests/test_config_manager.py @@ -1,9 +1,11 @@ """Test file for broker.config_manager module.""" -import yaml +from ruamel.yaml import YAML from broker.config_manager import ConfigManager, GH_CFG from broker.settings import settings_path -TEST_CFG_DATA = yaml.safe_load(settings_path.read_text()) + +yaml = YAML() +TEST_CFG_DATA = yaml.load(settings_path.read_text()) def test_basic_assertions(): @@ -19,7 +21,7 @@ def test_import_config(): cfg_mgr = ConfigManager() result = cfg_mgr._import_config(GH_CFG, is_url=True) assert isinstance(result, str) - converted = yaml.safe_load(result) + converted = yaml.load(result) assert isinstance(converted, dict) def test_get_e2e():