-
Notifications
You must be signed in to change notification settings - Fork 424
feat: Add JSON output for updateinfo #2200
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
base: master
Are you sure you want to change the base?
feat: Add JSON output for updateinfo #2200
Conversation
|
Hello @walkerever! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2025-01-28 01:22:39 UTC |
2325ee1 to
8fead9a
Compare
|
for A test about |
8fead9a to
036c511
Compare
|
Known limit as best effort without introducing significant code changes:
|
036c511 to
6119ab0
Compare
|
|
||
| # Keep quiet when dumping JSON output | ||
| if self.opts.json: | ||
| self.cli.redirect_logger(stdout=sys.maxsize, stderr=sys.maxsize) |
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.
This is good but there still are some redundant messages in the output, namely I think the download progress bars.
We could additionally do something similar to what the --quiet option does: https://github.com/rpm-software-management/dnf/blob/master/dnf/cli/cli.py#L834-L836
To take effect I believe it would have to be set in pre_configure of the command.
I would add something like:
def pre_configure(self):
if self.opts.json:
self.base.conf.debuglevel = 0Perhaps don't change the errorlevel since those message are more crucial.
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.
ack. will test this;)
| dtlst.append( | ||
| { | ||
| "name": aid, | ||
| "type": atype, | ||
| "severity": asev, | ||
| "nevra": nevra, | ||
| "buildtime": aupdated, |
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.
dnf5 has special logic when --with-bz or --with-cve is used: https://github.com/rpm-software-management/dnf5/blob/main/dnf5/commands/advisory/advisory_list.cpp#L75-L81
It adds references to the list subcommand and changes the IDs I think it would be good to be compatible as much as possible. Could you look into that?
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.
sure, ack.
| pkg_str += f".{pkg_info.get('arch')}" | ||
| package_list.append(pkg_str) | ||
|
|
||
| REFERENCE_TYPES = {0: 'unknown', 1: 'bugzilla', 2: 'cve', 3: 'vendor', 4: 'security'} |
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 think there is no reference type security.
Also you should not define the numbers here by hand, it would be better to use the exported hawkey constatns:
REFERENCE_TYPES = {hawkey.REFERENCE_UNKNOWN: 'unknown', hawkey.REFERENCE_BUGZILLA: 'bugzilla',
hawkey.REFERENCE_CVE: 'cve', hawkey.REFERENCE_VENDOR: 'vendor'}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.
ack
| asev = self.SECURITY2LABEL.get(sev, _('None')) | ||
| asev = asev.split("/")[0].strip() |
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 think a simpler solution would be to just do:
asev = sevIt is also what dnf5 does so the output will be more compatible.
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.
ack
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.
for severity, for dnf updateinfo --list output, the format is Medium/Sec for cve type, and single word for others,
i FEDORA-2024-3c18fe0d93 Important/Sec. python-unversioned-command-3.13.1-2.fc41.noarch
i FEDORA-2025-e911f71d99 Moderate/Sec. python-unversioned-command-3.13.2-1.fc41.noarch
i FEDORA-2024-3c18fe0d93 Important/Sec. python3-3.13.1-2.fc41.x86_64
i FEDORA-2025-e911f71d99 Moderate/Sec. python3-3.13.2-1.fc41.x86_64
i FEDORA-2025-52b16605d4 bugfix python3-argcomplete-3.5.3-1.fc41.noarch
i FEDORA-2025-397632c71b bugfix python3-babel-2.17.0-1.fc41.noarch
while for dnf5 json output, it's simply Medium. example as below. reference rpm-software-management/dnf5#1531. that was the reason asev didn't copy from sev.
# dnf advisory list --json | head -20
[
{
"name":"FEDORA-2024-56efaa7783",
"type":"enhancement",
"severity":"Low",
"nevra":"alternatives-1.31-1.fc41.x86_64",
"buildtime":"2024-12-22 02:00:45"
},
{
"name":"FEDORA-2025-9bef5569b2",
"type":"enhancement",
"severity":"None",
"nevra":"audit-libs-4.0.3-1.fc41.x86_64",
"buildtime":"2025-01-10 01:32:24"
},
{
"name":"FEDORA-2024-4b75866373",
"type":"bugfix",
"severity":"Low",
"nevra":"coreutils-9.5-11.fc41.x86_64",
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.
For dnf4 the list output contains a type (bugfix, enhancement, security..) and when the type is security it also contains the severity (Low, Moderate, Important...) so it combines them both.
With dnf5 json both type and severity are always present but they are separated. I think this is better for the json output.
| advisories = set() | ||
| for apkg, advisory, installed in apkg_adv_insts: | ||
| advisories.add(advisory2info(advisory, installed)) | ||
| dt_advisories.update(self._process_advisory(advisory)) |
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 would prefer if we did this only when the self.opts.json is set otherwise its running always even when not used.
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.
ack.
I think you can make it work, just remove the backslashes but keep the indent: 'Vendor': (getattr(advisory, 'vendor', None)
or getattr(advisory, 'author', None) |
Yes, I agree, I am only thinking if it makes sense to show the field if it will always be empty. |
100%;) let me remove them and resubmit. |
This is a backporting for the feature introduced to dnf5 by the following pull requests: - rpm-software-management/dnf5#1531 - rpm-software-management/dnf5#1970 The feature enables JSON format output for updateinfo command.
ack;) uh, |
6119ab0 to
9b59c70
Compare
removed. tests re-triggered. |
|
oh, spec got updated? |
looks they were removed from I'll go with the code change for now;) |
Yea, though I don't think you need to rebase. If you do need the new libdnf there are builds in https://copr.fedorainfracloud.org/coprs/rpmsoftwaremanagement/dnf-nightly/ just be aware there is also dnf5 in there. |
This is a backporting for the feature introduced to dnf5 by the following pull requests:
The feature enables JSON format output for updateinfo command.