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

lib: Fix ListingTable nesting for non-expandable rows #19520

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

martinpitt
Copy link
Member

<Tr> always needs to be wrapped into a <Tbody>.

Clean up the unnecessary special cases and handle them uniformly.

Adjust the tests to fix the selectors, they previously relied on the
buggy behaviour.


Split out from PR #19509, this is already a very intrusive fix which should land separately.

@martinpitt
Copy link
Member Author

The other pixel changes are correct -- these are now tbody's which look like separate "cards", as opposed to the adjoined "expandable row" ones.

Same for the storage ones, so I'll accept them.

@martinpitt martinpitt marked this pull request as ready for review October 24, 2023 08:19
@martinpitt
Copy link
Member Author

martinpitt commented Oct 24, 2023

Argh, what a yak shave -- NFS kdump is flaky on RHEL 8.9 and RHEL 9.3. It's not new, it's a flake , but 3x affected retries expose it. On my todo list..

@martinpitt
Copy link
Member Author

martinpitt commented Oct 24, 2023

I can locally reproduce the NFS kdump failure rather well (about 1 in 2 runs). When that happens, the NFS server indeed does not have /srv/kdump/dumps/10.111.113.1* but it does have /srv/kdump/var/crash/10.111.113.1-2023-10-24-07:29:14/vmcore -- but I think that's still from the previous test which covers the default path.

The config is correct:

nfs 10.111.113.2:/srv/kdump
path dumps

and the dialog also confirms that it got the right path.

We recently had cockpit-project/bots#5263 which came and go, and bz wasn't touched by anyone except us. That seems related..

The super-annoying thing here is that this can only really be debugged with virsh console, as the kdump boot doesn't go into the journal. But as soon as virsh console is running, the bug goes away entirely, at least locally. 🤯

I added an extra commit which will make this much easier to investigate, and also naughty -- it will now show the whole VM console on failure in the log. Now they look like this or this.

mvollmer
mvollmer previously approved these changes Oct 24, 2023
@martinpitt
Copy link
Member Author

That simplification broke two tests, will fix tomorrow morning.

The kdump boot does not appear in the journal, so it was very difficult
to investigate bugs that break kdumping. Capture the console and show it
on failures.
We already ascertained that `error` is defined at that point. Avoids a
coverage complaint.
The Kdump dialog sets this to a `<CodeBlockCode>` aka. `<pre>` element,
which cannot be a child of a `<p>`. However, we do need the `<p>` for
plain strings according to the PF docs. So do a type check and support
both cases.
`<Tr>` always needs to be wrapped into a `<Tbody>`.

Clean up the unnecessary special cases and handle them uniformly.

Adjust the tests to fix the selectors, they previously relied on the
buggy behaviour.

Also adjust the pixel tests -- tbodys are separated by some space.
@martinpitt
Copy link
Member Author

I restored a bit of the selector in content_row_action() that was important for disambiguation, that should fix the tests. I also added an extra commit to make the KdumpNFS flakes easier to debug. @mvollmer PTAL?

@@ -90,7 +90,7 @@ InlineNotification.propTypes = {
export const ModalError = ({ dialogError, dialogErrorDetail, id, isExpandable }) => {
return (
<Alert id={id} variant='danger' isInline title={dialogError} isExpandable={!!isExpandable}>
{ dialogErrorDetail && <p> {dialogErrorDetail} </p> }
{ typeof dialogErrorDetail === 'string' ? <p>{dialogErrorDetail}</p> : dialogErrorDetail }
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test. Details

@martinpitt
Copy link
Member Author

That stratis rawhide failure is entirely new and unrelated, I'll send a separate PR to fix it.

@martinpitt martinpitt merged commit 1a903b4 into cockpit-project:main Oct 25, 2023
98 of 101 checks passed
@martinpitt martinpitt deleted the listing-table-nesting branch October 25, 2023 05:57
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.

3 participants