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

Add a search CLI verb and DBus API #4478

Merged
merged 1 commit into from
Jun 22, 2023

Conversation

lukewarmtemp
Copy link
Contributor

@lukewarmtemp lukewarmtemp commented Jun 20, 2023

Fixes #1877

Implemented Features:

  • Builtin search command for rpm-ostree to search for packages within the set of repos (rpm-ostree search kernel)
  • Allows multiple search terms (rpm-ostree search kernel python)
  • Allows glob patterns (rpm-ostree search *kernel)

Edge cases covered:

  • Returns from function when no packages are specified
  • Outputs No matches found. when no packages match the search query

@lukewarmtemp
Copy link
Contributor Author

Example output:

$ rpm-ostree search kernel python

===== Name & Summary Matched =====
python-ipykernel-doc : Documentation for python-ipykernel
python-metakernel-doc : Documentation for python-metakernel
python3-ipykernel : IPython Kernel for Jupyter
python3-metakernel+test : Tests for python3-metakernel
python3-metakernel-echo : A simple echo kernel for Jupyter/IPython
python3-metakernel-python : A Python kernel for Jupyter/IPython

===== Name Matched =====
python-ipykernel-doc : Documentation for python-ipykernel
python-metakernel-doc : Documentation for python-metakernel
python3-bash-kernel : Bash kernel for Jupyter
python3-ipykernel : IPython Kernel for Jupyter
python3-jupyter-c-kernel : Minimalistic C kernel for Jupyter
python3-jupyter-kernel-singular : Jupyter kernel for Singular
python3-jupyter-kernel-test : Machinery for testing Jupyter kernels via the messaging protocol
python3-metakernel+test : Tests for python3-metakernel
python3-metakernel : Metakernel for Jupyter
python3-metakernel-echo : A simple echo kernel for Jupyter/IPython
python3-metakernel-python : A Python kernel for Jupyter/IPython
python3-octave-kernel : A Jupyter kernel for Octave
python3-spyder-kernels : Jupyter kernels for the Spyder console

===== Summary Matched =====
python-ipykernel-doc : Documentation for python-ipykernel
python-metakernel-doc : Documentation for python-metakernel
python3-ipykernel : IPython Kernel for Jupyter
python3-kmod : Python module to work with kernel modules
python3-metakernel+test : Tests for python3-metakernel
python3-metakernel-echo : A simple echo kernel for Jupyter/IPython
python3-metakernel-python : A Python kernel for Jupyter/IPython
python3-uinput : Pythonic API to the Linux uinput kernel module
pythran : Ahead of Time Python compiler for numeric kernels

@lukewarmtemp lukewarmtemp self-assigned this Jun 20, 2023
@jmarrero
Copy link
Member

This looks pretty great to me at first glance. We use clang format to format the C code. Part of the failures here are that. Also we tend to squash commits too.

@lukewarmtemp lukewarmtemp force-pushed the 1877-package-search-feature branch 2 times, most recently from 5a93871 to 9405e9c Compare June 20, 2023 19:40
@lukewarmtemp lukewarmtemp force-pushed the 1877-package-search-feature branch 2 times, most recently from e6ed8a3 to 4852d27 Compare June 20, 2023 20:54
@cgwalters
Copy link
Member

It looks like you provided information for the pull request text, but not in the actual commit message which has an empty body text. On this project we're somewhat strict about commit messages. I often reference https://cbea.ms/git-commit/ but there are plenty of other guides.

How about something like this to start?

Add a `search` CLI verb and DBus API

This closes a longstanding feature request and improves compatibility with the `dnf`/`yum` CLI.

The feature set and output text intentionally matches that tool, e.g. globs like `rpm-ostree search kernel*` are supported.

Closes: https://github.com/coreos/rpm-ostree/issues/1877

For the subject, notice I changed it to use the imperative form.

I also think we should have the direct Closes: link in the body text.

As far as the rest of the body, what you had there was OK, I just felt it was a bit more direct and informative to say basically "we're doing what dnf does".

@cgwalters
Copy link
Member

To elaborate a bit on why we do this - it's because it's highly likely that a person who didn't write the code will need to debug it potentially years later. And context that might have seemed obvious at the time is likely not anymore. It's helpful to know what the person was thinking - the why of the commit, not just the what.

Here's a random example of a commit from this project over 9 years ago: f8ddf38

Notice I added a bit of context there: "it's not going to be practical
to change every RPM right now though. " - that's the "why" that's important.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Really nice work here overall!

src/app/rpmostree-pkg-builtins.cxx Outdated Show resolved Hide resolved
src/app/rpmostree-pkg-builtins.cxx Outdated Show resolved Hide resolved
src/app/rpmostree-pkg-builtins.cxx Show resolved Hide resolved
src/daemon/rpmostreed-os.cxx Outdated Show resolved Hide resolved
src/daemon/rpmostreed-os.cxx Outdated Show resolved Hide resolved
src/daemon/rpmostreed-os.cxx Show resolved Hide resolved
src/daemon/rpmostreed-os.cxx Outdated Show resolved Hide resolved
src/daemon/rpmostreed-os.cxx Outdated Show resolved Hide resolved
src/daemon/rpmostreed-os.cxx Outdated Show resolved Hide resolved
@lukewarmtemp
Copy link
Contributor Author

To elaborate a bit on why we do this - it's because it's highly likely that a person who didn't write the code will need to debug it potentially years later. And context that might have seemed obvious at the time is likely not anymore. It's helpful to know what the person was thinking - the why of the commit, not just the what.

On a related note, I noticed that there aren't many comments within the code itself. Do you think adding comments would be helpful as well, or are we aiming for the code to be able to speak for itself?

@cgwalters
Copy link
Member

On a related note, I noticed that there aren't many comments within the code itself. Do you think adding comments would be helpful as well, or are we aiming for the code to be able to speak for itself?

For sure we could use some more comments! I've probably historically been too "code speaks for itself" but I think we could have a bit more discipline around things like ensuring internal helper functions have a minimal doc comment at least or so.

@lukewarmtemp
Copy link
Contributor Author

lukewarmtemp commented Jun 21, 2023

Also we tend to squash commits too.

If I make changes in response to a PR review should I wait until the changes get approved before squashing? I'm thinking that it would make it easier for the reviewer to compare the differences between the two commits.

@cgwalters
Copy link
Member

cgwalters commented Jun 21, 2023

If I make changes in response to a PR review should I wait until the changes get approved before squashing? I'm thinking that it would make it easier for the reviewer to compare the differences between the two commits.

As of lately, github has a less-than obvious way to see the "interdiff" i.e. the changes between the changes. Look in the PR for "force-pushed".

So our SOP is to squash - unless the changes/fixes are big enough to really stand on their own.

@lukewarmtemp lukewarmtemp force-pushed the 1877-package-search-feature branch 2 times, most recently from 37bb4bc to c593800 Compare June 22, 2023 13:34
This closes a longstanding feature request and improves compatibility
with the `dnf`/`yum` CLI.

The feature set and output text intentionally matches that tool, e.g.
globs like `rpm-ostree search kernel*` or multi term searches like
`rpm-ostree search kernel python` are supported.

Search results per section are limited to 50 due to DBus message size
limits.

Closes: coreos#1877
@lukewarmtemp
Copy link
Contributor Author

There seems to be an error in the Container Integration test relating to a 404 error when trying to download the ignition package:

Downloading https://kojipkgs.fedoraproject.org//packages/ignition/2.15.0/2.fc38/x86_64/ignition-2.15.0-2.fc38.x86_64.rpm...failed
error: Handling argument https://kojipkgs.fedoraproject.org//packages/ignition/2.15.0/2.fc38/x86_64/ignition-2.15.0-2.fc38.x86_64.rpm: HTTP status client error (404 Not Found) for url (https://kojipkgs.fedoraproject.org//packages/ignition/2.15.0/2.fc38/x86_64/ignition-2.15.0-2.fc38.x86_64.rpm)

Any thoughts on how to go about resolving this?

@cgwalters
Copy link
Member

Maintaining build systems but particularly CI is like gardening; it requires regular maintenance, and sometimes weed will just spring up fast when you weren't ready.

I took a crack at fixing this one over here #4483

@cgwalters cgwalters changed the title 1877 package search feature Add a search CLI verb and DBus API Jun 22, 2023
@lukewarmtemp lukewarmtemp merged commit 1701971 into coreos:main Jun 22, 2023
}

return TRUE;
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: missing an empty line here at the end of the file

@travier
Copy link
Member

travier commented Jun 26, 2023

Thanks!

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

Successfully merging this pull request may close these issues.

Add feature to search for packages
4 participants