Wizard: Disk customization (HMS-8945) #3692
Conversation
regexowl
force-pushed
the
editable-disk
branch
from
96edfac to
c5728fd
Compare
regexowl
changed the title
regexowl
force-pushed
the
editable-disk
branch
from
c5728fd to
113c721
Compare
regexowl
force-pushed
the
editable-disk
branch
5 times, most recently
from
24397a6 to
05ee746
Compare
src/Components/CreateImageWizard/steps/FileSystem/components/MinimumSize.tsx
Show resolved
Hide resolved
regexowl
changed the title
regexowl
force-pushed
the
editable-disk
branch
2 times, most recently
from
1df31c0 to
8003b1e
Compare
|
Since the customization is still very much a work in progress I feel temped to add an alert that would state so. And warn users that should they use the advanced partitioning option we expect they know what they're doing. It's currently gated in stage:preview only, but exposed in on-prem, does that make sense or do we want to remove it from on-prem until it's a bit more refined? |
|
A note about constraints: While the API supports an array of volume groups, internally we only allow creating one VG. This choice was made for simplicity, but we're keeping the possibility open for allowing multiple VGs in the future. I suspect we might get a feature request to add support for multiple VGs at some point. So perhaps we can design this in a way that allows adding the option easily when we need to. |
regexowl
force-pushed
the
editable-disk
branch
from
8003b1e to
2533d04
Compare
Ah, got it. Let's just hide the "add vg" button when there's already a volume group added. This will also make it very easy to re-enable adding multiple groups in the future. |
regexowl
force-pushed
the
editable-disk
branch
5 times, most recently
from
48d4433 to
9d595d3
Compare
regexowl
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
playwright/Customizations/Disk.spec.ts (3)
55-60: Consider removing or tracking the commented code.The commented-out validation for the
auto-lvmradio button has a note indicating uncertainty about whether this feature should exist. Consider either removing this commented code or creating a tracking issue if the validation should be added once the feature decision is made.
64-64: Prefer specific selectors over positional nth() when possible.The test uses
nth()extensively to select elements by position (e.g., lines 64, 96-97, 108-112, 139-140, 204-209). While functional, this makes the test fragile to UI reordering. Lines 122-140 demonstrate a better approach usinggetByRole('row', { name: '...' })to select specific rows by their content. Where feasible, consider using:
- More specific role queries with accessible names
- Test IDs for elements that need stable selection
- Container scoping (selecting within a specific parent element)
This will improve test maintainability and reduce false failures when UI structure changes.
Also applies to: 96-98, 108-112, 139-140, 204-209
275-275: Minor: Useframefor consistency.Line 275 uses
pagewhile the rest of the step usesframe. Although functionally equivalent in hosted mode (where this step runs), usingframewould maintain consistency with the surrounding code (lines 244-273).Apply this diff:
- await page.getByRole('button', { name: 'Cancel' }).click(); + await frame.getByRole('button', { name: 'Cancel' }).click();src/store/wizardSlice.ts (1)
803-836: Optimize LVM search loops with early returns.The LVM search loops in
changePartitionMountpoint,changePartitionUnit,changePartitionMinSize,changePartitionType, andchangePartitionNamecontinue iterating after finding and updating a logical volume. Since IDs are unique, the loop can exit immediately after the update.For example, in
changePartitionMountpoint(lines 824-835):for (const partition of state.disk.partitions) { if (partition.type === 'lvm') { const logicalVolumeIndex = partition.logical_volumes.findIndex( (lv) => lv.id === id, ); if (logicalVolumeIndex !== -1) { partition.logical_volumes[logicalVolumeIndex].mountpoint = mountpoint; + return; } } }Apply the same pattern to lines 854-864, 883-893, 915-925, and 947-957.
Also applies to: 837-865, 866-894, 895-926, 927-958
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
playwright/Customizations/Disk.spec.ts(1 hunks)src/store/wizardSlice.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/store/wizardSlice.ts (2)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (7)
FscDisk(21-25)FilesystemPartition(10-15)PartitioningCustomization(8-8)Units(17-17)FSType(19-19)DiskPartition(43-46)DiskPartitionBase(27-31)src/store/service/imageBuilderApi.ts (1)
LogicalVolume(707-717)
playwright/Customizations/Disk.spec.ts (5)
playwright/helpers/wizardHelpers.ts (8)
deleteBlueprint(79-111)registerLater(56-61)fillInDetails(37-49)createBlueprint(16-27)exportBlueprint(117-136)verifyExportedBlueprint(143-149)importBlueprint(157-172)fillInImageOutputGuest(67-70)playwright/helpers/login.ts (1)
ensureAuthenticated(43-69)playwright/helpers/navHelpers.ts (3)
navigateToLandingPage(63-69)ibFrame(50-57)fillInImageOutput(18-44)playwright/helpers/helpers.ts (1)
isHosted(22-24)playwright/fixtures/data/exportBlueprintContents.ts (1)
exportedFilesystemBP(1-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (25)
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: Cockpit Tests
- GitHub Check: Service Tests
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: playwright-tests
🔇 Additional comments (3)
playwright/Customizations/Disk.spec.ts (3)
27-46: LGTM! Clean test setup.The test setup properly handles authentication, navigation, cleanup registration, and initial wizard navigation. The use of UUID for blueprint name ensures uniqueness across test runs.
217-239: LGTM! Export/import flow correctly implemented.The export and import steps now correctly:
- Capture the exported blueprint path (line 219)
- Register cleanup for the temporary file (line 221)
- Pass the exported path to the import function (line 238)
This addresses the critical issues raised in previous review comments.
181-188: Excellent validation of mountpoint suffix behavior!The test properly validates that the mountpoint suffix field is visible for
/tmpbut hidden for/usr, addressing the edge case feedback from previous review comments. This ensures the UI correctly handles mount points that don't support suffixes.
regexowl
force-pushed
the
editable-disk
branch
from
771512f to
b641804
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Components/CreateImageWizard/utilities/requestMapper.ts (1)
160-172: Align VG/Btrfs minsize handling with plain partitions and LVsThere are a couple of inconsistencies around
minsizethat can bite both import and export flows:
- VG/Btrfs import into wizard state
In
convertDiskToFscDisk:if ('logical_volumes' in disk) { return { id, - min_size: disk.minsize, + min_size: size, unit: unit as Units, name: disk.name, type: disk.type, logical_volumes: disk.logical_volumes.map(convertLogicalVolume), }; } if ('subvolumes' in disk) { return { id, - min_size: disk.minsize, + min_size: size, unit: unit as Units, type: disk.type, subvolumes: disk.subvolumes, }; }Plain partitions and LVs are normalized to
min_size: size+unit, but VGs and Btrfs currently keep the raw"1 GiB"string inmin_size. That’s inconsistent with howMinimumSizeand validation treatmin_sizeas a digits‑only string and complicates editing for imported configs.Using the parsed
sizeformin_sizekeeps all UI-facing partitions in the same shape.
- VG/Btrfs export from wizard state back to API
In
getDisk:if (partition.type === 'lvm') { return { - minsize: partition.min_size ? partition.min_size : undefined, + minsize: + partition.min_size && partition.unit + ? `${partition.min_size} ${partition.unit}` + : undefined, name: partition.name, type: partition.type, logical_volumes: partition.logical_volumes.map((lv) => ({ minsize: lv.min_size + ' ' + lv.unit, name: lv.name, fs_type: lv.fs_type, mountpoint: lv.mountpoint, })), }; } if (partition.type === 'btrfs') { return { - minsize: partition.min_size, + minsize: + partition.min_size && partition.unit + ? `${partition.min_size} ${partition.unit}` + : undefined, type: partition.type, subvolumes: partition.subvolumes, }; }Without combining
min_sizeandunit, new VGs and Btrfs partitions created in the wizard will emitminsize: '1'instead of'1 GiB', while LVs and plain partitions already use the combined"size unit"format.Aligning VG/Btrfs handling with the plain/LV paths will:
- Keep wizard state consistent (
min_sizeis just the numeric string).- Produce the
minsizeformat the API expects for all disk partition types.I realize you mentioned follow‑ups for minsize handling; this is one of the key correctness spots to cover there.
Also applies to: 174-216, 218-235, 876-918
♻️ Duplicate comments (3)
src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemTable.tsx (1)
88-121: Fix TbodyonDrophandler and wire reordering for disk modesTwo related issues:
- Miswired Tbody
onDrop<Tbody onDragOver={onDragOver} - onDrop={onDragOver} + onDrop={onDrop} onDragLeave={onDragLeave} ref={bodyRef} >Using
onDrop={onDragOver}prevents the real drop handler (and itschangePartitionOrderdispatch /onDragCancel) from running when the event lands on the<tbody>rather than a row. This was already flagged before; it’s still present.
- Disk-mode drag‑and‑drop doesn’t persist
onDropalways dispatcheschangePartitionOrder(tempItemOrder), but the reducer only reordersfileSystem.partitions. In disk modes (mode === 'disk-plain' | 'disk-lvm'),partitionscomes fromstate.wizard.disk.partitions, so:
- The DOM reorders temporarily via
move(...).- Redux state for disk partitions never changes, so the order will revert on the next render.
I’d suggest either:
- Extending
changePartitionOrderto accept acustomization: 'fileSystem' | 'disk'and branch on it, or- Adding a dedicated
changeDiskPartitionOrderand invoking that whenmode !== 'filesystem'.Right now advanced disk layouts can’t be reliably reordered.
Also applies to: 197-225
src/store/wizardSlice.ts (1)
723-755: Advanced mode always resets disk partitionsIn
changeFscMode:case 'advanced': state.disk.partitions = [ { id: uuidv4(), mountpoint: '/', fs_type: 'ext4', min_size: '1', unit: 'GiB', type: 'plain', }, ]; break;Every transition into
'advanced'wipes whatever disk layout the user already had and replaces it with a single default/partition. That means:
- Toggling basic ⇄ advanced loses any advanced layout the user created.
- There’s no explicit “reset disk layout” action in the UI; it happens implicitly on mode change.
Given your note about a future conversion function between basic/advanced, I’d recommend at least guarding this so you only seed when there is no existing disk config:
case 'advanced': - state.disk.partitions = [ /* default */ ]; + if (state.disk.partitions.length === 0) { + state.disk.partitions = [ /* default */ ]; + } break;You can then revisit full conversion semantics in the dedicated follow‑up without surprising users in the meantime.
src/Components/CreateImageWizard/steps/FileSystem/components/PartitionName.tsx (1)
14-41: VG name validation still wired to wrong field keyVolume groups and LVs share this component, but the field name is always
lvname-${partition.id}.useFilesystemValidationsets:
- VG errors under
name-${partition.id}- LV errors under
lvname-${lv.id}So VG name validation will block “Next” but never show an inline error.
You can align this by switching the key based on the shape:
const PartitionName = ({ partition, customization, }: PartitionNamePropTypes) => { const dispatch = useAppDispatch(); const stepValidation = useFilesystemValidation(); + const isVolumeGroup = 'logical_volumes' in partition; + const fieldName = isVolumeGroup + ? `name-${partition.id}` + : `lvname-${partition.id}`; + return ( <ValidatedInputAndTextArea ariaLabel='Partition name input' value={partition.name || ''} onChange={(event, name) => { dispatch( changePartitionName({ id: partition.id, name: name, customization: customization, }), ); }} stepValidation={stepValidation} - fieldName={`lvname-${partition.id}`} + fieldName={fieldName} /> ); };That keeps VG and LV validation in sync with
useFilesystemValidation.
🧹 Nitpick comments (2)
playwright/fixtures/data/exportBlueprintContents.ts (1)
1-27: Consider adding test fixtures for edge cases.The current fixture provides a straightforward happy-path scenario. As constraints behavior and conversion logic are refined in follow-ups, consider adding fixtures that test edge cases such as:
- Swap partitions (if mountpoint behavior differs)
- Validation boundaries (e.g., minimum size limits)
- Multiple logical volumes within a single VG
- Plain-only partitions without LVM (if supported)
This can help catch constraint violations and improve test coverage as the feature matures.
src/Components/CreateImageWizard/steps/FileSystem/components/DiskRow.tsx (1)
45-66: Clarify DiskRow responsibilities and unreachable PartitionName pathA few points worth tightening up here:
The early return for
partition.type === 'lvm' || partition.type === 'btrfs'means this row only ever renders plain partitions. Given that, thepartition.type !== 'plain'check around<PartitionName>is effectively unreachable, except for partitions wheretypeisundefined. If plain partitions are never meant to have names, it’s clearer to drop thePartitionNamecell entirely fromDiskRowand keep VG/LV naming in the dedicated components (e.g.VolumeGroups).For readability, consider making the early return explicit:
if (partition.type === 'lvm' || partition.type === 'btrfs') {
return;
}
- if (partition.type === 'lvm' || partition.type === 'btrfs') {
- return null;
- }
so the component clearly returns nothing for non-plain disk entries. - The remove‑button guard ```ts isDisabled={ partition.mountpoint === '/' && partitions.filter((p) => p.type === 'plain').length > 1 }enforces your intended “root only removable when it’s the only plain partition” rule. If that’s the desired advanced‑mode semantics, it’s fine as is; if you ever want the more typical “never allow removing the last
/”, this condition will need to be inverted.None of these are blockers, but simplifying the dead
PartitionNamepath will make the component’s responsibilities easier to follow.Also applies to: 67-80, 95-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
playwright/Customizations/Disk.spec.ts(1 hunks)playwright/fixtures/data/exportBlueprintContents.ts(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/DiskRow.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemTable.tsx(4 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/MinimumSize.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/MountpointPrefix.tsx(3 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/MountpointSuffix.tsx(2 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/PartitionName.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/PartitionType.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/SizeUnit.tsx(2 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/VolumeGroups.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts(1 hunks)src/Components/CreateImageWizard/utilities/requestMapper.ts(7 hunks)src/store/wizardSlice.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Components/CreateImageWizard/steps/FileSystem/components/MountpointSuffix.tsx
- src/Components/CreateImageWizard/steps/FileSystem/components/VolumeGroups.tsx
- src/Components/CreateImageWizard/steps/FileSystem/components/MountpointPrefix.tsx
- playwright/Customizations/Disk.spec.ts
- src/Components/CreateImageWizard/steps/FileSystem/components/PartitionType.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
src/Components/CreateImageWizard/steps/FileSystem/components/PartitionName.tsx (4)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (3)
DiskPartitionBase(27-31)LogicalVolumeWithBase(33-33)PartitioningCustomization(8-8)src/store/hooks.ts (1)
useAppDispatch(6-6)src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useFilesystemValidation(304-351)src/Components/CreateImageWizard/ValidatedInput.tsx (1)
ValidatedInputAndTextArea(47-118)
src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemTable.tsx (1)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (2)
FilesystemPartition(10-15)DiskPartition(43-46)
src/Components/CreateImageWizard/steps/FileSystem/components/SizeUnit.tsx (1)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (5)
FilesystemPartition(10-15)LogicalVolumeWithBase(33-33)VolumeGroupWithExtendedLV(35-41)PartitioningCustomization(8-8)Units(17-17)
src/Components/CreateImageWizard/steps/FileSystem/components/MinimumSize.tsx (4)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (4)
FilesystemPartition(10-15)LogicalVolumeWithBase(33-33)VolumeGroupWithExtendedLV(35-41)PartitioningCustomization(8-8)src/store/hooks.ts (1)
useAppDispatch(6-6)src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useFilesystemValidation(304-351)src/Components/CreateImageWizard/ValidatedInput.tsx (1)
ValidatedInputAndTextArea(47-118)
src/Components/CreateImageWizard/utilities/requestMapper.ts (3)
src/store/service/imageBuilderApi.ts (5)
Filesystem(673-677)FilesystemTyped(679-690)VolumeGroup(718-728)BtrfsVolume(699-706)LogicalVolume(707-717)src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (3)
FilesystemPartition(10-15)DiskPartition(43-46)Units(17-17)src/store/wizardSlice.ts (3)
selectDiskMinsize(445-447)selectDiskPartitions(449-451)selectDiskType(441-443)
src/Components/CreateImageWizard/steps/FileSystem/components/DiskRow.tsx (3)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (2)
DiskPartition(43-46)FilesystemPartition(10-15)src/store/hooks.ts (2)
useAppDispatch(6-6)useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectDiskPartitions(449-451)
src/store/wizardSlice.ts (2)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (7)
FscDisk(21-25)FilesystemPartition(10-15)PartitioningCustomization(8-8)Units(17-17)FSType(19-19)DiskPartition(43-46)DiskPartitionBase(27-31)src/store/service/imageBuilderApi.ts (1)
LogicalVolume(707-717)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: playwright-tests
- GitHub Check: Service Tests
🔇 Additional comments (4)
playwright/fixtures/data/exportBlueprintContents.ts (1)
1-27: No changes required - fixture is correct per official API specification.The API accepts minsize as "an integer (in bytes) or a string with a data unit (for example 3 GiB)." The
exportedDiskBPfixture uses the string-with-units format throughout, which is validated by official Red Hat Image Builder documentation that shows the exact same nested TOML structure with string-formatted minsize values like "50 GiB", "20 GiB", and "2 GiB". The TOML nested array-of-tables syntax[[customizations.disk.partitions.logical_volumes]]correctly appends logical volumes to the last partition element, per the TOML 1.0 specification. The fixture structure and format match official specifications exactly.src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (1)
8-46: Types look consistent with state and API usage
PartitioningCustomization,FscDisk, andDiskPartition(with the sharedDiskPartitionBase) line up cleanly with howwizardSliceandrequestMapperindexstate[customization].partitionsand convert to/from API types. The discriminant viatype: 'plain' | 'lvm' | 'btrfs'should make narrowing straightforward in callers.No issues from my side here.
src/Components/CreateImageWizard/steps/FileSystem/components/SizeUnit.tsx (1)
26-57: Unit selector correctly generalized to filesystem + disk contextsThe updated
SizeUnitprops and dispatches (customizationrouted intochangePartitionMinSize/changePartitionUnit) look consistent with the newwizardSliceAPIs, and theunit === 'B'special case is preserved.No changes needed here.
src/Components/CreateImageWizard/steps/FileSystem/components/MinimumSize.tsx (1)
17-23: Minimum size input correctly generalized to disk + filesystemThe updated
MinimumSizecomponent now:
- Accepts the union of filesystem partitions, LVs, and VGs.
- Threads
customizationthrough tochangePartitionMinSize/changePartitionUnit.- Keeps the input controlled via
partition.min_size || ''and preserves theunit === 'B'warning/disable behavior.This lines up with the new reducer signatures and the validation hook; I don’t see any issues here.
Also applies to: 25-61
|
Last hiccup after fixing stuff to be up-to-date with the export on-prem blueprint PR - it looks like the disk customization doesn't get written into exported blueprint. Trying to figure out why that is 😵 |
regexowl
force-pushed
the
editable-disk
branch
from
b641804 to
80ed077
Compare
This renames Partition type to FilesystemPartition to make naming cleare after a new type for disk partition is added.
This adds raw and LVM radio buttons to advanced partitioning options, allowing to switch between the partitioning modes.
Move partitions outside of FileSystemTable and pass them as a prop.
This adds plain disk partition table and an option to add a volume group. Volume groups are added as a card that includes a file system table of logical volumes.
This will show the "Add LVM volume group" button only when there's no volume group added.
This adds validation to name fields for volume group and logical volumes.
This should add more clarity to which type are needed for basic partitioning (filesystem customization) and which are needed for advanced (disk).
This moves `'disk' | 'fileSystem'` to a constant.
This removes setting the mode to 'raw' for now. The default mode is 'auto-lvm', which might need its own radio. Will be addressed in a follow up.
regexowl
force-pushed
the
editable-disk
branch
from
80ed077 to
752dcb0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/store/wizardSlice.ts (1)
717-755: Advanced mode still resets disk layout on mode switchIn
changeFscMode, the'advanced'branch unconditionally overwritesstate.disk.partitionswith a single default/partition. Any existing advanced layout is lost as soon as the user leaves advanced mode and comes back, which is surprising given how much effort disk customization can take.Given this is a wizard‑wide UX concern and already noted in earlier review, consider only seeding when there is no existing disk configuration:
- case 'advanced': - state.disk.partitions = [ - { - id: uuidv4(), - mountpoint: '/', - fs_type: 'ext4', - min_size: '1', - unit: 'GiB', - type: 'plain', - }, - ]; - break; + case 'advanced': + if (state.disk.partitions.length === 0) { + state.disk.partitions = [ + { + id: uuidv4(), + mountpoint: '/', + fs_type: 'ext4', + min_size: '1', + unit: 'GiB', + type: 'plain', + }, + ]; + } + break;src/Components/CreateImageWizard/utilities/requestMapper.ts (2)
174-216: Inconsistentmin_sizehandling for disks, VGs, Btrfs, and LVsThe new disk converters mix “raw number” and “
<number> <unit>” formats inmin_size, which breaks the assumptions used by the UI and bygetDisk:
- For volume groups and Btrfs partitions you set
min_size: disk.minsize(full string like"2 GiB"), while plain partitions and LVs store only the numericsize.MinimumSizeand related UI code assumemin_sizeis digits only and constrain input via/^\d+$/. Whenmin_sizecomes in as"2 GiB", editing becomes effectively impossible.- On the way out,
getDiskexpectsmin_sizeto be numeric for plain partitions and LVs (it concatenatesmin_size + ' ' + unit) but for VGs/Btrfs usesmin_sizedirectly, so after a user edit you’ll send"2"instead of"2 GiB".I’d strongly recommend normalizing the internal shape so that:
DiskPartitionBase.min_sizealways holds the numeric string (orundefined), and- all API
minsizefields are built asmin_size + ' ' + unit(whereunitdefaults sensibly).Conceptually:
- if ('logical_volumes' in disk) { - return { - id, - min_size: disk.minsize, - unit: unit as Units, - ... - }; - } + if ('logical_volumes' in disk) { + return { + id, + min_size: size, + unit: unit as Units, + ... + }; + } - if ('subvolumes' in disk) { - return { - id, - min_size: disk.minsize, - unit: unit as Units, - ... - }; - } + if ('subvolumes' in disk) { + return { + id, + min_size: size, + unit: unit as Units, + ... + }; + }and mirror that in
getDiskfor all disk types.Also applies to: 218-235, 385-392
876-918: VGminsizeshould be built frommin_size+unitIn
getDisk, the LVM branch currently sends:minsize: partition.min_size ? partition.min_size : undefined,while logical volumes and plain partitions send
min_size + ' ' + unit. After the UI edits a VG’s minimum size, API will see"2"instead of"2 GiB", which is inconsistent with both the API types and the LV/plain partition handling.Once
DiskPartitionBase.min_sizeis normalized to a numeric string, this branch should follow the same pattern:- minsize: partition.min_size ? partition.min_size : undefined, + minsize: + partition.min_size && partition.unit + ? `${partition.min_size} ${partition.unit}` + : undefined,
🧹 Nitpick comments (2)
src/Components/CreateImageWizard/steps/FileSystem/components/MinimumSize.tsx (1)
10-15: Minimum size wiring is correct; disk‑side validation still missingThe widened
partitionunion andcustomizationprop are correctly propagated intochangePartitionMinSizeandchangePartitionUnit, and thepartition.min_size || ''/ default'GiB'unit handling makes sense for newly created disk/LV entries.Note that
useFilesystemValidationstill only validatesfilesystemPartitions, so advanced/diskmin_sizefields (including VGs) won’t currently gate the Next button. This matches the PR description about deferring size validation for disk customization, but it’s worth tracking explicitly.Also applies to: 17-23, 25-26, 32-33, 40-41, 46-58
src/store/wizardSlice.ts (1)
772-796: Partition mutation reducers correctly route between filesystem, disk, and LVsThe updated reducers (
addPartition,removePartition,changePartitionMountpoint,changePartitionUnit,changePartitionMinSize,changePartitionType,changePartitionName) correctly:
- Use
customization: 'disk' | 'fileSystem'to select the right top‑level slice, and- Fall back to scanning
state.disk.partitionsfor LVM logical volumes byid.The defensive checks (
'mountpoint' in ...,'fs_type' in ...,'name' in ...) avoid mutating inappropriate union members. The trade‑off is some repeated LVM‑loop code across reducers; if this grows further, you might want a small helper to encapsulate “find LV by id and apply mutation”, but that’s optional for now.Also applies to: 803-835, 837-894, 895-958
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
playwright/Customizations/Disk.spec.ts(1 hunks)playwright/fixtures/data/exportBlueprintContents.ts(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/DiskRow.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemTable.tsx(4 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/MinimumSize.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/MountpointPrefix.tsx(3 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/MountpointSuffix.tsx(2 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/PartitionName.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/PartitionType.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/SizeUnit.tsx(2 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/VolumeGroups.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts(1 hunks)src/Components/CreateImageWizard/utilities/requestMapper.ts(7 hunks)src/store/wizardSlice.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/Components/CreateImageWizard/steps/FileSystem/components/MountpointPrefix.tsx
- src/Components/CreateImageWizard/steps/FileSystem/components/PartitionType.tsx
- src/Components/CreateImageWizard/steps/FileSystem/components/PartitionName.tsx
- src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemTable.tsx
- playwright/Customizations/Disk.spec.ts
- playwright/fixtures/data/exportBlueprintContents.ts
- src/Components/CreateImageWizard/steps/FileSystem/components/VolumeGroups.tsx
- src/Components/CreateImageWizard/steps/FileSystem/components/DiskRow.tsx
- src/Components/CreateImageWizard/steps/FileSystem/components/MountpointSuffix.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
src/Components/CreateImageWizard/steps/FileSystem/components/MinimumSize.tsx (4)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (4)
FilesystemPartition(10-15)LogicalVolumeWithBase(33-33)VolumeGroupWithExtendedLV(35-41)PartitioningCustomization(8-8)src/store/hooks.ts (1)
useAppDispatch(6-6)src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useFilesystemValidation(304-351)src/Components/CreateImageWizard/ValidatedInput.tsx (1)
ValidatedInputAndTextArea(47-118)
src/Components/CreateImageWizard/utilities/requestMapper.ts (3)
src/store/service/imageBuilderApi.ts (5)
Filesystem(673-677)FilesystemTyped(679-690)VolumeGroup(718-728)BtrfsVolume(699-706)LogicalVolume(707-717)src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (3)
FilesystemPartition(10-15)DiskPartition(43-46)Units(17-17)src/store/wizardSlice.ts (3)
selectDiskMinsize(445-447)selectDiskPartitions(449-451)selectDiskType(441-443)
src/Components/CreateImageWizard/steps/FileSystem/components/SizeUnit.tsx (1)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (5)
FilesystemPartition(10-15)LogicalVolumeWithBase(33-33)VolumeGroupWithExtendedLV(35-41)PartitioningCustomization(8-8)Units(17-17)
src/store/wizardSlice.ts (2)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (7)
FscDisk(21-25)FilesystemPartition(10-15)PartitioningCustomization(8-8)Units(17-17)FSType(19-19)DiskPartition(43-46)DiskPartitionBase(27-31)src/store/service/imageBuilderApi.ts (1)
LogicalVolume(707-717)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: playwright-tests
- GitHub Check: Service Tests
🔇 Additional comments (5)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (1)
1-6: Type refactor is coherent and future‑proofThe new
PartitioningCustomization,FscDisk,DiskPartitionBase, andDiskPartitiontypes align cleanly with the API types and the updated reducers/selectors. This also centralizes the'disk' | 'fileSystem'union that was previously repeated in several places, which should make future changes to partitioning flows easier.Also applies to: 8-8, 10-15, 17-31, 33-46
src/Components/CreateImageWizard/utilities/requestMapper.ts (1)
160-172: Filesystem → wizard partition mapping looks good
convertFilesystemToPartition’s use ofparseSizeUnitand theFilesystemPartitionshape is consistent withgetFileSystem(which later converts back to bytes withgetConversionFactor). This should round‑trip basic filesystem customizations cleanly.src/store/wizardSlice.ts (2)
11-16: State/type restructuring for disk and filesystem looks soundSwitching
wizardState.disktoFscDiskandfileSystem.partitionstoFilesystemPartition[], together with the new imports fromfscTypes, is consistent with the revamped fsc model and requestMapper. Initial state fordisk(minsize: '',partitions: [],type: undefined) also matches howgetDiskand the new disk reducers expect to operate.Also applies to: 19-28, 90-143, 198-255
959-1047: Disk‑specific reducers and exports align with new disk modelThe new disk actions (
changeDiskMinsize,changeDiskType,addDiskPartition,removeDiskPartition,changeDiskPartitionMinsize,changeDiskPartitionName,addLogicalVolumeToVolumeGroup,changePartitioningMode) operate purely onstate.diskand respect theDiskPartition/DiskPartitionBaseunions. The two‑phase delete inremoveDiskPartition(root partition, then LV) is a reasonable compromise given the current single‑VG assumption.Exports at the bottom correctly expose these new actions for use by the wizard components and requestMapper.
Also applies to: 1442-1553
src/Components/CreateImageWizard/steps/FileSystem/components/SizeUnit.tsx (1)
16-22: Customization wiring and union types look consistentThe widened
partitionunion and addedcustomizationprop are correctly threaded into bothchangePartitionMinSizeandchangePartitionUnitpayloads, matching the updated reducers. Only thing to watch is that for any new partitions you always initializeunitbefore renderingSizeUnit; otherwise the toggle label will be blank until the first update.If you want to double‑check, you can grep for all call sites of
SizeUnitand confirm they only pass partitions with a definedunit.Also applies to: 27-31, 34-34, 40-57
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemTable.tsx (1)
199-199: TbodyonDrophandler is assigned toonDragOverinstead ofonDrop.Line 199 uses
onDrop={onDragOver}, which only prevents default behavior and updates visual feedback. TheonDrophandler (lines 88-94) that dispatcheschangePartitionOrderis only attached to individual Row and DiskRow components. If a drop lands directly on the<Tbody>element (not on a specific row), the order change won't be committed to the store.Change line 199 to
onDrop={onDrop}to ensure consistent behavior across all drop scenarios.
♻️ Duplicate comments (7)
src/Components/CreateImageWizard/steps/FileSystem/components/PartitionName.tsx (1)
14-41: Volume group name errors won’t display due to fieldName mismatch
useFilesystemValidationsets errors for:
- Volume groups:
errors["name-${partition.id}"]- Logical volumes:
errors["lvname-${lv.id}"]This component always uses
fieldName={\lvname-${partition.id}`}for both(VolumeGroup & DiskPartitionBase)andLogicalVolumeWithBase`. As a result, invalid volume group names will block “Next” but won’t show an inline error, which is confusing UX and makes it hard to see why the step is blocked.You can fix this by switching the field name based on whether the partition is a volume group or logical volume:
type PartitionNamePropTypes = { partition: (VolumeGroup & DiskPartitionBase) | LogicalVolumeWithBase; customization: PartitioningCustomization; }; @@ const stepValidation = useFilesystemValidation(); + const isVolumeGroup = 'logical_volumes' in partition; + const fieldName = isVolumeGroup + ? `name-${partition.id}` // Volume group + : `lvname-${partition.id}`; // Logical volume + return ( <ValidatedInputAndTextArea ariaLabel='Partition name input' @@ }} stepValidation={stepValidation} - fieldName={`lvname-${partition.id}`} + fieldName={fieldName} /> );src/Components/CreateImageWizard/validators.ts (1)
52-54: Regex validation issue already tracked for follow-up.As noted in the previous review, the regex allows invalid LVM names (starting with hyphen, '.' and '..').
playwright/Customizations/Disk.spec.ts (1)
237-272: Known test structure issue acknowledged for later.As noted in previous review,
fillInImageOutputGueststarts a new wizard flow rather than validating the imported blueprint. Developer marked this as out of scope for this PR.src/Components/CreateImageWizard/steps/FileSystem/components/AdvancedPartitioning.tsx (1)
96-105: Missing unit selector tracked for follow-up.As noted in previous review, the minimum disk size input lacks a unit selector and validation. This is acknowledged for a follow-up.
src/Components/CreateImageWizard/utilities/requestMapper.ts (2)
876-918: LV and plain-partitionminsizecomposition could produce"undefined"fragmentsIn
getDisk, when composingminsize:
- For logical volumes:
minsize: lv.min_size + ' ' + lv.unit- For plain partitions:
minsize: partition.min_size + ' ' + partition.unitIf either
min_sizeorunitis unset, this yields strings like"undefined GiB"or"1 undefined", which are not valid for the API and may be hard to debug.You might want to guard similarly to the earlier Graphite suggestion, e.g.:
minsize: lv.min_size && lv.unit ? `${lv.min_size} ${lv.unit}` : undefined,and the same pattern for plain partitions.
174-216: Unclearmin_sizesemantics for LVM and Btrfs partitions (inconsistent with plain partitions)In
convertDiskToFscDisk:
- LVM branch (Lines 185‑195):
min_sizeis set todisk.minsize(full string like"2 GiB"), even though you already splitdisk.minsizeintosize/unitabove.- Btrfs branch (Lines 198‑205): same pattern (
min_size: disk.minsize).- Plain branch (Lines 208‑215): uses parsed
sizeandunit, somin_sizeis just the numeric portion.Later,
getDiskassumes different shapes:
- For VGs and Btrfs, it sends
minsize: partition.min_sizeunchanged.- For plain partitions and logical volumes, it formats
minsizeasmin_size + ' ' + unit.This means:
- Imported blueprints will have
min_sizecontaining"N UNIT"for VGs/Btrfs but"N"for plain partitions.- Newly edited VGs/Btrfs (where the UI likely treats
min_sizeas numeric) can produce strings without units going back to the API, or vice versa, depending on how the form is wired.Given the API expects
minsizein"N UNIT"form, a more consistent approach would be:
- Use parsed
sizeformin_sizein all internal types (LVM, Btrfs, plain).- Always recombine
min_size + ' ' + unitwhen mapping back to the API for every variant, guarding for missing values.I know this overlaps with the existing Graphite comments on
min_size/minsizehandling you already plan to address, but calling it out here because it affects both import and export paths.src/store/wizardSlice.ts (1)
723-755: Advanced mode still resets disk layout on entryIn
changeFscMode, when switching to'advanced'you unconditionally overwritestate.disk.partitionswith a single default/plain partition:case 'advanced': state.disk.partitions = [ { id: uuidv4(), mountpoint: '/', fs_type: 'ext4', min_size: '1', unit: 'GiB', type: 'plain', }, ]; break;This means any existing advanced layout is discarded whenever the user leaves advanced mode and comes back, without an explicit “reset” action.
If preserving the current advanced configuration across mode switches is desired, consider seeding only when
state.disk.partitionsis empty. I realize this echoes earlier feedback you’ve already discussed; reiterating here so it doesn’t get lost as the reducers evolve.
🧹 Nitpick comments (5)
src/Components/CreateImageWizard/steps/FileSystem/components/PartitionType.tsx (1)
19-71: Improve typing and toggle handling inPartitionTypeThe behavior looks correct, but there are a couple of easy wins:
- Type
fs_typesasFSType[](and theselectionparameter accordingly) to keep the compiler aligned with the allowed filesystem types.- Use the functional state setter for the toggle (
setIsOpen((prev) => !prev)) and consider relying solely ononOpenChangefor open/close to avoid potential state desync between the toggle click handler and PatternFly’s internal open tracking.-const fs_types = ['ext4', 'xfs', 'vfat', 'swap']; +const fs_types: FSType[] = ['ext4', 'xfs', 'vfat', 'swap']; @@ - const onSelect = (event?: React.MouseEvent, selection?: string | number) => { + const onSelect = ( + event?: React.MouseEvent, + selection?: string | number, + ) => { @@ - const onToggleClick = () => { - setIsOpen(!isOpen); - }; + const onToggleClick = () => { + setIsOpen((prev) => !prev); + }; @@ - {fs_types.map((type, index) => ( - <SelectOption key={index} value={type}> + {fs_types.map((type) => ( + <SelectOption key={type} value={type}> {type} </SelectOption>src/Components/CreateImageWizard/steps/FileSystem/components/DiskRow.tsx (1)
45-47: Prefer narrowing DiskPartition instead of casting toFilesystemPartitionYou already guard out
'lvm'and'btrfs'at Lines 45‑47, so at this pointpartitionis effectively the(FilesystemTyped & DiskPartitionBase)variant. Theas FilesystemPartitioninMountpointPrefix/MountpointSuffixis therefore only needed for TypeScript, not runtime.To keep the types honest (and future‑proof if
DiskPartitiongains more variants), consider either:
- Narrowing the prop type for
DiskRow.partitionto the filesystem variant used here, or- Introducing a small type guard (e.g.,
isFilesystemPartitionLike) and letting TS infer the narrowed type instead of usingas.This is non‑blocking, but would remove unnecessary casts and make the intent clearer.
Also applies to: 68-71, 76-80
src/Components/CreateImageWizard/utilities/requestMapper.ts (1)
385-397: Defensive handling for missing disk partitions when importing/exportingIn
commonRequestToStateyou map disk partitions via:partitions: request.customizations.disk.partitions.map((d) => convertDiskToFscDisk(d), ),and in
getDiskyou do:const partitions = selectDiskPartitions(state); const diskPartitions = partitions.map((partition) => { ... });Both assume
partitionsis always defined and non‑null arrays. If the API ever omitsdisk.partitions(e.g., disk customization present but no explicit partitions), or the wizard state holdsundefinedby mistake, these will throw.Consider making these paths more defensive, e.g.:
- Defaulting to an empty array on import (
request.customizations.disk.partitions || []).- Guarding against
undefinedingetDiskor ensuring the slice always initializesdisk.partitionsto[](which you currently do).Given the initial state sets
disk.partitions: [], this is likely only a concern for import robustness.Also applies to: 876-918
src/store/wizardSlice.ts (2)
803-835: Unified partition updates correctly handle both disk and LVM logical volumes
changePartitionMountpoint:
- First looks up the partition by
idunderstate[customization].partitionsand, if that entry has amountpointfield, updates it directly.- If not found (or the entry doesn’t expose
mountpoint), it scansstate.disk.partitionsfor atype === 'lvm'volume group and updates the matching logical volume’smountpoint.This gives you a single action that can update:
- Basic filesystem partitions (
customization: 'fileSystem'),- Plain disk partitions (
customization: 'disk'),- LVM logical volumes (via the fallback path).
The pattern is slightly over‑broad (the LVM scan also runs when
customization === 'fileSystem'), but it’s safe and keeps the action surface small. If you want to micro‑optimize, you could skip the LVM loop whencustomization === 'fileSystem', but it’s not necessary.
838-893: Similar pattern for unit and min_size changes; watch for missing LV entries
changePartitionUnitandchangePartitionMinSizemirror the mountpoint logic:
- Update
state[customization].partitions[partitionIndex]if found.- Otherwise, scan
state.disk.partitionsfortype === 'lvm'and update the matching logical volume.This should correctly propagate changes to both plain partitions and LVs. As above, the LV loop will also run when
customization === 'fileSystem', but that’s harmless. If you ever introduce more disk partition variants, consider extracting a small helper to encapsulate “find LV by id and update field” so that future changes stay consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
playwright/Customizations/Disk.spec.ts(1 hunks)playwright/Customizations/Filesystem.spec.ts(4 hunks)playwright/fixtures/data/exportBlueprintContents.ts(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/AdvancedPartitioning.tsx(2 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/DiskRow.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemConfiguration.tsx(3 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemPartition.tsx(4 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemTable.tsx(4 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/MinimumSize.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/MountpointPrefix.tsx(3 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/MountpointSuffix.tsx(2 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/PartitionName.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/PartitionType.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/Row.tsx(4 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/SizeUnit.tsx(2 hunks)src/Components/CreateImageWizard/steps/FileSystem/components/VolumeGroups.tsx(1 hunks)src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts(1 hunks)src/Components/CreateImageWizard/steps/Oscap/components/useSelectorHandlers.tsx(2 hunks)src/Components/CreateImageWizard/steps/Review/ReviewStepTextLists.tsx(2 hunks)src/Components/CreateImageWizard/utilities/requestMapper.ts(7 hunks)src/Components/CreateImageWizard/utilities/useValidation.tsx(4 hunks)src/Components/CreateImageWizard/validators.ts(4 hunks)src/store/wizardSlice.ts(10 hunks)src/test/Components/Blueprints/ImportBlueprintModal.test.tsx(2 hunks)src/test/Components/CreateImageWizard/steps/FileSystemConfiguration/FileSystemConfiguration.test.tsx(2 hunks)src/test/fixtures/editMode.ts(0 hunks)src/test/fixtures/importBlueprints.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/test/fixtures/editMode.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/test/Components/CreateImageWizard/steps/FileSystemConfiguration/FileSystemConfiguration.test.tsx
- src/Components/CreateImageWizard/steps/FileSystem/components/VolumeGroups.tsx
- src/Components/CreateImageWizard/steps/FileSystem/components/MountpointPrefix.tsx
- src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemConfiguration.tsx
- src/test/Components/Blueprints/ImportBlueprintModal.test.tsx
- src/Components/CreateImageWizard/steps/Oscap/components/useSelectorHandlers.tsx
- src/Components/CreateImageWizard/steps/FileSystem/components/Row.tsx
- playwright/fixtures/data/exportBlueprintContents.ts
- src/Components/CreateImageWizard/steps/FileSystem/components/MinimumSize.tsx
- playwright/Customizations/Filesystem.spec.ts
🧰 Additional context used
🧬 Code graph analysis (14)
src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemTable.tsx (1)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (2)
FilesystemPartition(10-15)DiskPartition(43-46)
src/Components/CreateImageWizard/steps/FileSystem/components/PartitionName.tsx (4)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (3)
DiskPartitionBase(27-31)LogicalVolumeWithBase(33-33)PartitioningCustomization(8-8)src/store/hooks.ts (1)
useAppDispatch(6-6)src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
useFilesystemValidation(304-351)src/Components/CreateImageWizard/ValidatedInput.tsx (1)
ValidatedInputAndTextArea(47-118)
src/Components/CreateImageWizard/validators.ts (1)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (1)
FilesystemPartition(10-15)
src/Components/CreateImageWizard/steps/FileSystem/components/DiskRow.tsx (3)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (2)
DiskPartition(43-46)FilesystemPartition(10-15)src/store/hooks.ts (2)
useAppDispatch(6-6)useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectDiskPartitions(449-451)
src/Components/CreateImageWizard/steps/Review/ReviewStepTextLists.tsx (1)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (1)
FilesystemPartition(10-15)
src/Components/CreateImageWizard/steps/FileSystem/components/AdvancedPartitioning.tsx (2)
src/store/hooks.ts (2)
useAppDispatch(6-6)useAppSelector(7-7)src/store/wizardSlice.ts (2)
selectDiskMinsize(445-447)selectDiskPartitions(449-451)
src/Components/CreateImageWizard/utilities/requestMapper.ts (3)
src/store/service/imageBuilderApi.ts (5)
Filesystem(673-677)FilesystemTyped(679-690)VolumeGroup(718-728)BtrfsVolume(699-706)LogicalVolume(707-717)src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (3)
FilesystemPartition(10-15)DiskPartition(43-46)Units(17-17)src/store/wizardSlice.ts (3)
selectDiskMinsize(445-447)selectDiskPartitions(449-451)selectDiskType(441-443)
src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemPartition.tsx (2)
src/store/hooks.ts (2)
useAppDispatch(6-6)useAppSelector(7-7)src/store/wizardSlice.ts (2)
selectFscMode(437-439)selectPartitioningMode(457-459)
src/Components/CreateImageWizard/steps/FileSystem/components/PartitionType.tsx (2)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (3)
LogicalVolumeWithBase(33-33)PartitioningCustomization(8-8)FSType(19-19)src/store/hooks.ts (1)
useAppDispatch(6-6)
src/Components/CreateImageWizard/steps/FileSystem/components/MountpointSuffix.tsx (1)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (2)
FilesystemPartition(10-15)PartitioningCustomization(8-8)
src/store/wizardSlice.ts (2)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (7)
FscDisk(21-25)FilesystemPartition(10-15)PartitioningCustomization(8-8)Units(17-17)FSType(19-19)DiskPartition(43-46)DiskPartitionBase(27-31)src/store/service/imageBuilderApi.ts (1)
LogicalVolume(707-717)
src/Components/CreateImageWizard/utilities/useValidation.tsx (3)
src/store/hooks.ts (1)
useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectDiskPartitions(449-451)src/Components/CreateImageWizard/validators.ts (1)
isPartitionNameValid(52-54)
playwright/Customizations/Disk.spec.ts (4)
playwright/helpers/wizardHelpers.ts (7)
deleteBlueprint(79-111)registerLater(56-61)fillInDetails(37-49)createBlueprint(16-27)exportBlueprint(117-136)importBlueprint(157-172)fillInImageOutputGuest(67-70)playwright/helpers/login.ts (1)
ensureAuthenticated(43-69)playwright/helpers/navHelpers.ts (3)
navigateToLandingPage(63-69)ibFrame(50-57)fillInImageOutput(18-44)playwright/helpers/helpers.ts (1)
isHosted(22-24)
src/Components/CreateImageWizard/steps/FileSystem/components/SizeUnit.tsx (1)
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (5)
FilesystemPartition(10-15)LogicalVolumeWithBase(33-33)VolumeGroupWithExtendedLV(35-41)PartitioningCustomization(8-8)Units(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (55)
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: Service Tests
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: playwright-tests
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: Sourcery review
🔇 Additional comments (23)
src/Components/CreateImageWizard/steps/Review/ReviewStepTextLists.tsx (1)
88-90: FilesystemPartition wiring for MinSize looks consistentUsing
FilesystemPartition[]forpartitionsand importing the type fromfscTypesmatchesselectFilesystemPartitionsand the waymin_size/unitare consumed inMinSize. No issues from a typing or runtime perspective.Also applies to: 198-200
src/Components/CreateImageWizard/steps/FileSystem/fscTypes.ts (1)
1-46: Disk/FS type modeling looks coherent with validators and selectorsThe new types (
PartitioningCustomization,FSType,FscDisk,DiskPartitionBase,LogicalVolumeWithBase,VolumeGroupWithExtendedLV,DiskPartition) form a clear discriminated union and line up with how validation and selectors consumedisk.partitions. The duplication ofmin_size/unit(UI) andminsize(API) on volume groups is a bit subtle but appears intentional given the importedMinsizetype.src/Components/CreateImageWizard/utilities/useValidation.tsx (1)
25-26: Disk partition and LV name validation is correctly integratedThe added
selectDiskPartitionsselector andisPartitionNameValidusage are wired cleanly intouseFilesystemValidation:
- Partition names use
errors["name-${partition.id}"].- LV names under LVM VGs use
errors["lvname-${lv.id}"].disabledNextis set whenever any of these fail, matching the existing filesystem validation behavior.Once
PartitionNameis updated to usename-${partition.id}for VGs (andlvname-${id}for LVs), inline errors will align perfectly with these keys.Also applies to: 63-64, 304-350
src/Components/CreateImageWizard/validators.ts (1)
68-68: LGTM: Clean type migration.The function signatures correctly updated to use
FilesystemPartition[], aligning with the broader refactor.Also applies to: 103-105
src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemPartition.tsx (2)
72-97: Nested radio implementation looks correct.The raw/lvm partitioning mode radios are properly wired with dispatch actions and conditional rendering. The initialization concern for
partitioningModeis already tracked for follow-up.
51-51: Conditional rendering aligns with feature gating strategy.The logic correctly exposes advanced partitioning in on-prem while keeping it feature-flagged in hosted environments.
Also applies to: 62-62
playwright/Customizations/Disk.spec.ts (2)
45-75: Test structure validates basic UI correctly.The commented-out raw partitioning checks are reasonable pending confirmation of the auto-lvm radio design.
215-235: Export/import flow correctly implemented.The blueprint path is properly captured and passed to the import step, with cleanup registered.
src/Components/CreateImageWizard/steps/FileSystem/components/AdvancedPartitioning.tsx (2)
30-66: Handler functions correctly create partition structures.UUID generation and default values are appropriate for both plain partitions and LVM volume groups.
125-136: Single volume group constraint correctly enforced.The conditional rendering hides the "Add LVM volume group" button when one already exists, aligning with the design decision to support a single VG for simplicity while making multiple VGs easy to enable later.
src/Components/CreateImageWizard/steps/FileSystem/components/MountpointSuffix.tsx (1)
7-7: LGTM: Customization prop correctly integrated.The component properly accepts and forwards the
customizationprop to the dispatch action, maintaining type safety with the updatedFilesystemPartitiontype.Also applies to: 11-18, 33-33
src/Components/CreateImageWizard/steps/FileSystem/components/FileSystemTable.tsx (2)
22-32: Type system and guards are well-designed.The discriminated union and type guard cleanly separate filesystem from disk partitions, ensuring
rootPartitionsCountonly considers filesystem entries for the single-root protection.Also applies to: 164-174
203-224: Conditional rendering correctly separates filesystem and disk modes.The mode-based branching properly renders
Rowfor filesystem partitions with root protection andDiskRowfor disk partitions.src/test/fixtures/importBlueprints.ts (1)
284-284: LGTM: Fixture values are valid and consistent.The concrete mountpoints and names replace placeholder "string" values with valid paths ("/", "/home") and names ("homesv", "homelv", "mainvg") that align with the disk customization test scenarios.
Also applies to: 291-292, 302-303, 307-307
src/Components/CreateImageWizard/steps/FileSystem/components/SizeUnit.tsx (2)
27-34: LGTM: Type union and customization prop correctly implemented.The partition type union covers all variants, and the
customizationprop is properly integrated into the component signature.
40-58: Dispatch logic correctly threads customization.The unit selection handlers properly include the
customizationprop in bothchangePartitionMinSizeandchangePartitionUnitactions, with appropriate fallback for empty min_size values.src/Components/CreateImageWizard/utilities/requestMapper.ts (1)
160-172: Filesystem import helper looks consistent with basic-mode semantics
convertFilesystemToPartitioncorrectly splitsfilesystem.min_sizeinto a numeric string and unit viaparseSizeUnit, and stores them inFilesystemPartition. This matches howgetFileSystemlater recomputes bytes usingmin_sizeandunit, so basic filesystem round‑tripping should behave as expected.src/store/wizardSlice.ts (6)
140-143: Disk/fileSystem state typing matches new fscTypesThe changes to
wizardState.disk(FscDisk) andfileSystem.partitions(FilesystemPartition[]), along with the matchinginitialStateshape, look consistent with the new per‑filesystem/disk typing infscTypes. This aligns with howrequestMapper.getDiskandgetFileSystemconsume this state.Also applies to: 198-207
772-796: Partition add/remove for basic filesystem mode looks correct
addPartition,removePartition, andremovePartitionByMountpointnow operate onFilesystemPartition[], and the logic is straightforward push/find/splice. This matches the new basic‑mode state shape and should behave the same as before with the updated types.
895-958: Name/type updates are carefully guarded to only touch variants that support those fieldsIn
changePartitionTypeandchangePartitionNameyou:
- Check for a partition with the given
id,- Guard with
'fs_type' in ...or'name' in ...before assigning,- Fall back to scanning LVM
logical_volumesfor theidand updating there.This avoids accidentally writing fields onto filesystem‑only partitions or Btrfs volumes that don’t support
name/fs_type, while still supporting VG and LV edits. The guards make sense given theDiskPartitionunion.
959-1041: Disk-level minsize/type and logical-volume addition look soundThe new disk‑specific actions:
changeDiskMinsizeandchangeDiskTypesimply mutatestate.diskfields.addDiskPartition/removeDiskPartitionmanage top‑leveldisk.partitions, withremoveDiskPartitionalso handling removal of a logical volume by id when it belongs to a VG.addLogicalVolumeToVolumeGroupappends aLogicalVolume & DiskPartitionBaseto the appropriate VG, after checking that the partition exposeslogical_volumes.This matches how
getDiskexpects to read disk state and should be sufficient for the LVM UI flows you’ve described.
995-1005: Dedicated disk-partition setters are narrow and predictable
changeDiskPartitionMinsizeandchangeDiskPartitionNamework only on top‑leveldisk.partitions(they don’t try to reach into LVM LVs), and both guard on the presence of the corresponding field (namein partition). That’s a good separation from the more genericchangePartitionMinSize/changePartitionNameactions and keeps the semantics clear.Also applies to: 1007-1021
1042-1047: New partitioning_mode wiring is straightforward
changePartitioningModeupdatesstate.partitioning_mode, and the action is exported alongside the other wizard actions. Combined with the newselectPartitioningModeselector and its use ingetCustomizations, this should give you the expected request payload field.Also applies to: 1442-1553
|
Ok, temporarily disabled the test checking the on-prem exported blueprint - disk is currently not being written into the blueprint at all. I know what the issue is and have some kind of idea about how to fix it, but don't want to make this PR even bigger as the export wasn't in the original scope. Will address it in a follow up PR. |
6 participants
This is ready for initial review - the log of changes already crept far outside initial estimations. What I'd ask for first - just click through the changes in UI/on-prem and let's see if something obvious is missing or if there are any no-goes / breaking changes. I expect this will need to go back to draft several times.
If possible I'd like to split some functionality into follow ups (for the sake of hopefully easier reviews), I've put together list of stuff that still needs to be implemented. Some things could be done separately, some probably couldn't...
What's missing so far:
Will be addressed in a follow up: