webui: mount point assignment support #4843

KKoukiou · 2023-06-19T21:25:58Z

Enhanced version of #4757

I made many changes that I felt maybe force pushing into the existing branch is bad idea. Let me know if you prefer.

@jelly kept your commit an d fixed the issue with the custom mountpoint option still showing plain disks.

Wanna take a look?

ui/webui/src/components/storage/CustomMountPoint.jsx

ui/webui/src/components/storage/InstallationDestination.jsx

jelly · 2023-06-21T10:32:43Z

Some UI things I did not address, which can be a follow up:

Allows selection when you have no partitions

Allows selection, when you have a partition but no Filesystem

After going back and adding a new partition, the file system was not detected.

I think this is due to the re-use of the ManualPartitioning so when we click on Detect disks we should probably remove the old ManualPartitioning model.

Note that every time you change something with the partitions you need to go back and click Detect disks, adding a button everywhere to detect disks is a solution but not super nice if you ask me.

Error stays after switching back and forth.

When switching back and forth because I had a missing filesystem, then created one and clicked Detect disks I noticed the error is still shown:

Nice to have

Formatting a partition as ntfs and assigning it as / gives:

This could be an inline error, downside is that we'd have to hardcode a rule in the web ui.

jelly · 2023-06-21T12:01:11Z

ui/webui/src/components/storage/CustomMountPoint.jsx

+
+ useEffect(() => {
+ if (requests === null) {
+ findPartitioning({ method: "MANUAL" }).then(([partitioning]) => {


TLDR:

When scanning for new devices we correctly call ResetPartitioning but this does not pick up filesystem changes, only new partitions. (As GatherRequests which is called here does the right thing)

@poncovka I assume this is not expected behaviour but a bug?

Creating a new Partitining does pick up the filesystem changes on disk:

[anaconda root@localhost ~]# busctl --verbose --address=$(cat /run/anaconda/bus.address) call org.fedoraproject.Anaconda.Modules.Storage /org/fedoraproject/Anaconda/Modules/Storage org.fedoraproject.Anaconda.Modules.Storage CreatePartitioning s "MANUAL" [anaconda root@localhost ~]# busctl --verbose --address=$(cat /run/anaconda/bus.address) call org.fedoraproject.Anaconda.Modules.Storage /org/fedoraproject/Anaconda/Modules/Storage/Partitioning/3 org.fedoraproject.Anaconda.Modules.Storage.Partitioning.Manual GatherRequests | grep ext4 STRING "ext4"; STRING "ext4";

Old partitioning model:

[anaconda root@localhost ~]# busctl --verbose --address=$(cat /run/anaconda/bus.address) call org.fedoraproject.Anaconda.Modules.Storage /org/fedoraproject/Anaconda/Modules/Storage/Partitioning/2 org.fedoraproject.Anaconda.Modules.Storage.Partitioning.Manual GatherRequests | grep ext4 [anaconda root@localhost ~]#

@KKoukiou so this is where we find the "existing" ManualPartitioning. If I recall correctly you can't deleted CreatedPartitioning models so findPartitioning just finds the last one. So this is a bit of an issue, it can be fixed in two ways I think.

Either invalidate the PartioningModel's Requests property so it fetches the new information from the DeviceTree. I thought ResetPartitioning should do that But it does not seem so.

Keep some global state somewhere, so create a ManualPartitioning object when you select Custom Mount point and if you click Rescan devices. This is however a rather suboptimal solution I'd say.

Listing Partitionings:

[anaconda root@localhost ~]# busctl --verbose --address=$(cat /run/anaconda/bus.address) get-property org.fedoraproject.Anaconda.Modules.Storage /org/fedoraproject/Anaconda/Modules/Storage org.fedoraproject.Anaconda.Modules.Storage CreatedPartitioning ARRAY "o" { OBJECT_PATH "/org/fedoraproject/Anaconda/Modules/Storage/Partitioning/1"; OBJECT_PATH "/org/fedoraproject/Anaconda/Modules/Storage/Partitioning/2"; };

Getting the GatherRequests (Turns should turn into a Requests property).

busctl --verbose --address=$(cat /run/anaconda/bus.address) call org.fedoraproject.Anaconda.Modules.Storage /org/fedoraproject/Anaconda/Modules/Storage/Partitioning/2 org.fedoraproject.Anaconda.Modules.Storage.Partitioning.Manual GatherRequests

We should be able to call ResetPartitioning to Reset the Partitioning model.

busctl --verbose --address=$(cat /run/anaconda/bus.address) call org.fedoraproject.Anaconda.Modules.Storage /org/fedoraproject/Anaconda/Modules/Storage org.fedoraproject.Anaconda.Modules.Storage ResetPartitioning

And then call GatherRequests again, but this does not seem to discover new filesystems.

M4rtinK · 2023-06-21T12:20:27Z

Would it perhaps make sense to split the PR into more commits ? Say one commit for the JS API code calling DBus, one for the review screen & similar ?

KKoukiou · 2023-06-22T09:45:28Z

Would it perhaps make sense to split the PR into more commits ? Say one commit for the JS API code calling DBus, one for the review screen & similar ?

But we need the front end to make the JS APIs call for dbus testable.

M4rtinK · 2023-06-26T13:17:23Z

/boot-iso --webui

ui/webui/src/components/storage/CustomMountPoint.jsx

M4rtinK · 2023-06-27T11:59:57Z

/boot-iso --webui

ui/webui/src/actions/storage-actions.js

M4rtinK · 2023-06-28T09:21:35Z

/boot-iso --webui

This get's reprinted on each re-render of the component polluting the browser console and therefore the test run outputs.

ui/webui/src/components/storage/CustomMountPoint.jsx

KKoukiou · 2023-06-28T15:05:05Z

@jelly i need some support on this. Checked your review comments from above:

no custom mountpoint section is disabled if no partitions on the device
when no f ilesystem on the partition I still show it - and we get then the error when clicking next as you suggest. Mind if we fi this in followup?
errors don't get discared currently when switching between pages indeed. Since a change in this approach is affecting the whole app, not just this feature let's please check this as well seperately
Parsing the errors to make this inline can be considered - but let's keep it as well for enhancement.

M4rtinK

Looks good to me - quite the piece of work @KKoukiou & @jelly! :)

Just a couple minor remarks. :)

ui/webui/src/apis/storage.js

M4rtinK · 2023-06-28T16:26:04Z

ui/webui/src/components/storage/CustomMountPoint.jsx

+
+const MountPointSelect = ({ partition, requests, mountpoint, handleOnSelect, isDisabled }) => {
+ // TODO: extend?
+ const defaultOptions = [


No really relevant for this review, but I'm thinking this might eventually end up in the config file, as various distro variants might have very different ideas of what the suggested partitions should be (eq. swap, separate /var, /boot not on separate partition, etc.) & might want to set it up in a custom manner.

M4rtinK · 2023-06-28T16:39:24Z

ui/webui/src/components/storage/HelpAutopartOptions.jsx

+ <TextContent>
+ <Text component={TextVariants.p}>
+ {_("This option requires you to first partition and format your disk and " +
+ "then assign custom mount points in the next step.")}


I suggest also mentioning (possibly in another paragraph) that if the layout already exists (from an existing installation or already created by the user beforehand) that the user can proceed right away to assign the mount points.

ui/webui/src/components/storage/StorageConfiguration.jsx

Support a new storage configuration method where a user first has to manually partition and create file systems using the tools on the installation image. When selecting the custom mount point option the user can now select which partition to map to mount point either from a well known list of Mount points or a manually created one. Co-authored-by: Katerina Koukiou <kkoukiou@redhat.com>

…scan button Previously we were replacing the wizard content with a loading empty state but that causes a flicker when rescanning disks.

KKoukiou · 2023-06-29T14:13:24Z

/kickstart-test --waive webui only

KKoukiou requested a review from jelly June 19, 2023 21:26

github-actions bot added f39 webui labels Jun 19, 2023

KKoukiou commented Jun 20, 2023

View reviewed changes

ui/webui/src/components/storage/CustomMountPoint.jsx Outdated Show resolved Hide resolved

KKoukiou force-pushed the custom-mount-point branch 2 times, most recently from 0f55651 to b911384 Compare June 20, 2023 08:49

rvykydal reviewed Jun 21, 2023

View reviewed changes

ui/webui/src/components/storage/InstallationDestination.jsx Outdated Show resolved Hide resolved

KKoukiou requested a review from rvykydal June 21, 2023 08:40

jelly reviewed Jun 21, 2023

View reviewed changes

KKoukiou force-pushed the custom-mount-point branch from b911384 to 8b2377b Compare June 26, 2023 16:46

KKoukiou commented Jun 26, 2023

View reviewed changes

ui/webui/src/components/storage/CustomMountPoint.jsx Outdated Show resolved Hide resolved

KKoukiou force-pushed the custom-mount-point branch from 8b2377b to a0baf0e Compare June 27, 2023 10:32

KKoukiou force-pushed the custom-mount-point branch 2 times, most recently from 0766591 to 651ad21 Compare June 27, 2023 13:54

github-advanced-security bot found potential problems Jun 27, 2023

View reviewed changes

ui/webui/src/actions/storage-actions.js Fixed Show fixed Hide fixed

KKoukiou force-pushed the custom-mount-point branch 3 times, most recently from 2b2b038 to 39bb620 Compare June 27, 2023 16:07

webui: remove forgotten console.info object

91c86ee

This get's reprinted on each re-render of the component polluting the browser console and therefore the test run outputs.

KKoukiou force-pushed the custom-mount-point branch from 39bb620 to 312d20e Compare June 28, 2023 14:32

github-advanced-security bot found potential problems Jun 28, 2023

View reviewed changes

ui/webui/src/components/storage/CustomMountPoint.jsx Fixed Show fixed Hide fixed

KKoukiou mentioned this pull request Jun 28, 2023

Custom mount point #4757

Closed

5 tasks

KKoukiou force-pushed the custom-mount-point branch from 312d20e to 9e51641 Compare June 28, 2023 14:52

KKoukiou force-pushed the custom-mount-point branch from 9e51641 to 775ff4c Compare June 28, 2023 15:28

KKoukiou requested review from M4rtinK and jkonecny12 June 28, 2023 15:29

M4rtinK approved these changes Jun 28, 2023

View reviewed changes

KKoukiou force-pushed the custom-mount-point branch from 775ff4c to 38081fc Compare June 29, 2023 08:51

KKoukiou force-pushed the custom-mount-point branch from 38081fc to 792d3e9 Compare June 29, 2023 08:52

KKoukiou added 2 commits June 29, 2023 12:33

webui: when re-scanning disks or loading initial date disable the re-…

b3ec6db

…scan button Previously we were replacing the wizard content with a loading empty state but that causes a flicker when rescanning disks.

webui: always disable next button if the form is invalid

bf0226b

KKoukiou removed the request for review from rvykydal June 29, 2023 11:22

KKoukiou merged commit 56fcaf7 into rhinstaller:master Jun 29, 2023
14 checks passed

KKoukiou deleted the custom-mount-point branch June 29, 2023 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

webui: mount point assignment support #4843

webui: mount point assignment support #4843

KKoukiou commented Jun 19, 2023 •

edited

Loading

jelly commented Jun 21, 2023 •

edited

Loading

jelly Jun 21, 2023 •

edited

Loading

M4rtinK commented Jun 21, 2023

KKoukiou commented Jun 22, 2023

M4rtinK commented Jun 26, 2023

M4rtinK commented Jun 27, 2023

M4rtinK commented Jun 28, 2023

KKoukiou commented Jun 28, 2023 •

edited

Loading

M4rtinK left a comment

M4rtinK Jun 28, 2023

M4rtinK Jun 28, 2023

KKoukiou commented Jun 29, 2023

webui: mount point assignment support #4843

webui: mount point assignment support #4843

Conversation

KKoukiou commented Jun 19, 2023 • edited Loading

jelly commented Jun 21, 2023 • edited Loading

Allows selection when you have no partitions

Allows selection, when you have a partition but no Filesystem

Error stays after switching back and forth.

Nice to have

jelly Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

M4rtinK commented Jun 21, 2023

KKoukiou commented Jun 22, 2023

M4rtinK commented Jun 26, 2023

M4rtinK commented Jun 27, 2023

M4rtinK commented Jun 28, 2023

KKoukiou commented Jun 28, 2023 • edited Loading

M4rtinK left a comment

Choose a reason for hiding this comment

M4rtinK Jun 28, 2023

Choose a reason for hiding this comment

M4rtinK Jun 28, 2023

Choose a reason for hiding this comment

KKoukiou commented Jun 29, 2023

KKoukiou commented Jun 19, 2023 •

edited

Loading

jelly commented Jun 21, 2023 •

edited

Loading

jelly Jun 21, 2023 •

edited

Loading

KKoukiou commented Jun 28, 2023 •

edited

Loading