Skip to content

kdump: Port KdumpTestDialog to Dialogs API #23421

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.

Sign up for GitHub

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

Open
jelly wants to merge 1 commit into
base: main
Choose a base branch
from
Open

kdump: Port KdumpTestDialog to Dialogs API #23421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 49 additions & 25 deletions pkg/kdump/kdump-view.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import { Tooltip } from "@patternfly/react-core/dist/esm/components/Tooltip/inde
import { useDialogs, DialogsContext } from "dialogs.jsx";
import { read_os_release } from "os-release.js";
import { fmt_to_fragments } from 'utils.jsx';
import { show_modal_dialog } from "cockpit-components-dialog.jsx";
import { FormHelper } from "cockpit-components-form-helper";
import { ModalError } from 'cockpit-components-inline-notification.jsx';
import { PrivilegedButton } from "cockpit-components-privileged";
Expand Down Expand Up @@ -316,6 +315,52 @@ const KdumpSettingsModal = ({ settings, initialTarget, handleSave }) => {
</Modal>);
};

const KdumpTestDialog = ({ verifyMessage, onCrashKernel }) => {
const Dialogs = useDialogs();
const [task, setTask] = useState(null);
const [error, setError] = useState(null);
Comment on lines +318 to +321

@cockpituous cockpituous Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These 4 added lines are not executed by any test. Details


function crash() {
setError(null);
setTask(onCrashKernel()

@mvollmer mvollmer Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't show any error in the dialog when onCrashKernel() throws an exception, right?

I think the old code didn't do that either, and would actually crash JavaScript in that case, because it doesn't provide a set_error function in the footer props. Hmm,

@mvollmer mvollmer Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the old code didn't do that either, and would actually crash JavaScript in that case, because it doesn't provide a set_error function in the footer props. Hmm,

No, show_modal_dialog sets set_error by itself. So we are loosing error reporting here.

.then(Dialogs.close)
.catch(error => {
setTask(null);
setError(error);
Comment on lines +323 to +329

@cockpituous cockpituous Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These 7 added lines are not executed by any test. Details

}));
}

return (
<Modal position="top" variant="small" id="kdump-test-dialog" isOpen
onClose={Dialogs.close}>
<ModalHeader title={_("Test kdump settings")} titleIconVariant="warning" />
<ModalBody>
{error && <ModalError dialogError={error.message || error} />}
<Content>
<Content component={ContentVariants.p}>
{_("Test kdump settings by crashing the kernel. This may take a while and the system might not automatically reboot. Do not purposefully crash the system while any important task is running.")}
</Content>
{verifyMessage && <Content component={ContentVariants.p}>
{verifyMessage}
</Content>}
</Content>
</ModalBody>
<ModalFooter>
<Button variant="danger"
isLoading={!!task}
isDisabled={!!task}
onClick={crash}>
{_("Crash system")}
</Button>
<Button variant="link"
isDisabled={!!task}
onClick={Dialogs.close}>
{_("Cancel")}
</Button>
</ModalFooter>
</Modal>);
Comment on lines +333 to +361

@cockpituous cockpituous Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These 29 added lines are not executed by any test. Details

};

/* Show kdump status of the system and offer options to change or test the state
* Expected properties:
* kdumpActive kdump service status
Expand All @@ -340,6 +385,7 @@ export class KdumpPage extends React.Component {
}

handleTestSettingsClick() {
const Dialogs = this.context;

@cockpituous cockpituous Jul 6, 2026

Copy link
Copy Markdown
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

// if we have multiple targets defined, the config is invalid
const target = this.props.kdumpStatus.target;
let verifyMessage;
Expand All @@ -359,30 +405,8 @@ export class KdumpPage extends React.Component {
}
}

// open a dialog to confirm crashing the kernel to test the settings - then do it
const dialogProps = {
title: _("Test kdump settings"),
body: (<Content>
<Content component={ContentVariants.p}>
{_("Test kdump settings by crashing the kernel. This may take a while and the system might not automatically reboot. Do not purposefully crash the system while any important task is running.")}
</Content>
{verifyMessage && <Content component={ContentVariants.p}>
{verifyMessage}
</Content>}
</Content>),
titleIconVariant: "warning",
};
// also test modifying properties in subsequent render calls
const footerProps = {
actions: [
{
clicked: this.props.onCrashKernel.bind(this),
caption: _("Crash system"),
style: 'danger',
}
],
};
show_modal_dialog(dialogProps, footerProps);
Dialogs.show(<KdumpTestDialog verifyMessage={verifyMessage}
onCrashKernel={this.props.onCrashKernel} />);
Comment on lines +408 to +409

@cockpituous cockpituous Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These 2 added lines are not executed by any test. Details

}

handleServiceDetailsClick() {
Expand Down
2 changes: 1 addition & 1 deletion test/verify/check-kdump
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class KdumpHelpers(testlib.MachineCase):
# we should get a warning dialog, confirm
browser.click(f".pf-v6-c-modal-box button{self.danger_btn_class}")
# wait until we've actually triggered a crash
browser.wait_visible(".apply.pf-m-in-progress")
browser.wait_visible("button.pf-m-in-progress")

# wait for disconnect and then try connecting again
browser.switch_to_top()
Expand Down