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

/boot/efi support for mount point assignment #5066

Merged
merged 4 commits into from
Aug 30, 2023

Conversation

vojtechtrefny
Copy link
Contributor

This makes sure /boot/efi is added to required mount points on EFI systems and adds an check to make sure it is mounted on a correct device/format.
I also added support for creating a EFI VM for the tests, I am not 100 % sure this will work in CI so creating this as a draft for now.

@vojtechtrefny vojtechtrefny force-pushed the master_uefi-support branch 5 times, most recently from f184584 to c68eb4e Compare August 22, 2023 11:26
@vojtechtrefny vojtechtrefny marked this pull request as ready for review August 23, 2023 11:42
@AdamWill
Copy link
Contributor

oof. this seems really kinda inelegant. don't we already have a whole bunch of code for figuring out what mount points are required for what platforms and so on? why are we recreating this ad hoc in the webUI front end code?

Aren't there special requirements on powerpc? We do produce a Workstation live image for PowerPC: do we now have to add more ad hoc recreations of the powerpc boot partition requirements too?

@AdamWill
Copy link
Contributor

Filed https://bugzilla.redhat.com/show_bug.cgi?id=2234967 as an F39 Beta FE bug.

@vojtechtrefny
Copy link
Contributor Author

oof. this seems really kinda inelegant. don't we already have a whole bunch of code for figuring out what mount points are required for what platforms and so on? why are we recreating this ad hoc in the webUI front end code?

The code for required mount points lives in the backend and was never needed in the front end so making it available in the WebUI will require more work. I plan to do this the right way, but it will take more time. This is definitely an ugly hack, but I wanted to have this in the WebUI as soon as possible. You'll get an error before starting the installation about missing /boot/efi (or other required partitions on other platforms) from the backend so this is mostly about showing the mount point in the UI and not waiting for the last moment that something is missing.

@vojtechtrefny vojtechtrefny force-pushed the master_uefi-support branch 3 times, most recently from cce96e9 to c73360d Compare August 28, 2023 12:47
@jkonecny12
Copy link
Member

Copy link
Contributor

@M4rtinK M4rtinK left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! :)

ui/webui/test/machine_install.py Outdated Show resolved Hide resolved
ui/webui/src/components/storage/MountPointMapping.jsx Outdated Show resolved Hide resolved
@vojtechtrefny
Copy link
Contributor Author

/kickstart-test --testtype smoke

@M4rtinK
Copy link
Contributor

M4rtinK commented Aug 30, 2023

/kickstart-test --waive webui only

@vojtechtrefny vojtechtrefny merged commit e40c5e4 into rhinstaller:master Aug 30, 2023
14 checks passed
@AdamWill
Copy link
Contributor

AdamWill commented Sep 1, 2023

This does not fully fix the problem. It fixes it on only the first trip through the mount point assignment screen. If you contrive to return to it - e.g. by clicking Back then Next, or clicking Installation method on the left and proceeding to Mount point assignment again - /boot/efi never again shows up as Required.

@vojtechtrefny
Copy link
Contributor Author

This does not fully fix the problem. It fixes it on only the first trip through the mount point assignment screen. If you contrive to return to it - e.g. by clicking Back then Next, or clicking Installation method on the left and proceeding to Mount point assignment again - /boot/efi never again shows up as Required.

I've created #5121 which should fix this and also does gets the mountpoints from the backend instead of manually checking /sys/firmware/efi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants