rpm PoC: Allow specifying DNF options via CLI input JSON #522
Conversation
eskultety
requested review from
brunoapimentel,
taylormadore,
ben-alkov,
chmeliik,
ejegrova and
slimreaper35
|
@brunoapimentel I wonder if I should extract the |
eskultety
force-pushed
the
rpm_prefetch_options
branch
from
21f454f to
28d3b3c
Compare
I did some tests with the implementation based on The only thing I was in middle of reviewing before getting carried away with something else was the custom validation you added in the last commit. From the few manual tests I did, it seemed that Pydantic handled these same use cases pretty well, and I thought that the error messages were reasonable. But I've likely missed the issues you had with unit tests. |
Soo, let me illustrate the issue on 1 example: ^This is what I get without the validator, IOW the only bit of information I get is 'Field required' but it doesn't tell me anything else like what field is missing, so despite vs This is after introducing the validator It's at least IMO clear that a keyword 'dnf' is missing, so from that perspective I think that message is clearer. |
Oh, I see it now. When you invoke the actual CLI, the output you get is different (although I'm not sure which piece is making the conversion). I started with your example here, and extrapolated a little: I think these messages are a lot clearer than the ones you posted before, so maybe we could just go with them. Wdyt? |
Uhm, I'm sorry, I'm afraid you lost me a bit in your examples, so do you want me to modify the messages I emit in the validator in any way or are we okay with what is proposed? |
eskultety
force-pushed
the
rpm_prefetch_options
branch
from
28d3b3c to
e213c13
Compare
eskultety
force-pushed
the
rpm_prefetch_options
branch
from
e213c13 to
596f705
Compare
|
@brunoapimentel I needed to update this PR to reflect your recent integration test changes by adjusting them to compare on the parsed structure contents rather than verbatim file contents when it comes to the Configparser instance I introduced. Let me know if you still agree with that adjustment so that I can finally merge this. (this time for real if CI passes) |
eskultety
force-pushed
the
rpm_prefetch_options
branch
from
596f705 to
192536d
Compare
The changes on the integration test LGTM. Let's get this merged :) |
Cosmetic change - this is mainly to improve the UX of the test results run summary helping to identify/describe what test cases we run for the input validation. Signed-off-by: Erik Skultety <[email protected]>
Cosmetic change - we can convey the same message on a single line and enhance code readability just a tiny bit without line splits. Signed-off-by: Erik Skultety <[email protected]>
This is called as a callback from the cli.py module where we always have both from_output_dir and for_output_dir resolved. It's fair to assume any inject_files post action would need both of these to accomplish whatever it is the action needs to do, so listing both arguments as positional in the actual callback implementation makes the code a bit easier to read. Signed-off-by: Erik Skultety <[email protected]>
Repo files follow the INI syntax for which we can use ConfigParser. The motivation behind using a library abstraction to generate the INI repo files stems from the fact that we already have some options rendered conditionally with a fair chance of extending this in the future with more custom user-supplied DNF repo options which means even more conditionals which would stretch the capabilities of a plain triple string (or a template for that matter) a bit too far. Therefore, this patch proposes using ConfigParser instance for creating and dumping the repo files in the inject_files post action. Note that to make things a tiny bit more ergonomic, we subclass the ConfigParser class to define a couple utility methods that can tell us whether the ConfigParser instance is effectively empty (because an empty instance of ConfigParser still evaluates to True) and thus can be dumped to a file as well as some extra handling of default options, e.g. defaulting to 'gpgcheck=1' explicitly in the generated .repo file. Signed-off-by: Erik Skultety <[email protected]>
The sorting was added solely to make comparing of the expected and generated repo files' contents more deterministic in unit tests, but now that we're driving the generation process with ConfigParser, we can perform the comparisons on the object level rather than actual file contents, and so we no longer need to sacrifice the beauty of using iterdir's generator by converting it to a list just to keep tests deterministic. Signed-off-by: Erik Skultety <[email protected]>
Introduce a new input JSON attribute 'options' for the PoC rpm package
manager allowing consumers to pass arbitrary DNF repo configuration
options that would be applied later through the inject-files command.
The general schema for the input JSON is as follows:
{
type: <package_manager>
path: <relative_path>
options: {
<namespace>: {
<key1>..<keyN>: Union[str, Any]<val1>
}
}
}
which translated to DNF repo options for RPM might then look like:
{
type: rpm
path: .
options: {
dnf: {
repoid1: {gpgcheck: 0, deltarpm: 1},
repoid2: {fastestmirro: 1, sslverify: 0}
}
}
}
This implementation is trying to be generic enough to be potentially
later extended to other package managers as well which also means that
due to the fairly complex (multi-level nested dictionary) input
a new intermediary model _DNFOptions was needed to be introduced in
this patch, but most importantly a custom 'before' model validator for
this model had to be implemented to generate stable and deterministic
errors which could be reliably unit-tested (default pydantic error
messages for such complex structures are madness).
Worth noting would be that unless options were specified, they would
not appear in the build-config.json --> exclude_none=True
Signed-off-by: Erik Skultety <[email protected]>
eskultety
force-pushed
the
rpm_prefetch_options
branch
from
192536d to
a0497be
Compare
e5caa8c
Introduce a new input JSON attribute 'options' for the PoC rpm package
manager allowing consumers to pass arbitrary DNF repo configuration
options that would be applied later through the inject-files command.
The general schema for the input JSON is as follows:
which translated to DNF repo options for RPM might then look like:
which in turn would end up being dumped to the
.build-config.jsonsimilarly to this:and finally the resulting generated
.repofile might look like:Notes:
optionsare only ever formatted in the build config if some were actually set on the CLI (rather than being formatted asoptions: {}) so that this would be transparent to other package manager for which we don't intend to support options at the moment; this was all achieved by settingexclude_none=Trueglobally forbuildconfig.model_dump_json.build-config.jsonnamespaced by"rpm"is due to the fact that it's possible to specify more than one package with the input JSON on CLI, thus theoretically possible to provide various options for various package managers (if ever turned on) so we'd need to know which options relate to which package manager type which wouldn't otherwise be possible from current build config contents.Maintainers will complete the following section
Docs updated (if applicable)Docs links in the code are still valid (if docs were updated)Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:
/ok-to-test(as is the standard for Pipelines as Code)