From 5bf1bbc6ea1852c545d35126e0f204fb9e57ef85 Mon Sep 17 00:00:00 2001 From: Rohan McGovern Date: Thu, 31 Oct 2024 10:43:01 +1100 Subject: [PATCH 1/4] Let BootMode enum have a round-trippable repr enum.Enum surprisingly violates the usual convention of having "repr" be the inverse of "eval" (see bug [1]). For example, it produces a string like . This breaks pushsource-ls command with the default black formatter, since black expects the input to be valid Python code. Also, even if the formatting worked, it would break the ability to copy-paste the generated push items into a python script. Fix it by overriding `__repr__`. The implementation is identical to the suggestion in the stdlib docs[2]. [1] https://bugs.python.org/issue40066 [2] https://docs.python.org/3/library/enum.html#enum.Enum.__repr__ --- src/pushsource/_impl/model/vms.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/pushsource/_impl/model/vms.py b/src/pushsource/_impl/model/vms.py index 8d969f16..babc223a 100644 --- a/src/pushsource/_impl/model/vms.py +++ b/src/pushsource/_impl/model/vms.py @@ -19,6 +19,10 @@ class BootMode(enum.Enum): legacy = "legacy" """Supports only BIOS mode.""" + def __repr__(self): + cls_name = self.__class__.__name__ + return f"{cls_name}.{self.name}" + @attr.s() class VMIRelease(object): From 424a6894c99df25dd307bca8b8549518e990d5ae Mon Sep 17 00:00:00 2001 From: Rohan McGovern Date: Thu, 31 Oct 2024 11:04:17 +1100 Subject: [PATCH 2/4] Fix pushsource-ls unable to serialize BootMode as YAML pushsource-ls with YAML format enabled would crash if encountering a BootMode field (as used on AMI push items). This is because pyyaml does not enable any representer for enums by default. Fix it by adding a custom representer which will simply serialize the underlying value (e.g. BootMode.hybrid becomes "hybrid"). Note this is only being fixed for pushsource-ls and not in pushsource library proper because there does not seem to be a single universally-correct method of serializing enums into YAML. The chosen representation here is merely something "good enough" for pushsource-ls use-cases. --- src/pushsource/_impl/list_cmd.py | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/pushsource/_impl/list_cmd.py b/src/pushsource/_impl/list_cmd.py index 57f8fd4a..66f8123a 100755 --- a/src/pushsource/_impl/list_cmd.py +++ b/src/pushsource/_impl/list_cmd.py @@ -25,15 +25,40 @@ from argparse import ArgumentParser, RawDescriptionHelpFormatter import subprocess import sys +import enum import attr import yaml +import pushsource from pushsource import Source LOG = logging.getLogger("pushsource-ls") +class ItemDumper(yaml.SafeDumper): + # Custom dumper adding support for any types appearing on pushitems + # which are not natively supported by pyyaml. + + @classmethod + def represent_enum(cls, dumper: yaml.Dumper, value: enum.Enum): + # enums are unwrapped and represented using whatever's the underlying + # type, e.g. a string enum of value "foo" will be serialized the + # same as a plain string "foo". + return dumper.represent_data(value.value) + + @classmethod + def add_enum_representers(cls): + # Register our enum representer for any enum classes in the API. + for attrname in dir(pushsource): + attrval = getattr(pushsource, attrname) + if isinstance(attrval, enum.EnumType): + cls.add_representer(attrval, cls.represent_enum) + + +ItemDumper.add_enum_representers() + + def format_python(item): return repr(item) + "," @@ -51,7 +76,7 @@ def format_python_black(item): def format_yaml(item): data = {type(item).__name__: attr.asdict(item, recurse=True)} - return yaml.dump([data], Dumper=yaml.SafeDumper) + return yaml.dump([data], Dumper=ItemDumper) def default_format(): From 45d294c92893f3eff0705c3f11488cc9a442bc91 Mon Sep 17 00:00:00 2001 From: Rohan McGovern Date: Thu, 31 Oct 2024 11:47:14 +1100 Subject: [PATCH 3/4] Fix pushsource-ls unable to serialize frozendict (on some versions) On at least some pyyaml and frozendict versions, it is necessary to explicitly add a representer for the frozendict type, as it will not automatically be processed in the same way as a regular dict. --- src/pushsource/_impl/list_cmd.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pushsource/_impl/list_cmd.py b/src/pushsource/_impl/list_cmd.py index 66f8123a..7e484cd5 100755 --- a/src/pushsource/_impl/list_cmd.py +++ b/src/pushsource/_impl/list_cmd.py @@ -29,6 +29,7 @@ import attr import yaml +from frozendict.core import frozendict import pushsource from pushsource import Source @@ -57,6 +58,7 @@ def add_enum_representers(cls): ItemDumper.add_enum_representers() +ItemDumper.add_representer(frozendict, ItemDumper.represent_dict) def format_python(item): From 8aa40769156dbedb06675a37e09fb4e1400fc4a6 Mon Sep 17 00:00:00 2001 From: Rohan McGovern Date: Thu, 31 Oct 2024 11:14:55 +1100 Subject: [PATCH 4/4] tests: improve pushsource-ls coverage Test the pushsource-ls command over each staging area in the test data. This helps ensure that any new fields on pushitems can be serialized successfully. For example, it would have caught the serialization issue with BootMode enum fixed in prior commits. Requires adding 'black' to the test requirements so that this format can be tested. --- src/pushsource/_impl/list_cmd.py | 2 +- test-requirements.in | 1 + test-requirements.txt | 39 ++++++++++++++++++++++++++++++++ tests/cmd/test_list_cmd.py | 30 ++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 1 deletion(-) diff --git a/src/pushsource/_impl/list_cmd.py b/src/pushsource/_impl/list_cmd.py index 7e484cd5..f9578a3d 100755 --- a/src/pushsource/_impl/list_cmd.py +++ b/src/pushsource/_impl/list_cmd.py @@ -53,7 +53,7 @@ def add_enum_representers(cls): # Register our enum representer for any enum classes in the API. for attrname in dir(pushsource): attrval = getattr(pushsource, attrname) - if isinstance(attrval, enum.EnumType): + if isinstance(attrval, enum.EnumMeta): cls.add_representer(attrval, cls.represent_enum) diff --git a/test-requirements.in b/test-requirements.in index 18d404c8..62c7aa71 100644 --- a/test-requirements.in +++ b/test-requirements.in @@ -8,6 +8,7 @@ koji>=1.18 mock more-executors pre-commit +black pushcollector pylint pytest diff --git a/test-requirements.txt b/test-requirements.txt index aafd94e4..a0494be5 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -32,6 +32,30 @@ bandit==1.7.10 \ --hash=sha256:59ed5caf5d92b6ada4bf65bc6437feea4a9da1093384445fed4d472acc6cff7b \ --hash=sha256:665721d7bebbb4485a339c55161ac0eedde27d51e638000d91c8c2d68343ad02 # via -r test-requirements.in +black==24.10.0 \ + --hash=sha256:14b3502784f09ce2443830e3133dacf2c0110d45191ed470ecb04d0f5f6fcb0f \ + --hash=sha256:17374989640fbca88b6a448129cd1745c5eb8d9547b464f281b251dd00155ccd \ + --hash=sha256:1c536fcf674217e87b8cc3657b81809d3c085d7bf3ef262ead700da345bfa6ea \ + --hash=sha256:1cbacacb19e922a1d75ef2b6ccaefcd6e93a2c05ede32f06a21386a04cedb981 \ + --hash=sha256:1f93102e0c5bb3907451063e08b9876dbeac810e7da5a8bfb7aeb5a9ef89066b \ + --hash=sha256:2cd9c95431d94adc56600710f8813ee27eea544dd118d45896bb734e9d7a0dc7 \ + --hash=sha256:30d2c30dc5139211dda799758559d1b049f7f14c580c409d6ad925b74a4208a8 \ + --hash=sha256:394d4ddc64782e51153eadcaaca95144ac4c35e27ef9b0a42e121ae7e57a9175 \ + --hash=sha256:3bb2b7a1f7b685f85b11fed1ef10f8a9148bceb49853e47a294a3dd963c1dd7d \ + --hash=sha256:4007b1393d902b48b36958a216c20c4482f601569d19ed1df294a496eb366392 \ + --hash=sha256:5a2221696a8224e335c28816a9d331a6c2ae15a2ee34ec857dcf3e45dbfa99ad \ + --hash=sha256:63f626344343083322233f175aaf372d326de8436f5928c042639a4afbbf1d3f \ + --hash=sha256:649fff99a20bd06c6f727d2a27f401331dc0cc861fb69cde910fe95b01b5928f \ + --hash=sha256:680359d932801c76d2e9c9068d05c6b107f2584b2a5b88831c83962eb9984c1b \ + --hash=sha256:846ea64c97afe3bc677b761787993be4991810ecc7a4a937816dd6bddedc4875 \ + --hash=sha256:b5e39e0fae001df40f95bd8cc36b9165c5e2ea88900167bddf258bacef9bbdc3 \ + --hash=sha256:ccfa1d0cb6200857f1923b602f978386a3a2758a65b52e0950299ea014be6800 \ + --hash=sha256:d37d422772111794b26757c5b55a3eade028aa3fde43121ab7b673d050949d65 \ + --hash=sha256:ddacb691cdcdf77b96f549cf9591701d8db36b2f19519373d60d31746068dbf2 \ + --hash=sha256:e6668650ea4b685440857138e5fe40cde4d652633b1bdffc62933d0db4ed9812 \ + --hash=sha256:f9da3333530dbcecc1be13e69c250ed8dfa67f43c4005fb537bb426e19200d50 \ + --hash=sha256:fe4d6476887de70546212c99ac9bd803d90b42fc4767f058a0baa895013fbb3e + # via -r test-requirements.in certifi==2024.8.30 \ --hash=sha256:922820b53db7a7257ffbda3f597266d435245903d80737e34f8a45ff3e3230d8 \ --hash=sha256:bec941d2aa8195e248a60b31ff9f0558284cf01a52591ceda73ea9afffd69fd9 @@ -218,6 +242,10 @@ charset-normalizer==3.4.0 \ --hash=sha256:fe9f97feb71aa9896b81973a7bbada8c49501dc73e58a10fcef6663af95e5079 \ --hash=sha256:ffc519621dce0c767e96b9c53f09c5d215578e10b02c285809f76509a3931482 # via requests +click==8.1.7 \ + --hash=sha256:ae74fb96c20a0277a1d615f1e4d73c8414f5a98db8b799a7931d1582f3390c28 \ + --hash=sha256:ca9853ad459e787e2192211578cc907e7594e294c7ccc834310722b41b9ca6de + # via black coverage[toml]==7.6.4 \ --hash=sha256:00a1d69c112ff5149cabe60d2e2ee948752c975d95f1e1096742e6077affd376 \ --hash=sha256:023bf8ee3ec6d35af9c1c6ccc1d18fa69afa1cb29eaac57cb064dbb262a517f9 \ @@ -565,6 +593,10 @@ more-executors==2.11.4 \ # -r requirements.in # -r test-requirements.in # pushcollector +mypy-extensions==1.0.0 \ + --hash=sha256:4392f6c0eb8a5668a69e23d168ffa70f0be9ccfd32b5cc2d26a34ae5b844552d \ + --hash=sha256:75dbf8955dc00442a438fc4d0666508a9a97b6bd41aa2f0ffe9d2f2725af0782 + # via black nodeenv==1.9.1 \ --hash=sha256:6ec12890a2dab7946721edbfbcd91f3319c6ccc9aec47be7c7e6b7011ee6645f \ --hash=sha256:ba11c9782d29c27c70ffbdda2d7415098754709be8a7056d79a737cd901155c9 @@ -573,8 +605,13 @@ packaging==24.1 \ --hash=sha256:026ed72c8ed3fcce5bf8950572258698927fd1dbda10a5e981cdf0ac37f4f002 \ --hash=sha256:5b8f2217dbdbd2f7f384c41c628544e6d52f2d0f53c6d0c3ea61aa5d1d7ff124 # via + # black # pytest # sphinx +pathspec==0.12.1 \ + --hash=sha256:a0d503e138a4c123b27490a4f7beda6a01c6f288df0e4a8b79c7eb0dc7b4cc08 \ + --hash=sha256:a482d51503a1ab33b1c67a6c3813a26953dbdc71c31dacaef9a838c4e29f5712 + # via black pbr==6.1.0 \ --hash=sha256:788183e382e3d1d7707db08978239965e8b9e4e5ed42669bf4758186734d5f24 \ --hash=sha256:a776ae228892d8013649c0aeccbb3d5f99ee15e005a4cbb7e61d55a067b28a2a @@ -587,6 +624,7 @@ platformdirs==4.3.6 \ --hash=sha256:357fb2acbc885b0419afd3ce3ed34564c13c9b95c89360cd9563f73aa5e2b907 \ --hash=sha256:73e575e1408ab8103900836b97580d5307456908a03e92031bab39e4554cc3fb # via + # black # pylint # virtualenv pluggy==1.5.0 \ @@ -899,6 +937,7 @@ tomli==2.0.2 \ --hash=sha256:2ebe24485c53d303f690b0ec092806a085f07af5a5aa1464f3931eec36caaa38 \ --hash=sha256:d46d457a85337051c36524bc5349dd91b1877838e2979ac5ced3e710ed8a60ed # via + # black # coverage # pylint # pytest diff --git a/tests/cmd/test_list_cmd.py b/tests/cmd/test_list_cmd.py index 5138b9e6..591d923c 100644 --- a/tests/cmd/test_list_cmd.py +++ b/tests/cmd/test_list_cmd.py @@ -2,6 +2,7 @@ import sys import logging import textwrap +import itertools import yaml import pytest @@ -69,6 +70,35 @@ def test_typical(monkeypatch, capsys): assert "My wonderful ISO" in out +@pytest.mark.parametrize( + "format,case", + itertools.product( + # The supported formats + ["yaml", "python", "python-black"], + # The staging areas. + # Note: limited to 'simple' because there are some staging areas + # intentionally set up to trigger errors, which will also cause + # pushsource-ls to fail. + [ + e.name + for e in os.scandir(STAGED_DATA_DIR) + if e.is_dir() and "simple" in e.name + ], + ), +) +def test_staged(monkeypatch, format, case): + """pushsource-ls runs without crashing with all formats on staging areas + in the test data. + """ + + monkeypatch.setattr( + sys, "argv", ["", "--format", format, f"staged:{STAGED_DATA_DIR}/{case}"] + ) + + # It should run OK + list_main() + + def test_valid_yaml(monkeypatch, capsys): """'--format yaml' generates valid YAML."""