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

web: Reuse devices #1165

Merged
merged 32 commits into from
May 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
4c082bf
web: Put "Delete" volume option at the end
dgdavid Apr 26, 2024
1a107bd
web: Add the "Reset location" volume action
dgdavid Apr 26, 2024
0a18ab9
web: Refactor VolumeActions internal component
dgdavid Apr 26, 2024
dd8ce25
web: Extract few components from VolumeRow
dgdavid Apr 26, 2024
dc24960
web: Improvements in tables
joseivanlopez Apr 26, 2024
d88d1f0
web: Fix a bunch of typos
dgdavid Apr 26, 2024
7fb343b
web: WIP first version of the new location dialog
joseivanlopez Apr 29, 2024
66b9b00
service: Fix bug converting settings from Y2Storage
joseivanlopez Apr 30, 2024
4bcef6b
web: Improve wording
joseivanlopez Apr 30, 2024
7a4f457
service: Always export the file system human string
joseivanlopez Apr 30, 2024
dc6c80f
web: Remove unnecessary conversion for file system type
joseivanlopez Apr 30, 2024
07f850d
web: Do not consider reused devices as installation devices
joseivanlopez Apr 30, 2024
bded1d0
wer: Add alert when editing volume
joseivanlopez Apr 30, 2024
a9b8d0d
web: Several improvements
joseivanlopez May 2, 2024
3d479a8
web: Force two rows layout in VolumeLocationDialog
dgdavid May 2, 2024
78a57ae
web: Increase a bit the size of radio labels
dgdavid May 2, 2024
d458000
web: Add spaces between VolumeLocationDialog elements
dgdavid May 2, 2024
9bfedf7
web: CSS tweaks for VolumeLocationDialog table
dgdavid May 2, 2024
98a46ac
service: Do not set device if reusing a device
joseivanlopez May 2, 2024
b2d3512
web: Small fixes in styles
joseivanlopez May 2, 2024
fd5d7be
web: Small visual fix at DeviceSelectorTable
ancorgs May 2, 2024
82f7be5
web: Add tests for VolumeLocationDialog
joseivanlopez May 2, 2024
b397231
web: Small fixes
joseivanlopez May 2, 2024
1460a2d
web: Fix some tests
joseivanlopez May 2, 2024
a848039
Merge branch 'master' into reuse-device
joseivanlopez May 2, 2024
e77b83a
web: Fix and adapt some tests
joseivanlopez May 2, 2024
e2ae65d
web: Make scrollable table more discoverable
dgdavid May 2, 2024
546f82c
web: Please CSpell
dgdavid May 2, 2024
068e7b3
web: Add and adapt tests
joseivanlopez May 3, 2024
f4ce92b
web: Minor fixes
joseivanlopez May 3, 2024
2dddefa
web: Changelog
joseivanlopez May 3, 2024
ca7e244
web: Improvements from review
joseivanlopez May 3, 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
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def filesystem_sid
#
# @return [String] e.g., "ext4"
def filesystem_type
storage_device.filesystem.type.to_s
storage_device.filesystem.type.to_human_string
end

# Mount path of the file system.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,9 @@ def missing_volumes
def volume_device_conversion(target)
return unless settings.device.is_a?(DeviceSettings::Disk)

target.volumes.select(&:proposed?).each { |v| v.device ||= settings.device.name }
target.volumes
.select { |v| v.proposed? && !v.reuse_name }
.each { |v| v.device ||= settings.device.name }
end

# @param target [Y2Storage::ProposalSettings]
Expand Down
4 changes: 2 additions & 2 deletions service/lib/agama/storage/volume_conversion/from_y2storage.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ def sizes_conversion(target)
planned = planned_device_for(target.mount_path)
return unless planned

target.min_size = planned.min
target.max_size = planned.max
target.min_size = planned.min if planned.respond_to?(:min)
target.max_size = planned.max if planned.respond_to?(:max)
end

# Planned device for the given mount path.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@

describe "#filesystem_type" do
it "returns the file system type" do
expect(subject.filesystem_type).to eq("ext4")
expect(subject.filesystem_type).to eq("Ext4")
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,25 @@
expect(y2storage_settings.candidate_devices).to contain_exactly("/dev/sda")
end
end

context "and a volume is reusing a device" do
before do
settings.volumes = [
Agama::Storage::Volume.new("/test1").tap do |volume|
volume.location.target = :device
volume.location.device = "/dev/sdb"
end
]
end

it "does not set the target device as device for the volume with missing device" do
y2storage_settings = subject.convert

expect(y2storage_settings.volumes).to contain_exactly(
an_object_having_attributes(mount_point: "/test1", device: nil)
)
end
end
end

context "when the device settings is set to create a new LVM volume group" do
Expand Down
1 change: 1 addition & 0 deletions web/cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
"subvolume",
"subvolumes",
"svgrrc",
"scrollbox",
"teleporter",
"testfile",
"testsuite",
Expand Down
6 changes: 6 additions & 0 deletions web/package/cockpit-agama.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
-------------------------------------------------------------------
Fri May 3 09:45:36 UTC 2024 - José Iván López González <jlopez@suse.com>

- Allow reusing an existing device or file system
(gh#openSUSE/agama#1165).

-------------------------------------------------------------------
Thu May 02 11:07:23 UTC 2024 - Balsa Asanovic <balsaasanovic95@gmail.com>

Expand Down
34 changes: 34 additions & 0 deletions web/src/assets/styles/composition.scss
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,40 @@
flex-wrap: wrap;
}

// TODO: make it less specific.
.location-layout > div {
display: flex;

form {
display: flex;
flex-direction: column;
flex: 1 1 0;
gap: 0
}

form > div:nth-child(2) {
overflow-y: auto;
min-block-size: 120px;
margin-block-end: var(--spacer-medium);
table { background: transparent; }
}

form > div:last-child {
min-block-size: min-content;
}
}

// FIXME: Temporary solution to hide expand button. The button should be removed instead.
.location-layout table button[id^="expand-toggle"] {
display: none;
}

.location-layout table tbody tr:not(:first-child) {
td:nth-child(3) {
padding-inline-start: 5ch;
}
}

[data-state="reversed"] {
flex-direction: row-reverse;
}
Expand Down
8 changes: 8 additions & 0 deletions web/src/assets/styles/patternfly-overrides.scss
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,14 @@ ul {
vertical-align: middle;
}

.pf-v5-c-radio {
align-items: center;
}

.pf-v5-c-radio__label {
font-size: var(--pf-v5-c-form__label--FontSize);
}

