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

ci: Run cockpit-machines tests in PRs #2235

Draft
wants to merge 1 commit into
base: rawhide
Choose a base branch
from

Conversation

zpytela
Copy link
Contributor

@zpytela zpytela commented Jul 15, 2024

No description provided.

Copy link

Cockpit tests failed for commit 928ad3f. @martinpitt, @jelly, @mvollmer please check.

@martinpitt
Copy link
Contributor

hey @zpytela ! Argh, just today someone broke libvirt in rawhide -- we just started to see that failure in c-machines PRs as well, first occurrence in cockpit-project/cockpit-machines#1724. I'll take a look.

If you don't want to get hurt by the rawhide madness so much -- perhaps we can fix up the packaging/testing in Fedora 40, and run that?

@zpytela
Copy link
Contributor Author

zpytela commented Jul 15, 2024

@martinpitt libvirt support in F40 needs backporting of a lot of known commits, to say nothing about the not-fixed problems yet

i don't rush on this pr

@martinpitt
Copy link
Contributor

Or actually, is it unrelated? All tests fail during setUp() in

error: Failed to get libvirt version from the dbus API: {"problem":null,"name":"org.libvirt.Error","message":"Failed to connect socket to '/var/run/libvirt/virtqemud-sock': Permission denied"}

That's exactly the same as the recent RHEL regression: https://issues.redhat.com/browse/RHEL-46893 -- and a new selinux-policy version landed in https://bodhi.fedoraproject.org/updates/FEDORA-2024-5025a99508

So that test just catches this?

@zpytela
Copy link
Contributor Author

zpytela commented Jul 15, 2024

i'd not call it a regression, it was just a first step expected not to be complete

@martinpitt
Copy link
Contributor

If you mean the failure here, that's not a "regression" for this PR, right. If you mean the upload of
selinux-policy-41.8-4 (i.e. https://issues.redhat.com/browse/RHEL-46893): that is a major regression. I filed https://bugzilla.redhat.com/show_bug.cgi?id=2297965 to track it in rawhide.

Copy link

Cockpit tests failed for commit e3f5d6d. @martinpitt, @jelly, @mvollmer please check.

@martinpitt
Copy link
Contributor

Thanks for the rebase, this got a lot further! There's just something wrong -- The /network and /storage plans were not intended to run. Instead, tmt ran these three plans as sub-tests of a single "basic" plan. Cockpit's three plans OTOH ran just fine.

Comparing the existing cockpit.fmf plan against the proposed machines plan, I see these differences:

  • The plan file name is "cockpit-machines-basic.fmf", which is a bit redundant. It should be "cockpit-machines.fmf". However, I think that's just a cosmetical issue.
  • There should be a top-level "enabled: false" in the machines plan, so that the conditional enabling actually works (as plans are enabled by default). However, that should only become relevant once selinux-policy grows its own "native" plan.
  • The main difference is that cockpit-machines-basic.fmf defines a test -- it directly defines require: and test:, but that information should be kept in cockpit-machines.git. It should instead define a plan:
/main:
    summary: basic functionality
    discover+:
        test: /test/browser/main

Let's try with that? @zpytela if you want I can force-push into your branch, but I won't do that without permission.

Copy link

Cockpit tests failed for commit 6eb6978. @martinpitt, @jelly, @mvollmer please check.

plans/cockpit-machines.fmf Outdated Show resolved Hide resolved
plans/cockpit-machines.fmf Outdated Show resolved Hide resolved
plans/cockpit-machines.fmf Outdated Show resolved Hide resolved
@martinpitt
Copy link
Contributor

Cool! LGTM, thanks!

Copy link
Contributor

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

I'll leave the decision whether to land this or not to you. Of course we can also disable this at any time if it's too troublesome, this shouldn't be a lifetime commitment 💍 😁

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.

2 participants