Skip to content
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

Updateinfo direct command and verbose list (RhBug:1801092) #1607

Closed

Conversation

kontura
Copy link
Contributor

@kontura kontura commented Mar 20, 2020

This changes the output of updateinfo list command in verbose mode, while someone might depend on that I think its fine because the original behavior is still present in non verbose mode.

(The first commit also fixes a bug with direct commands)

Tests:
rpm-software-management/ci-dnf-stack#809

https://bugzilla.redhat.com/show_bug.cgi?id=1801092

@lukash lukash self-assigned this Mar 31, 2020
@lukash
Copy link
Contributor

lukash commented Mar 31, 2020

We've discussed this before and it is perhaps subjective, but I'm not convinced hooking additional output data into -v is a good solution. -v becomes a catch-all for anything being added to the output and lacks clean definition of what it actually adds.

Would --with-date be a better option for this?

@kontura
Copy link
Contributor Author

kontura commented Apr 1, 2020

It probably is subjective, I would rather have catch-all -v than several other really specific options. To me adding an option to dnf is not justified by this simple column and -v is fine as long as its mentioned in the docs for the specific command, which admittedly I forgot here.

Nevertheless I am fine with --with-date if we have such a policy against using -v.

@lukash
Copy link
Contributor

lukash commented Apr 3, 2020

Apart from the missing documentation LGTM...

@kontura
Copy link
Contributor Author

kontura commented Apr 6, 2020

Updated the docs.

@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/rpm-software-management-dnf-1607
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@lukash
Copy link
Contributor

lukash commented Apr 6, 2020

Didn't this have another commit that was removing a [0] from the command name check for the aliases of the command? Or am I misremembering?

You've rebased while pushing the last change so I can't check the diff there...

@m-blaha
Copy link
Member

m-blaha commented Apr 6, 2020

@lukash [0] was removed as part of this PR #1611

@lukash
Copy link
Contributor

lukash commented Apr 6, 2020

@m-blaha I see, thanks!

@lukash
Copy link
Contributor

lukash commented Apr 6, 2020

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit 85b10ee has been approved by lukash

@rh-atomic-bot
Copy link

⌛ Testing commit 85b10ee with merge ae8c504...

Copy link
Contributor

@lukash lukash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually forgot to review the doc update... 🙂

information (``--info``). When the ``-v`` option is used with ``--info``, the
information is even more detailed.
information (``--info``). The ``-v`` option extends the output. When
used with ``--info`` the information is even more detailed. When used
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... used with --info, the information is ...

(Add a comma)

information is even more detailed.
information (``--info``). The ``-v`` option extends the output. When
used with ``--info`` the information is even more detailed. When used
with ``--list`` additional column with date of last advisory update is added.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... with --list, an additional column with date of the last advisory update is added.

@rh-atomic-bot
Copy link

☀️ Test successful - status-papr
Approved by: lukash
Pushing ae8c504 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants