-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add JSON output to advisory list #1531
Add JSON output to advisory list #1531
Conversation
Mainly as a draft because I don't have any kind of schema / end user documentation on it (let alone tests), and I want to ensure that exists before anyone may start consuming it (or I backport to... 😢 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There appear to be some formatting problems (we have a style defined in .clang-format
) check out the Pre Commit Hooks
action.
a31c2c6
to
6e8302e
Compare
I should have fixed this up now... |
LIBDNF_CLI_API void print_advisorylist_references_table( | ||
std::vector<libdnf5::advisory::AdvisoryPackage> & advisory_package_list_not_installed, | ||
std::vector<libdnf5::advisory::AdvisoryPackage> & advisory_package_list_installed, | ||
std::string pretty_reference_type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry but since this is marked LIBDNF_CLI_API
we don't want to change it like this. It would be a breaking API change.
Please keep the original signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I'll keep the reference_type
and move the pretty name over to a separate function it can call.
Since updateinfo.xml
in the wild lists a lot more than bugzilla
and cve
, see
dnf5/doc/repository_format/updateinfo-permissive.xsd
Lines 506 to 531 in 65076dc
<xs:documentation xml:lang="en"> | |
The openSUSE schema defines the only valid values as either cve or | |
bugzilla. In practice, this should likely be extended, but yum and | |
dnf also use this nomenclature as the only options. | |
As per opensuse-leap-15.4-oss-2024-05-02 (at least), jira is another option. | |
As per opensuse-leap-15.5-sle-2024-05-23 (at least), fate and github are other options | |
OpenSuSE 11.4 references launchpad. | |
OpenSuSE 12.2, 12.3, 13.1 reference sourceforge. | |
Alma Linux references rhsa and self. | |
MariaDB repositories reference other. | |
Amazon Linux 1 references redhat. | |
</xs:documentation> | |
</xs:annotation> | |
<xs:enumeration value="bugzilla"/> | |
<xs:enumeration value="cve"/> | |
<xs:enumeration value="jira"/> | |
<xs:enumeration value="fate"/> | |
<xs:enumeration value="github"/> | |
<xs:enumeration value="launchpad"/> | |
<xs:enumeration value="sourceforge"/> | |
<xs:enumeration value="rhsa"/> | |
<xs:enumeration value="redhat"/> | |
<xs:enumeration value="self"/> | |
<xs:enumeration value="other"/> | |
</xs:restriction> | |
</xs:simpleType> |
Thus, for a future patch, I want to support these too.... just not sure how we would want to deal with the pretty names for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about it and personally I would just drop the pretty name from the json output. For the plain text output it is just a column header to let the user know what it means but in json output the type information is already there. Plus we have to guess how it should look like.
I like the fallback to the type string (even for the plain text). 👍
Otherwise this looks quite good to me. I have made an issue to add tests to our CI and I think we can merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (in the revision I'm about to push).
If there's a guide to how to add some tests, I'm happy to help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI is a somewhat complicated. You can read the README for general overview and how to run it in a container.
Its also possible to run directly (which is fine in this case since these commands don't modify the host system in any way) described here. Though its a bit outdated, for dnf5 the behave command should look something like:
behave --tags @dnf5 -Ddnf_command=dnf5 ./ci-dnf-stack/dnf-behave-tests/dnf/updateinfo.feature
Dependencies from requirements.spec
and python requirements.txt
are needed.
For the actual tests I would say the features are quite self explanatory, look at: https://github.com/rpm-software-management/ci-dnf-stack/blob/main/dnf-behave-tests/dnf/updateinfo.feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should add that the build with the advisory list --json
option will be available in https://copr.fedorainfracloud.org/coprs/rpmsoftwaremanagement/dnf-nightly/builds/ tomorrow so if you build the container today by default it won't be there.
6e8302e
to
363cf63
Compare
Oh and there is a couple of formatting issues again. |
I'm going to judge the |
Similar to the repolist --json output, do it for listing advisories that apply to the system ("dnf advisory list") as well as for the references currently supported Bugzilla and CVE types of references. These follow what is output on the terminal. The exception is that the queries with a reference (--with-bz and --with-cve) also print the advisory name. This is just to make it a bit easier for other tools which may already know about that advisory ID. Example of advisory list --json: [ { "name":"FEDORA-2024-143f0443e1", "type":"unspecified", "severity":"None", "nevra":"yum-4.19.2-1.fc40.noarch", "buildtime":"2024-04-19 21:20:20" }, { "name":"FEDORA-2024-1e8a70c30e", "type":"enhancement", "severity":"None", "nevra":"zstd-1.5.6-1.fc40.aarch64", "buildtime":"2024-04-19 21:20:20" } ] Example of advisory list --with-bz --json: [ { "advisory_name":"FEDORA-2024-0c9d3b51d4", "advisory_type":"security", "advisory_severity":"security", "advisory_buildtime":"2024-05-02 01:55:56", "nevra":"tpm2-tss-fapi-4.1.0-1.fc40.aarch64", "references":[ { "reference_id":"2271763", "reference_type":"bugzilla", }, { "reference_id":"2277437", "reference_type":"bugzilla", } ] } ]
363cf63
to
417cb70
Compare
Great, thank you! |
12c73b2
Similar to the repolist --json output, do it for listing advisories that apply to the system ("dnf advisory list") as well as for the references currently supported Bugzilla and CVE types of references.
(examples edited as of 417cb70)
Example of advisory list --json:
Example of advisory list --with-bz --json:
Alternative implementation of #1524 using the same style as the
repolist
JSON output added in 4ffa9d1.