diff --git a/service/lib/agama/storage/config.rb b/service/lib/agama/storage/config.rb index 3df7501307..daf7560099 100644 --- a/service/lib/agama/storage/config.rb +++ b/service/lib/agama/storage/config.rb @@ -72,15 +72,24 @@ def boot_device boot_drive&.found_device&.name end - # return [Array] + # @return [Array] def partitions drives.flat_map(&:partitions) end - # return [Array] + # @return [Array] def logical_volumes volume_groups.flat_map(&:logical_volumes) end + + # @return [Array] + def filesystems + ( + drives.map(&:filesystem) + + partitions.map(&:filesystem) + + logical_volumes.map(&:filesystem) + ).compact + end end end end diff --git a/service/lib/agama/storage/config_checker.rb b/service/lib/agama/storage/config_checker.rb index 9cefb995f4..b6f04d3967 100644 --- a/service/lib/agama/storage/config_checker.rb +++ b/service/lib/agama/storage/config_checker.rb @@ -21,6 +21,7 @@ require "agama/config" require "agama/storage/config_checkers/boot" +require "agama/storage/config_checkers/filesystems" require "agama/storage/config_checkers/drive" require "agama/storage/config_checkers/volume_group" require "agama/storage/config_checkers/volume_groups" @@ -41,6 +42,7 @@ def initialize(storage_config, product_config = nil) # @return [Array] def issues [ + filesystems_issues, boot_issues, drives_issues, volume_groups_issues @@ -62,6 +64,13 @@ def boot_issues ConfigCheckers::Boot.new(storage_config, product_config).issues end + # Issues related to the list of filesystems (mount paths) + # + # @return [Array] + def filesystems_issues + ConfigCheckers::Filesystems.new(storage_config, product_config).issues + end + # Issues from drives. # # @return [Array] diff --git a/service/lib/agama/storage/config_checkers/base.rb b/service/lib/agama/storage/config_checkers/base.rb index a55397236c..6513253399 100644 --- a/service/lib/agama/storage/config_checkers/base.rb +++ b/service/lib/agama/storage/config_checkers/base.rb @@ -51,13 +51,16 @@ def issues # Creates an error issue. # # @param message [String] + # @param kind [Symbol, nil] if nil or ommited, default value defined by Agama::Issue # @return [Issue] - def error(message) - Agama::Issue.new( - message, + def error(message, kind: nil) + issue_args = { source: Agama::Issue::Source::CONFIG, severity: Agama::Issue::Severity::ERROR - ) + } + issue_args[:kind] = kind if kind + + Agama::Issue.new(message, **issue_args) end end end diff --git a/service/lib/agama/storage/config_checkers/boot.rb b/service/lib/agama/storage/config_checkers/boot.rb index 013385d79c..41f3f5c5ed 100644 --- a/service/lib/agama/storage/config_checkers/boot.rb +++ b/service/lib/agama/storage/config_checkers/boot.rb @@ -60,7 +60,8 @@ def missing_alias_issue # solver logic changes. error( _("The boot device cannot be automatically selected because there is no root (/) " \ - "file system") + "file system"), + kind: :no_root ) end @@ -69,7 +70,10 @@ def invalid_alias_issue return unless configure? && device_alias && !valid_alias? # TRANSLATORS: %s is replaced by a device alias (e.g., "boot"). - error(format(_("There is no boot device with alias '%s'"), device_alias)) + error( + format(_("There is no boot device with alias '%s'"), device_alias), + kind: :no_such_alias + ) end # @return [Boolean] diff --git a/service/lib/agama/storage/config_checkers/encryption.rb b/service/lib/agama/storage/config_checkers/encryption.rb index 95590623e9..55da2d0cc5 100644 --- a/service/lib/agama/storage/config_checkers/encryption.rb +++ b/service/lib/agama/storage/config_checkers/encryption.rb @@ -64,6 +64,11 @@ def encryption config.encryption end + # @see Base + def error(message) + super(message, kind: :encryption) + end + # @return [Issue, nil] def missing_password_issue return unless encryption.missing_password? diff --git a/service/lib/agama/storage/config_checkers/filesystem.rb b/service/lib/agama/storage/config_checkers/filesystem.rb index f1f87fe122..6501168473 100644 --- a/service/lib/agama/storage/config_checkers/filesystem.rb +++ b/service/lib/agama/storage/config_checkers/filesystem.rb @@ -62,12 +62,17 @@ def filesystem config.filesystem end + # @see Base + def error(message) + super(message, kind: :filesystem) + end + # @return [Issue, nil] def missing_filesystem_issue return if filesystem.reuse? return if filesystem.type&.fs_type - # TRANSLATORS: %s is the replaced by a mount path (e.g., "/home"). + # TRANSLATORS: %s is replaced by a mount path (e.g., "/home"). error(format(_("Missing file system type for '%s'"), filesystem.path)) end diff --git a/service/lib/agama/storage/config_checkers/filesystems.rb b/service/lib/agama/storage/config_checkers/filesystems.rb new file mode 100644 index 0000000000..2e920100e1 --- /dev/null +++ b/service/lib/agama/storage/config_checkers/filesystems.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +# Copyright (c) [2025] SUSE LLC +# +# All Rights Reserved. +# +# This program is free software; you can redistribute it and/or modify it +# under the terms of version 2 of the GNU General Public License as published +# by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along +# with this program; if not, contact SUSE LLC. +# +# To contact SUSE LLC about this file by physical or electronic mail, you may +# find current contact information at www.suse.com. + +require "agama/storage/config_checkers/base" +require "agama/storage/proposal_settings_reader" +require "yast/i18n" + +module Agama + module Storage + module ConfigCheckers + # Class for checking the overal configuration of filesystems. + class Filesystems < Base + include Yast::I18n + + # Issues related to the configured set of filesystems. + # + # @return [Array] + def issues + [missing_paths_issue].compact + end + + private + + # @return [Issue, nil] + def missing_paths_issue + missing_paths = mandatory_paths.reject { |p| configured_path?(p) } + return if missing_paths.empty? + + error( + format( + # TRANSLATORS: %s is a path like "/" or a list of paths separated by commas + n_( + "A separate file system for %s is required.", + "Separate file systems are required for the following paths: %s", + missing_paths.size + ), + missing_paths.join(", ") + ), + kind: :required_filesystems + ) + end + + # @return [Boolean] + def configured_path?(path) + filesystems.any? { |fs| fs.path?(path) } + end + + # @return [Array] + def filesystems + @filesystems ||= storage_config.filesystems + end + + # @return [Array] + def mandatory_paths + product_config.mandatory_paths + end + end + end + end +end diff --git a/service/lib/agama/storage/config_checkers/logical_volume.rb b/service/lib/agama/storage/config_checkers/logical_volume.rb index 63212855db..2d66ec0e21 100644 --- a/service/lib/agama/storage/config_checkers/logical_volume.rb +++ b/service/lib/agama/storage/config_checkers/logical_volume.rb @@ -74,8 +74,11 @@ def missing_thin_pool_issue return if pool - # TRANSLATORS: %s is the replaced by a device alias (e.g., "pv1"). - error(format(_("There is no LVM thin pool volume with alias '%s'"), config.used_pool)) + error( + # TRANSLATORS: %s is the replaced by a device alias (e.g., "pv1"). + format(_("There is no LVM thin pool volume with alias '%s'"), config.used_pool), + kind: :no_such_alias + ) end end end diff --git a/service/lib/agama/storage/config_checkers/search.rb b/service/lib/agama/storage/config_checkers/search.rb index 5d8ea9b1e4..ae9b7a6f73 100644 --- a/service/lib/agama/storage/config_checkers/search.rb +++ b/service/lib/agama/storage/config_checkers/search.rb @@ -61,6 +61,11 @@ def search config.search end + # @see Base + def error(message) + super(message, kind: :search) + end + # @return [Issue, nil] def not_found_issue return if search.device || search.skip_device? diff --git a/service/lib/agama/storage/config_checkers/volume_group.rb b/service/lib/agama/storage/config_checkers/volume_group.rb index cbcae6025c..50e91b95a4 100644 --- a/service/lib/agama/storage/config_checkers/volume_group.rb +++ b/service/lib/agama/storage/config_checkers/volume_group.rb @@ -84,8 +84,11 @@ def missing_physical_volume_issue(pv_alias) configs = storage_config.drives + storage_config.drives.flat_map(&:partitions) return if configs.any? { |c| c.alias == pv_alias } - # TRANSLATORS: %s is the replaced by a device alias (e.g., "pv1"). - error(format(_("There is no LVM physical volume with alias '%s'"), pv_alias)) + error( + # TRANSLATORS: %s is the replaced by a device alias (e.g., "pv1"). + format(_("There is no LVM physical volume with alias '%s'"), pv_alias), + kind: :no_such_alias + ) end # Issues from physical volumes devices (target devices). @@ -109,7 +112,8 @@ def missing_physical_volumes_device_issue(device_alias) # TRANSLATORS: %s is the replaced by a device alias (e.g., "disk1"). _("There is no target device for LVM physical volumes with alias '%s'"), device_alias - ) + ), + kind: :no_such_alias ) end diff --git a/service/lib/agama/storage/config_checkers/volume_groups.rb b/service/lib/agama/storage/config_checkers/volume_groups.rb index 4113deb481..5d4e621617 100644 --- a/service/lib/agama/storage/config_checkers/volume_groups.rb +++ b/service/lib/agama/storage/config_checkers/volume_groups.rb @@ -54,7 +54,8 @@ def overused_physical_volumes_devices_issues # TRANSLATORS: %s is the replaced by a device alias (e.g., "disk1"). _("The device '%s' is used several times as target device for physical volumes"), device - ) + ), + kind: :vg_target_devices ) end end diff --git a/service/lib/agama/storage/proposal.rb b/service/lib/agama/storage/proposal.rb index 05f5f7593d..b0244397e7 100644 --- a/service/lib/agama/storage/proposal.rb +++ b/service/lib/agama/storage/proposal.rb @@ -368,7 +368,7 @@ def storage_manager # @return [Issue] def failed_issue Issue.new( - _("Cannot accommodate the required file systems for installation"), + _("Cannot calculate a valid storage setup with the current configuration"), source: Issue::Source::SYSTEM, severity: Issue::Severity::ERROR ) diff --git a/service/lib/y2storage/agama_proposal.rb b/service/lib/y2storage/agama_proposal.rb index ad367483d5..3d0594d2f3 100644 --- a/service/lib/y2storage/agama_proposal.rb +++ b/service/lib/y2storage/agama_proposal.rb @@ -103,7 +103,6 @@ def calculate_proposal issues_list.concat(issues) if fatal_error? - # This means some IfNotFound is set to "error" and we failed to find a match @devices = nil return @devices end diff --git a/service/test/agama/storage/autoyast_proposal_test.rb b/service/test/agama/storage/autoyast_proposal_test.rb index 78ebeb75b5..085a110bea 100644 --- a/service/test/agama/storage/autoyast_proposal_test.rb +++ b/service/test/agama/storage/autoyast_proposal_test.rb @@ -247,7 +247,7 @@ def root_filesystem(disk) subject.calculate_autoyast(partitioning) expect(subject.issues).to include( an_object_having_attributes( - description: /Cannot accommodate/, severity: Agama::Issue::Severity::ERROR + description: /Cannot calculate/, severity: Agama::Issue::Severity::ERROR ) ) end @@ -375,7 +375,7 @@ def root_filesystem(disk) subject.calculate_autoyast(partitioning) expect(subject.issues).to include( an_object_having_attributes( - description: /Cannot accommodate/, severity: Agama::Issue::Severity::ERROR + description: /Cannot calculate/, severity: Agama::Issue::Severity::ERROR ) ) end diff --git a/service/test/agama/storage/config_checker_test.rb b/service/test/agama/storage/config_checker_test.rb index b675e68b28..106ecacd37 100644 --- a/service/test/agama/storage/config_checker_test.rb +++ b/service/test/agama/storage/config_checker_test.rb @@ -231,17 +231,24 @@ let(:product_data) do { "storage" => { - "volume_templates" => [ - { - "mount_path" => "/", - "filesystem" => "btrfs", - "outline" => { "filesystems" => ["btrfs", "xfs"] } - } - ] + "volumes" => volumes, + "volume_templates" => volume_templates } } end + let(:volumes) { ["/"] } + + let(:volume_templates) do + [ + { + "mount_path" => "/", + "filesystem" => "btrfs", + "outline" => { "filesystems" => ["btrfs", "xfs"] } + } + ] + end + before do mock_storage(devicegraph: scenario) # To speed-up the tests. Use #allow_any_instance because #allow introduces marshaling problems @@ -785,6 +792,56 @@ end end + context "if some volumes are required" do + let(:volumes) { ["/", "swap"] } + + let(:volume_templates) do + [ + { + "mount_path" => "/", + "filesystem" => "btrfs", + "outline" => { "required" => true } + }, + { + "mount_path" => "swap", + "filesystem" => "swap", + "outline" => { "required" => true } + } + ] + end + + context "and one of them is omitted at the configuration" do + let(:config_json) do + { + drives: [ + { + partitions: [ + { filesystem: { path: "swap" } } + ] + } + ] + } + end + + it "includes an issue for the missing mount path" do + issues = subject.issues + expect(issues).to include an_object_having_attributes( + error?: true, + kind: :required_filesystems, + description: /file system for \/ is/ + ) + end + + it "does not include an issue for the present mount path" do + issues = subject.issues + expect(issues).to_not include an_object_having_attributes( + kind: :required_filesystems, + description: /file system for swap/ + ) + end + end + end + context "if the config has several issues" do let(:config_json) do { diff --git a/service/test/agama/storage/proposal_test.rb b/service/test/agama/storage/proposal_test.rb index dfe6246804..cb40631d90 100644 --- a/service/test/agama/storage/proposal_test.rb +++ b/service/test/agama/storage/proposal_test.rb @@ -917,7 +917,7 @@ def drive(partitions) subject.calculate_agama(config) expect(subject.issues).to include( - an_object_having_attributes(description: /Cannot accommodate/) + an_object_having_attributes(description: /Cannot calculate/) ) end end diff --git a/service/test/y2storage/agama_proposal_test.rb b/service/test/y2storage/agama_proposal_test.rb index 945e73b5ab..edff7343a2 100644 --- a/service/test/y2storage/agama_proposal_test.rb +++ b/service/test/y2storage/agama_proposal_test.rb @@ -116,7 +116,7 @@ def partition_config(name: nil, filesystem: nil, size: nil) "subvolumes" => ["home", "opt", "root", "srv"] }, "outline" => { - "required" => true, + "required" => false, "snapshots_configurable" => true, "filesystems" => ["btrfs", "xfs", "ext3", "ext4"], "auto_size" => { diff --git a/web/package/agama-web-ui.changes b/web/package/agama-web-ui.changes index 7e3b225c8e..df0bcb5178 100644 --- a/web/package/agama-web-ui.changes +++ b/web/package/agama-web-ui.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Wed Mar 19 19:04:09 UTC 2025 - Ancor Gonzalez Sosa + +- Allow temporary removal of the root file system + (gh#agama-project/agama#2160). + ------------------------------------------------------------------- Tue Mar 18 10:59:20 UTC 2025 - David Diaz diff --git a/web/src/components/storage/DriveEditor.test.tsx b/web/src/components/storage/DriveEditor.test.tsx index db24a8c89d..c7fa4a04db 100644 --- a/web/src/components/storage/DriveEditor.test.tsx +++ b/web/src/components/storage/DriveEditor.test.tsx @@ -164,6 +164,8 @@ const mockDeleteDrive = jest.fn(); const mockGetPartition = jest.fn(); const mockDeletePartition = jest.fn(); +let additionalDrives = true; + jest.mock("~/queries/storage", () => ({ ...jest.requireActual("~/queries/storage"), useAvailableDevices: () => [sda], @@ -174,11 +176,15 @@ jest.mock("~/queries/storage", () => ({ jest.mock("~/queries/storage/config-model", () => ({ ...jest.requireActual("~/queries/storage/config-model"), useConfigModel: () => ({ drives: [drive1, drive2] }), - useDrive: () => ({ + useDrive: (name) => ({ + isExplicitBoot: name === "/dev/sda", delete: mockDeleteDrive, getPartition: mockGetPartition, deletePartition: mockDeletePartition, }), + useModel: () => ({ + hasAdditionalDrives: additionalDrives, + }), })); describe("PartitionMenuItem", () => { @@ -195,16 +201,17 @@ describe("PartitionMenuItem", () => { expect(mockDeletePartition).toHaveBeenCalled(); }); - it("does not allow users to delete a required partition", async () => { + it("allows users to delete a required partition", async () => { const { user } = plainRender(); const partitionsButton = screen.getByRole("button", { name: "Partitions" }); await user.click(partitionsButton); const partitionsMenu = screen.getByRole("menu"); - const deleteRootButton = within(partitionsMenu).queryByRole("menuitem", { + const deleteRootButton = within(partitionsMenu).getByRole("menuitem", { name: "Delete /", }); - expect(deleteRootButton).not.toBeInTheDocument(); + await user.click(deleteRootButton); + expect(mockDeletePartition).toHaveBeenCalled(); }); it("allows users to edit a partition", async () => { @@ -222,28 +229,64 @@ describe("PartitionMenuItem", () => { }); describe("RemoveDriveOption", () => { - it("allows users to delete a drive which does not contain root", async () => { - const { user } = plainRender(); - - const driveButton = screen.getByRole("button", { name: "Drive" }); - await user.click(driveButton); - const drivesMenu = screen.getByRole("menu"); - const deleteDriveButton = within(drivesMenu).getByRole("menuitem", { - name: /Do not use/, + describe("if there are additional drives", () => { + beforeEach(() => { + additionalDrives = true; + }); + + it("allows users to delete regular drives", async () => { + const { user } = plainRender(); + + const driveButton = screen.getByRole("button", { name: "Drive" }); + await user.click(driveButton); + const drivesMenu = screen.getByRole("menu"); + const deleteDriveButton = within(drivesMenu).getByRole("menuitem", { + name: /Do not use/, + }); + await user.click(deleteDriveButton); + expect(mockDeleteDrive).toHaveBeenCalled(); + }); + + it("does not allow users to delete drives explicitly used to boot", async () => { + const { user } = plainRender(); + + const driveButton = screen.getByRole("button", { name: "Drive" }); + await user.click(driveButton); + const drivesMenu = screen.getByRole("menu"); + const deleteDriveButton = within(drivesMenu).queryByRole("menuitem", { + name: /Do not use/, + }); + expect(deleteDriveButton).not.toBeInTheDocument(); }); - await user.click(deleteDriveButton); - expect(mockDeleteDrive).toHaveBeenCalled(); }); - it("does not allow users to delete a drive which contains root", async () => { - const { user } = plainRender(); + describe("if there are no additional drives", () => { + beforeEach(() => { + additionalDrives = false; + }); + + it("does not allow users to delete regular drives", async () => { + const { user } = plainRender(); + + const driveButton = screen.getByRole("button", { name: "Drive" }); + await user.click(driveButton); + const drivesMenu = screen.getByRole("menu"); + const deleteDriveButton = within(drivesMenu).queryByRole("menuitem", { + name: /Do not use/, + }); + expect(deleteDriveButton).not.toBeInTheDocument(); + }); + + it("does not allow users to delete drives explicitly used to boot", async () => { + const { user } = plainRender(); - const driveButton = screen.getByRole("button", { name: "Drive" }); - await user.click(driveButton); - const drivesMenu = screen.getByRole("menu"); - const deleteDriveButton = within(drivesMenu).queryByRole("menuitem", { - name: /Do not use/, + const driveButton = screen.getByRole("button", { name: "Drive" }); + await user.click(driveButton); + const drivesMenu = screen.getByRole("menu"); + const deleteDriveButton = within(drivesMenu).queryByRole("menuitem", { + name: /Do not use/, + }); + expect(deleteDriveButton).not.toBeInTheDocument(); }); - expect(deleteDriveButton).not.toBeInTheDocument(); }); }); diff --git a/web/src/components/storage/DriveEditor.tsx b/web/src/components/storage/DriveEditor.tsx index 629c06716f..a8aab5daa1 100644 --- a/web/src/components/storage/DriveEditor.tsx +++ b/web/src/components/storage/DriveEditor.tsx @@ -25,11 +25,11 @@ import { useNavigate, generatePath } from "react-router-dom"; import { _, formatList } from "~/i18n"; import { sprintf } from "sprintf-js"; import { baseName, deviceLabel, formattedPath, SPACE_POLICIES } from "~/components/storage/utils"; -import { useAvailableDevices, useVolume } from "~/queries/storage"; +import { useAvailableDevices } from "~/queries/storage"; import { configModel } from "~/api/storage/types"; import { StorageDevice } from "~/types/storage"; import { STORAGE as PATHS } from "~/routes/paths"; -import { useDrive } from "~/queries/storage/config-model"; +import { useDrive, useModel } from "~/queries/storage/config-model"; import * as driveUtils from "~/components/storage/utils/drive"; import * as partitionUtils from "~/components/storage/utils/partition"; import { contentDescription } from "~/components/storage/utils/device"; @@ -380,14 +380,20 @@ const SearchSelector = ({ drive, selected, onChange }) => { const RemoveDriveOption = ({ drive }) => { const driveModel = useDrive(drive.name); + const { hasAdditionalDrives } = useModel(); if (!driveModel) return; const { isExplicitBoot, delete: deleteDrive } = driveModel; + // When no additional drives has been added, the "Do not use" button can be confusing so it is + // omitted for all drives. + if (!hasAdditionalDrives) return; + + // FIXME: in these two cases the button should likely be present, but disabled and with an + // explanation of why those particular drive definitions cannot be removed. if (isExplicitBoot) return; if (driveUtils.hasPv(drive)) return; - if (driveUtils.hasRoot(drive)) return; return ( <> @@ -572,8 +578,6 @@ const PartitionMenuItem = ({ driveName, mountPath }) => { const navigate = useNavigate(); const drive = useDrive(driveName); const partition = drive.getPartition(mountPath); - const volume = useVolume(mountPath); - const isRequired = volume.outline?.required || false; const description = partition ? partitionUtils.typeWithSize(partition) : null; return ( @@ -597,15 +601,13 @@ const PartitionMenuItem = ({ driveName, mountPath }) => { ) } /> - {!isRequired && ( - } - actionId={`delete-${mountPath}`} - aria-label={`Delete ${mountPath}`} - onClick={() => drive.deletePartition(mountPath)} - /> - )} + } + actionId={`delete-${mountPath}`} + aria-label={`Delete ${mountPath}`} + onClick={() => drive.deletePartition(mountPath)} + /> } > diff --git a/web/src/components/storage/FixableConfigInfo.tsx b/web/src/components/storage/FixableConfigInfo.tsx new file mode 100644 index 0000000000..92d8cd28da --- /dev/null +++ b/web/src/components/storage/FixableConfigInfo.tsx @@ -0,0 +1,58 @@ +/* + * Copyright (c) [2025] SUSE LLC + * + * All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, contact SUSE LLC. + * + * To contact SUSE LLC about this file by physical or electronic mail, you may + * find current contact information at www.suse.com. + */ + +import React from "react"; +import { Alert, List, ListItem } from "@patternfly/react-core"; +import { n_ } from "~/i18n"; +import { useConfigErrors } from "~/queries/issues"; + +const Description = ({ errors }) => { + return ( + + {errors.map((e, i) => ( + {e.description} + ))} + + ); +}; + +/** + * Information about a wrong but fixable storage configuration + * + */ +export default function FixableConfigInfo() { + const configErrors = useConfigErrors("storage"); + + if (!configErrors.length) return; + + const title = n_( + "The configuration must be adapted to address the following issue:", + "The configuration must be adapted to address the following issues:", + configErrors.length, + ); + + return ( + + + + ); +} diff --git a/web/src/components/storage/ProposalFailedInfo.tsx b/web/src/components/storage/ProposalFailedInfo.tsx index 276a079a24..b5af24c369 100644 --- a/web/src/components/storage/ProposalFailedInfo.tsx +++ b/web/src/components/storage/ProposalFailedInfo.tsx @@ -23,7 +23,7 @@ import React from "react"; import { Alert, Content } from "@patternfly/react-core"; import { _, n_, formatList } from "~/i18n"; -import { useIssues } from "~/queries/issues"; +import { useIssues, useConfigErrors } from "~/queries/issues"; import { useConfigModel } from "~/queries/storage/config-model"; import { IssueSeverity } from "~/types/issues"; import * as partitionUtils from "~/components/storage/utils/partition"; @@ -70,9 +70,11 @@ function Description({ partitions }) { * */ export default function ProposalFailedInfo() { + const configErrors = useConfigErrors("storage"); const errors = useIssues("storage").filter((s) => s.severity === IssueSeverity.Error); const model = useConfigModel({ suspense: true }); + if (configErrors.length) return; if (!errors.length) return; const modelPartitions = model.drives.flatMap((d) => d.partitions || []); diff --git a/web/src/components/storage/ProposalPage.tsx b/web/src/components/storage/ProposalPage.tsx index 9dfcae5f74..9849307c67 100644 --- a/web/src/components/storage/ProposalPage.tsx +++ b/web/src/components/storage/ProposalPage.tsx @@ -39,6 +39,7 @@ import { Icon, Loading } from "~/components/layout"; import ProposalResultSection from "./ProposalResultSection"; import ProposalTransactionalInfo from "./ProposalTransactionalInfo"; import ProposalFailedInfo from "./ProposalFailedInfo"; +import FixableConfigInfo from "./FixableConfigInfo"; import UnsupportedModelInfo from "./UnsupportedModelInfo"; import ConfigEditor from "./ConfigEditor"; import ConfigEditorMenu from "./ConfigEditorMenu"; @@ -184,6 +185,7 @@ function ProposalSections(): React.ReactNode { + {model && ( <> @@ -235,13 +237,9 @@ export default function ProposalPage(): React.ReactNode { if (isDeprecated) reprobe().catch(console.log); }, [isDeprecated, reprobe]); - /** - * @fixme For now, a config model is only considered as editable if there is no config error. The - * UI for handling a model is not prepared yet to properly work with a model generated from a - * config with errors. Components like ConfigEditor should be adapted in order to properly manage - * those scenarios. - */ - const isModelEditable = model && !configErrors.length; + const fixable = ["no_root", "required_filesystems"]; + const unfixableErrors = configErrors.filter((e) => !fixable.includes(e.kind)); + const isModelEditable = model && !unfixableErrors.length; const hasDevices = !!availableDevices.length; const hasResult = !systemErrors.length; const showSections = hasDevices && (isModelEditable || hasResult); diff --git a/web/src/queries/storage/config-model.ts b/web/src/queries/storage/config-model.ts index 22dc392a2b..a3cdbcfa1b 100644 --- a/web/src/queries/storage/config-model.ts +++ b/web/src/queries/storage/config-model.ts @@ -324,6 +324,27 @@ function unusedMountPaths(model: configModel.Config, volumes: Volume[]): string[ return volPaths.filter((p) => !assigned.includes(p)); } +/* + * Pretty artificial logic used to decide whether the UI should display buttons to remove + * some drives. The logic is tricky and misplaced, but it is the lesser evil taking into + * account the current code organization. + * + * TODO: Revisit when LVM support is added to the UI. + */ +function hasAdditionalDrives(model: configModel.Config): boolean { + if (model.drives.length <= 1) return false; + if (model.drives.length > 2) return true; + + // If there are only two drives, the following logic avoids the corner case in which first + // deleting one of them and then changing the boot settings can lead to zero disks. But it is far + // from being fully reasonable or understandable for the user. + const onlyToBoot = model.drives.find( + (d) => isExplicitBoot(model, d.name) && !isUsedDrive(model, d.name), + ); + + return !onlyToBoot; +} + const configModelQuery = { queryKey: ["storage", "configModel"], queryFn: fetchConfigModel, @@ -451,6 +472,8 @@ export type ModelHook = { model: configModel.Config; usedMountPaths: string[]; unusedMountPaths: string[]; + // Hacky solution used to decide whether it makes sense to allow to remove drives + hasAdditionalDrives: boolean; addDrive: (driveName: string) => void; }; @@ -467,5 +490,6 @@ export function useModel(): ModelHook { addDrive: (driveName) => mutate(addDrive(model, driveName)), usedMountPaths: model ? usedMountPaths(model) : [], unusedMountPaths: model ? unusedMountPaths(model, volumes) : [], + hasAdditionalDrives: hasAdditionalDrives(model), }; }