-
Notifications
You must be signed in to change notification settings - Fork 144
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
Refresh metadata cache after failed package installation #3348
Conversation
c736cd9
to
793ac8a
Compare
/packit test |
9c21ff0
to
a998cbf
Compare
@bajertom please, add a unit test for this new operation. It can be fairly trivial, and it would verify those commands work without crashing. See e.g. https://github.com/teemtee/tmt/blob/main/tests/unit/test_package_managers.py#L420 or other unit tests. To run new unit tests, the following command should work (with dev environment activated, and let's say you'd call the test $ hatch -e dev shell
$ pytest -vv --showlocals tests/unit/test_package_managers.py::test_refresh_metadata |
I've added unit tests covering the command. Since there was difference between cases in "Metadata C/cache C/created" in different distros but for the same package manager (yum) I've decided to just stick with "Metadata". Is this enough or should I catch a more specific part of the stdout in this case? Same goes for apt - it depends on whether the system is up to date or not. It's either (debian test)
before
if the system is up to date, so I decided to go for However I am lost with rpm-ostree packager. On my VM I can run
|
ff0d1c3
to
1b833b0
Compare
LGTM. It's not perfect, which is true for other test cases in that file, we're fine. Success of the command itself is a good indication things went well.
Never seen this one before, and there are other rpm-ostree tests where I'd expect this to appear. No idea so far. Can you rebase the PR? That should make the patch easier to review and play with. |
1b833b0
to
9ef2fa9
Compare
@bajertom I tried to play a bit with rpm-ostree, but no luck. @thrix @KwisatzHaderach @BeeGrech any tips, or better approach? So far I'd propose adding this, basically a no-op with a warning: diff --git a/tests/unit/test_package_managers.py b/tests/unit/test_package_managers.py
index 0655fcea..b7434ead 100644
--- a/tests/unit/test_package_managers.py
+++ b/tests/unit/test_package_managers.py
@@ -478,6 +478,15 @@ def _parametrize_test_refresh_metadata() -> \
r"rpm-ostree upgrade --check", \
'Available'
+ yield pytest.param(
+ container,
+ package_manager_class,
+ r"rpm-ostree refresh-md --force", \
+ 'Available',
+ marks=pytest.mark.skip(reason="refresh-md does not work with how tmt runs ostree container")
+ )
+
+
elif package_manager_class is tmt.package_managers.apk.Apk:
yield container, \
package_manager_class, \
diff --git a/tmt/package_managers/rpm_ostree.py b/tmt/package_managers/rpm_ostree.py
index 97224202..b37532c3 100644
--- a/tmt/package_managers/rpm_ostree.py
+++ b/tmt/package_managers/rpm_ostree.py
@@ -101,10 +101,20 @@ class RpmOstree(tmt.package_managers.PackageManager):
return extra_options
def refresh_metadata(self) -> CommandOutput:
- script = ShellScript(
- f'{self.command.to_script()} upgrade --check')
-
- return self.guest.execute(script)
+ self.guest.warn("Metadata refresh is not supported with rpm-ostree.")
+
+ return CommandOutput(stdout=None, stderr=None)
+
+ # The following should work, but it hits some ostree issue:
+ #
+ # System has not been booted with systemd as init system (PID 1). Can't operate.
+ # Failed to connect to bus: Host is down
+ # System has not been booted with systemd as init system (PID 1). Can't operate.
+ # Failed to connect to bus: Host is down
+ # error: Loading sysroot: exit status: 1
+ #
+ # script = ShellScript(f'{self.command.to_script()} refresh-md --force')
+ # return self.guest.execute(script)
def install(
self, |
As I already mentioned in my comment thread above, you can refresh with dnf, but it would look weird I have no problem with the above approach and don't have a better one apart from ^ |
e134228
to
104050f
Compare
LGTM, no other ideas aside from what has been mentioned |
Thanks for the input. Let's make it a no-op for now, with a warning, and we can get back to it later. I guess the way how tmt runs ostree containers could be improved, enabling the feature eventually. |
104050f
to
65ebaad
Compare
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.
Thanks for improving this!
Failing test is a network glitch, merging. |
Fixes #3277
When testing I found out that there is different behavior in dnf5 compared to dnf4, where a mere
dnf makecache
would be enough, but dnf5 needs the --refresh option as well (ordnf clean metadata
ordnf clean expire-cache
), see https://bugzilla.redhat.com/show_bug.cgi?id=2324177Not sure about the Exception being too generic. Should I create a new one, something like InstallationError? And notify the user through logger as to why
metadata --refresh
is happening?Pull Request Checklist