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.

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

feat(storage): configure boot #1808

Merged
merged 17 commits into from
Dec 20, 2024
Merged

feat(storage): configure boot #1808

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
6043d7b
fix(schema): use alias for boot device
joseivanlopez Dec 4, 2024
b5213c8
fix(storage): get boot device from alias
joseivanlopez Dec 5, 2024
eb3c5e1
chore(storage): split config checker
joseivanlopez Dec 5, 2024
02be420
feat(storage): check boot config
joseivanlopez Dec 5, 2024
35b6199
chore(storage): reorganize config solvers
joseivanlopez Dec 10, 2024
06fc6c0
feat(storage): extend boot config
joseivanlopez Dec 10, 2024
1ee52ea
feat(storage): add boot config solver
joseivanlopez Dec 11, 2024
fe10bce
fix(storage): avoid error in the old proposal settings
joseivanlopez Dec 11, 2024
e9036dc
feat(schema): add support for boot in storage model
joseivanlopez Dec 11, 2024
24ddb84
feat(storage): add boot to config model
joseivanlopez Dec 12, 2024
fe814ee
feat(web): update config-model types
joseivanlopez Dec 18, 2024
1401724
feat(web): extend and unify config model hooks
joseivanlopez Dec 18, 2024
d807efe
fix(web): adapt to new config model hooks
joseivanlopez Dec 18, 2024
cbf0d39
feat(web): allow changing boot options
joseivanlopez Dec 18, 2024
a108175
feat(storage): use proper space policy
joseivanlopez Dec 19, 2024
b12898a
fix(storage): use securerandom to generate aliases
joseivanlopez Dec 20, 2024
ce3e1e9
Fix typo
joseivanlopez Dec 20, 2024
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
2 changes: 1 addition & 1 deletion web/src/components/storage/BootConfigField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ import { STORAGE as PATHS } from "~/routes/paths";
const Link = ({ isBold = false }: { isBold?: boolean }) => {
const text = _("Change boot options");

return <RouterLink to={PATHS.bootingPartition}>{isBold ? <b>{text}</b> : text}</RouterLink>;
return <RouterLink to={PATHS.bootDevice}>{isBold ? <b>{text}</b> : text}</RouterLink>;
};