@media screen and (width <= 768px) {
.pf-m-grid-md.pf-v5-c-table tr:where(.pf-v5-c-table__tr):not(.pf-v5-c-table__expandable-row) {
padding-inline: 0;
Expand Down
12 changes: 12 additions & 0 deletions web/src/assets/styles/utilities.scss
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,18 @@
max-inline-size: calc(var(--ui-max-inline-size) + var(--spacer-large))
}

.scrollbox {
background:
linear-gradient(#fff 33%, rgb(255 255 255 / 0%)),
linear-gradient(rgb(255 255 255 / 0%), #fff 66%) 0 100%,
radial-gradient(farthest-side at 50% 0, rgb(102 102 102 / 50%), rgb(0 0 0 / 0%)),
radial-gradient(farthest-side at 50% 100%, rgba(102 102 102 / 50%), rgb(0 0 0 / 0%)) 0 100%;
background-color: #fff;
background-repeat: no-repeat;
background-attachment: local, local, scroll, scroll;
background-size: 100% 48px, 100% 48px, 100% 16px, 100% 16px;
}

.height-75 {
height: 75dvh;
}
Expand Down
45 changes: 43 additions & 2 deletions web/src/client/storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,40 @@ class ProposalManager {
return proxy.AvailableDevices.map(path => findDevice(systemDevices, path)).filter(d => d);
}

/**
* Gets the devices that can be selected as target for a volume.
*
* A device can be selected as target for a volume if either it is an available device for
* installation or it is a device built over the available devices for installation. For example,
* a MD RAID is a possible target only if all its members are available devices or children of the
* available devices.
*
* @returns {Promise<StorageDevice[]>}
*/
async getVolumeDevices() {
/** @type {StorageDevice[]} */
const availableDevices = await this.getAvailableDevices();

/** @type {(device: StorageDevice) => boolean} */
const isAvailable = (device) => {
const isChildren = (device, parentDevice) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: If I'm not wrong, we already have a isChildren function somewhere. Anyway, this kind of functions looks like a potential util that should live elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is a #deviceChildren function to generate the list of children. In this case the function checks if it is a children of a device. Anyway, I think we need to improve this part, see https://github.com/openSUSE/agama/pull/1165/files#diff-8cfada04978222410b4703fc1ca52a1ff139c6497329b3ca2e4e176ba5d14d00R26.

const partitions = parentDevice.partitionTable?.partitions || [];
return !!partitions.find(d => d.name === device.name);
};

return !!availableDevices.find(d => d.name === device.name || isChildren(device, d));
};

/** @type {(device: StorageDevice[]) => boolean} */
const allAvailable = (devices) => devices.every(isAvailable);

const system = await this.system.getDevices();
const mds = system.filter(d => d.type === "md" && allAvailable(d.devices));
const vgs = system.filter(d => d.type === "lvmVg" && allAvailable(d.physicalVolumes));

return [...availableDevices, ...mds, ...vgs];
}

/**
* Gets the list of meaningful mount points for the selected product
*
Expand Down Expand Up @@ -537,7 +571,7 @@ class ProposalManager {
return dbusTargetPVDevices.v.map(d => d.v);
};

// TODO: Read installation devices from D-Bus.
/** @todo Read installation devices from D-Bus. */
const buildInstallationDevices = (dbusSettings, devices) => {
const findDevice = (name) => {
const device = devices.find(d => d.name === name);
Expand All @@ -547,10 +581,17 @@ class ProposalManager {
return device;
};

// Only consider the device assigned to a volume as installation device if it is needed
// to find space in that device. For example, devices directly formatted or mounted are not
// considered as installation devices.
const volumes = dbusSettings.Volumes.v.filter(vol => (
[VolumeTargets.NEW_PARTITION, VolumeTargets.NEW_VG].includes(vol.v.Target.v))
);

const values = [
dbusSettings.TargetDevice?.v,
buildTargetPVDevices(dbusSettings.TargetPVDevices),
dbusSettings.Volumes.v.map(vol => vol.v.TargetDevice.v)
volumes.map(vol => vol.v.TargetDevice.v)
].flat();

if (dbusSettings.ConfigureBoot.v) values.push(dbusSettings.BootDevice.v);
Expand Down
43 changes: 29 additions & 14 deletions web/src/components/core/ExpandableSelector.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@
import React, { useState } from "react";
import { Table, Thead, Tr, Th, Tbody, Td, ExpandableRowContent, RowSelectVariant } from "@patternfly/react-table";

/**
* @typedef {import("@patternfly/react-table").TableProps} TableProps
* @typedef {import("react").RefAttributes<HTMLTableElement>} HTMLTableProps
*/

/**
* An object for sharing data across nested maps
*
Expand Down Expand Up @@ -93,23 +98,26 @@ const sanitizeSelection = (selection, allowMultiple) => {
};

/**
* Build a expandable table with selectable items
* Build a expandable table with selectable items.
* @component
*
* @note It only accepts one nesting level.
*
* @param {object} props
* @param {ExpandableSelectorColumn[]} props.columns - Collection of objects defining columns.
* @param {boolean} [props.isMultiple=false] - Whether multiple selection is allowed.
* @param {object[]} props.items - Collection of items to be rendered.
* @param {string} [props.itemIdKey="id"] - The key for retrieving the item id.
* @param {(item: object) => Array<object>} [props.itemChildren=() => []] - Lookup method to retrieve children from given item.
* @param {(item: object) => boolean} [props.itemSelectable=() => true] - Whether an item will be selectable or not.
* @param {(item: object) => (string|undefined)} [props.itemClassNames=() => ""] - Callback that allows adding additional CSS class names to item row.
* @param {object[]} [props.itemsSelected=[]] - Collection of selected items.
* @param {string[]} [props.initialExpandedKeys=[]] - Ids of initially expanded items.
* @param {(selection: Array<object>) => void} [props.onSelectionChange=noop] - Callback to be triggered when selection changes.
* @param {object} [props.tableProps] - Props for {@link https://www.patternfly.org/components/table/#table PF/Table}.
* @typedef {object} ExpandableSelectorBaseProps
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: Just a note not related with this PR. We are following this pattern of defining ComponentBaseProp and for now it is ok. But I wonder if, since it is something internal, we could call them just BaseProp instead. I see two problems, however:

* @property {ExpandableSelectorColumn[]} [columns=[]] - Collection of objects defining columns.
* @property {boolean} [isMultiple=false] - Whether multiple selection is allowed.
* @property {object[]} [items=[]] - Collection of items to be rendered.
* @property {string} [itemIdKey="id"] - The key for retrieving the item id.
* @property {(item: object) => Array<object>} [itemChildren=() => []] - Lookup method to retrieve children from given item.
* @property {(item: object) => boolean} [itemSelectable=() => true] - Whether an item will be selectable or not.
* @property {(item: object) => (string|undefined)} [itemClassNames=() => ""] - Callback that allows adding additional CSS class names to item row.
* @property {object[]} [itemsSelected=[]] - Collection of selected items.
* @property {any[]} [initialExpandedKeys=[]] - Ids of initially expanded items.
* @property {(selection: Array<object>) => void} [onSelectionChange=noop] - Callback to be triggered when selection changes.
*
* @typedef {ExpandableSelectorBaseProps & TableProps & HTMLTableProps} ExpandableSelectorProps
*
* @param {ExpandableSelectorProps} props
*/
export default function ExpandableSelector({
columns = [],
Expand All @@ -126,7 +134,14 @@ export default function ExpandableSelector({
}) {
const [expandedItemsKeys, setExpandedItemsKeys] = useState(initialExpandedKeys);
const selection = sanitizeSelection(itemsSelected, isMultiple);
const isItemSelected = (item) => selection.includes(item);
const isItemSelected = (item) => {
const selected = selection.find((selectionItem) => {
return Object.hasOwn(selectionItem, itemIdKey) &&
selectionItem[itemIdKey] === item[itemIdKey];
});

return selected !== undefined || selection.includes(item);
};
const isItemExpanded = (key) => expandedItemsKeys.includes(key);
const toggleExpanded = (key) => {
if (isItemExpanded(key)) {
Expand Down
10 changes: 5 additions & 5 deletions web/src/components/core/TreeTable.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ import { Table, Thead, Tr, Th, Tbody, Td, TreeRowWrapper } from '@patternfly/rea

/**
* @typedef {object} TreeTableColumn
* @property {string} title
* @property {(any) => React.ReactNode} content
* @property {string} name
* @property {(object) => React.ReactNode} value
* @property {string} [classNames]
*/

Expand Down Expand Up @@ -82,14 +82,14 @@ export default function TreeTable({
const renderColumns = (item, treeRow) => {
return columns.map((c, cIdx) => {
const props = {
dataLabel: c.title,
dataLabel: c.name,
className: c.classNames
};

if (cIdx === 0) props.treeRow = treeRow;

return (
<Td key={cIdx} {...props}>{c.content(item)}</Td>
<Td key={cIdx} {...props}>{c.value(item)}</Td>
);
});
};
Expand Down Expand Up @@ -138,7 +138,7 @@ export default function TreeTable({
>
<Thead>
<Tr>
{ columns.map((c, i) => <Th key={i} className={c.classNames}>{c.title}</Th>) }
{ columns.map((c, i) => <Th key={i} className={c.classNames}>{c.name}</Th>) }
</Tr>
</Thead>
<Tbody>
Expand Down
20 changes: 11 additions & 9 deletions web/src/components/storage/BootConfigField.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -56,23 +56,25 @@ const Button = ({ isBold = false, onClick }) => {
* Allows to select the boot config.
* @component
*
* @param {object} props
* @param {boolean} props.configureBoot
* @param {StorageDevice|undefined} props.bootDevice
* @param {StorageDevice|undefined} props.defaultBootDevice
* @param {StorageDevice[]} props.devices
* @param {boolean} props.isLoading
* @param {(boot: BootConfig) => void} props.onChange
* @typedef {object} BootConfigFieldProps
* @property {boolean} configureBoot
* @property {StorageDevice|undefined} bootDevice
* @property {StorageDevice|undefined} defaultBootDevice
* @property {StorageDevice[]} availableDevices
* @property {boolean} isLoading
* @property {(boot: BootConfig) => void} onChange
*
* @typedef {object} BootConfig
* @property {boolean} configureBoot
* @property {StorageDevice} bootDevice
*
* @param {BootConfigFieldProps} props
*/
export default function BootConfigField({
configureBoot,
bootDevice,
defaultBootDevice,
devices,
availableDevices,
isLoading,
onChange
}) {
Expand Down Expand Up @@ -113,7 +115,7 @@ export default function BootConfigField({
configureBoot={configureBoot}
bootDevice={bootDevice}
defaultBootDevice={defaultBootDevice}
devices={devices}
availableDevices={availableDevices}
onAccept={onAccept}
onCancel={closeDialog}
/>
Expand Down
Loading
Loading