export type BootConfig = {
Expand Down
149 changes: 66 additions & 83 deletions web/src/components/storage/BootSelection.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,10 @@

import React from "react";
import { screen, within } from "@testing-library/react";
import { plainRender } from "~/test-utils";
import { mockNavigateFn, plainRender } from "~/test-utils";
import BootSelection from "./BootSelection";
import { StorageDevice } from "~/types/storage";
import { BootHook } from "~/queries/storage/config-model";

const sda: StorageDevice = {
sid: 59,
Expand Down Expand Up @@ -94,50 +95,62 @@ const sdc: StorageDevice = {
udevPaths: ["pci-0000:00-19"],
};

let props;

describe.skip("BootSelection", () => {
beforeEach(() => {
props = {
isOpen: true,
configureBoot: false,
availableDevices: [sda, sdb, sdc],
bootDevice: undefined,
defaultBootDevice: undefined,
onCancel: jest.fn(),
onAccept: jest.fn(),
};
});
const mockAvailableDevices = [sda, sdb, sdc];

const mockBoot: BootHook = {
configure: false,
isDefault: false,
deviceName: undefined,
setDevice: jest.fn(),
setDefault: jest.fn(),
disable: jest.fn(),
};

jest.mock("react-router-dom", () => ({
...jest.requireActual("react-router-dom"),
useNavigate: () => mockNavigateFn,
}));

jest.mock("~/queries/storage", () => ({
...jest.requireActual("~/queries/storage"),
useAvailableDevices: () => mockAvailableDevices,
}));

jest.mock("~/queries/storage/config-model", () => ({
...jest.requireActual("~/queries/storage/config-model"),
useBoot: () => mockBoot,
}));

describe("BootSelection", () => {
const automaticOption = () => screen.queryByRole("radio", { name: "Automatic" });
const selectDiskOption = () => screen.queryByRole("radio", { name: "Select a disk" });
const notConfigureOption = () => screen.queryByRole("radio", { name: "Do not configure" });
const diskSelector = () => screen.queryByRole("combobox", { name: /choose a disk/i });

it("offers an option to configure boot in the installation disk", () => {
plainRender(<BootSelection {...props} />);
plainRender(<BootSelection />);
expect(automaticOption()).toBeInTheDocument();
});

it("offers an option to configure boot in a selected disk", () => {
plainRender(<BootSelection {...props} />);
plainRender(<BootSelection />);
expect(selectDiskOption()).toBeInTheDocument();
expect(diskSelector()).toBeInTheDocument();
});

it("offers an option to not configure boot", () => {
plainRender(<BootSelection {...props} />);
plainRender(<BootSelection />);
expect(notConfigureOption()).toBeInTheDocument();
});

describe("if the current value is set to boot from the installation disk", () => {
beforeEach(() => {
props.configureBoot = true;
props.bootDevice = undefined;
mockBoot.configure = true;
mockBoot.isDefault = true;
});

it("selects 'Automatic' option by default", () => {
plainRender(<BootSelection {...props} />);
plainRender(<BootSelection />);
expect(automaticOption()).toBeChecked();
expect(selectDiskOption()).not.toBeChecked();
expect(diskSelector()).toBeDisabled();
Expand All @@ -147,12 +160,13 @@ describe.skip("BootSelection", () => {

describe("if the current value is set to boot from a selected disk", () => {
beforeEach(() => {
props.configureBoot = true;
props.bootDevice = sdb;
mockBoot.configure = true;
mockBoot.isDefault = false;
mockBoot.deviceName = sda.name;
});

it("selects 'Select a disk' option by default", () => {
plainRender(<BootSelection {...props} />);
plainRender(<BootSelection />);
expect(automaticOption()).not.toBeChecked();
expect(selectDiskOption()).toBeChecked();
expect(diskSelector()).toBeEnabled();
Expand All @@ -162,91 +176,60 @@ describe.skip("BootSelection", () => {

describe("if the current value is set to not configure boot", () => {
beforeEach(() => {
props.configureBoot = false;
props.bootDevice = sdb;
mockBoot.configure = false;
});

it("selects 'Do not configure' option by default", () => {
plainRender(<BootSelection {...props} />);
plainRender(<BootSelection />);
expect(automaticOption()).not.toBeChecked();
expect(selectDiskOption()).not.toBeChecked();
expect(diskSelector()).toBeDisabled();
expect(notConfigureOption()).toBeChecked();
});
});

it("does not call onAccept on cancel", async () => {
const { user } = plainRender(<BootSelection {...props} />);
it("does not change the boot options on cancel", async () => {
const { user } = plainRender(<BootSelection />);
const cancel = screen.getByRole("button", { name: "Cancel" });

await user.click(cancel);

expect(props.onAccept).not.toHaveBeenCalled();
expect(mockBoot.setDevice).not.toHaveBeenCalled();
expect(mockBoot.setDefault).not.toHaveBeenCalled();
expect(mockBoot.disable).not.toHaveBeenCalled();
});

describe("if the 'Automatic' option is selected", () => {
beforeEach(() => {
props.configureBoot = false;
props.bootDevice = undefined;
});

it("calls onAccept with the selected options on accept", async () => {
const { user } = plainRender(<BootSelection {...props} />);

await user.click(automaticOption());
it("applies the expected boot options when 'Automatic' is selected", async () => {
const { user } = plainRender(<BootSelection />);
await user.click(automaticOption());

const accept = screen.getByRole("button", { name: "Confirm" });
await user.click(accept);
const accept = screen.getByRole("button", { name: "Accept" });
await user.click(accept);

expect(props.onAccept).toHaveBeenCalledWith({
configureBoot: true,
bootDevice: undefined,
});
});
expect(mockBoot.setDefault).toHaveBeenCalled();
});

describe("if the 'Select a disk' option is selected", () => {
beforeEach(() => {
props.configureBoot = false;
props.bootDevice = undefined;
});
it("applies the expected boot options when a disk is selected", async () => {
const { user } = plainRender(<BootSelection />);

it("calls onAccept with the selected options on accept", async () => {
const { user } = plainRender(<BootSelection {...props} />);
await user.click(selectDiskOption());
const selector = diskSelector();
const sdbOption = within(selector).getByRole("option", { name: /sdb/ });
await user.selectOptions(selector, sdbOption);

await user.click(selectDiskOption());
const selector = diskSelector();
const sdbOption = within(selector).getByRole("option", { name: /sdb/ });
await user.selectOptions(selector, sdbOption);
const accept = screen.getByRole("button", { name: "Accept" });
await user.click(accept);

const accept = screen.getByRole("button", { name: "Confirm" });
await user.click(accept);

expect(props.onAccept).toHaveBeenCalledWith({
configureBoot: true,
bootDevice: sdb,
});
});
expect(mockBoot.setDevice).toHaveBeenCalledWith(sdb.name);
});

describe("if the 'Do not configure' option is selected", () => {
beforeEach(() => {
props.configureBoot = true;
props.bootDevice = sda;
});

it("calls onAccept with the selected options on accept", async () => {
const { user } = plainRender(<BootSelection {...props} />);
it("applies the expected boot options when 'No configure' is selected", async () => {
const { user } = plainRender(<BootSelection />);
await user.click(notConfigureOption());

await user.click(notConfigureOption());
const accept = screen.getByRole("button", { name: "Accept" });
await user.click(accept);

const accept = screen.getByRole("button", { name: "Confirm" });
await user.click(accept);

expect(props.onAccept).toHaveBeenCalledWith({
configureBoot: false,
bootDevice: undefined,
});
});
expect(mockBoot.disable).toHaveBeenCalled();
});
});
64 changes: 35 additions & 29 deletions web/src/components/storage/BootSelection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,11 @@ import { DevicesFormSelect } from "~/components/storage";
import { Page } from "~/components/core";
import { deviceLabel } from "~/components/storage/utils";
import { StorageDevice } from "~/types/storage";
import { useAvailableDevices, useProposalMutation, useProposalResult } from "~/queries/storage";
import { useAvailableDevices } from "~/queries/storage";
import textStyles from "@patternfly/react-styles/css/utilities/Text/text";
import { sprintf } from "sprintf-js";
import { _ } from "~/i18n";
import { useBoot } from "~/queries/storage/config-model";

// FIXME: improve classNames
// FIXME: improve and rename to BootSelectionDialog
Expand All @@ -39,50 +40,49 @@ const BOOT_AUTO_ID = "boot-auto";
const BOOT_MANUAL_ID = "boot-manual";
const BOOT_DISABLED_ID = "boot-disabled";

type BootSelectionState = {
load: boolean;
selectedOption?: string;
configureBoot?: boolean;
bootDevice?: StorageDevice;
defaultBootDevice?: StorageDevice;
availableDevices?: StorageDevice[];
};

/**
* Allows the user to select the boot configuration.
*/
export default function BootSelectionDialog() {
type BootSelectionState = {
load: boolean;
selectedOption?: string;
configureBoot?: boolean;
bootDevice?: StorageDevice;
defaultBootDevice?: StorageDevice;
availableDevices?: StorageDevice[];
};

const [state, setState] = useState<BootSelectionState>({ load: false });
const { settings } = useProposalResult();
const availableDevices = useAvailableDevices();
const updateProposal = useProposalMutation();
const navigate = useNavigate();
const boot = useBoot();

useEffect(() => {
if (state.load) return;

let selectedOption: string;
const { bootDevice, configureBoot, defaultBootDevice } = settings;

if (!configureBoot) {
if (!boot.configure) {
selectedOption = BOOT_DISABLED_ID;
} else if (configureBoot && bootDevice === "") {
} else if (boot.isDefault) {
selectedOption = BOOT_AUTO_ID;
} else {
selectedOption = BOOT_MANUAL_ID;
}

const findDevice = (name: string) => availableDevices.find((d) => d.name === name);
const bootDevice = availableDevices.find((d) => d.name === boot.deviceName);
const defaultBootDevice = boot.isDefault ? bootDevice : undefined;

setState({
load: true,
bootDevice: findDevice(bootDevice) || findDevice(defaultBootDevice) || availableDevices[0],
configureBoot,
defaultBootDevice: findDevice(defaultBootDevice),
bootDevice: bootDevice || availableDevices[0],
configureBoot: boot.configure,
defaultBootDevice,
availableDevices,
selectedOption,
});
}, [availableDevices, settings, state.load]);
}, [availableDevices, boot, state.load]);

if (!state.load) return;

Expand All @@ -92,12 +92,18 @@ export default function BootSelectionDialog() {
// const formData = new FormData(e.target);
// const mode = formData.get("bootMode");
// const device = formData.get("bootDevice");
const newSettings = {
configureBoot: state.selectedOption !== BOOT_DISABLED_ID,
bootDevice: state.selectedOption === BOOT_MANUAL_ID ? state.bootDevice.name : undefined,
};

await updateProposal.mutateAsync({ ...settings, ...newSettings });
switch (state.selectedOption) {
case BOOT_DISABLED_ID:
boot.disable();
break;
case BOOT_AUTO_ID:
boot.setDefault();
break;
default:
boot.setDevice(state.bootDevice?.name);
}

navigate("..");
};

Expand Down Expand Up @@ -126,20 +132,20 @@ partitions in the appropriate disk.",
setState({ ...state, selectedOption: e.target.value });
};

const setBootDevice = (v) => {
const changeBootDevice = (v) => {
setState({ ...state, bootDevice: v });
};

return (
<Page>
<Page.Header>
<h2>{_("Select booting partition")}</h2>
<h2>{_("Boot options")}</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

A question for @dgdavid

When we navigate to page for defining a custom policy to find space, the title and subtitles are

Find space in /dev/sda
Select what to do with each partition bla bla

When we navigate to the boot options, they are:

Boot options
To ensure the new system is able to boot, the installer may need to create bla bla

Both are fine to me. But I wonder whether they should follow a more similar schema. I mean, the first one is explicitly exhorting the user to do something while the other is explaining where we are and what it means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, this is not a blocker for merging. Let's do a demo at the next review meeting that discuss those details afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm more for going to the second approach: let the user know where they are and what can do there.

That said, unless I have not strong opinion right now, it looks perfectly fine to me to now follow/force an schema just because. If it is better to use a different approach in a very specific page, why not? It will be better than providing nor enough informatio.

In any case, I fully agree with you on not blocking the PR for this and letting titles evolve with the rest of development. Do not forget that most probably Agama will end up getting breadcrumbs at some point, which can make us to rethink these texts.

<p className={textStyles.color_400}>{description}</p>
</Page.Header>

<Page.Content>
<Form id="bootSelectionForm" onSubmit={onSubmit}>
<Page.Section>
<Page.Section aria-label={_("Select a boot option")}>
<FormGroup isStack>
<Radio
name="bootMode"
Expand Down Expand Up @@ -183,7 +189,7 @@ partitions in the appropriate disk.",
name="bootDevice"
devices={state?.availableDevices || []}
selectedDevice={state.bootDevice}
onChange={setBootDevice}
onChange={changeBootDevice}
isDisabled={state.selectedOption !== BOOT_MANUAL_ID}
/>
</Stack>
Expand Down
Loading
Loading