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

Make boot not required 2 #5381

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

rvykydal
Copy link
Contributor

@rvykydal rvykydal commented Dec 15, 2023

This is a take on #5372 keeping compatibility with current webui. The code is the same as in the PR, just the old API is not changed (/boot is still required in the old API)
The tests should now pass.

The API should provide also information whether a mount point is
required or recommended.
In general it is recommended to have a separate /boot, but not strictly
required.

In some cases it is required but we need to add API for that, because it
depends mainly on the filesystem of the root partition.

So now the requirement regards mainly biosboot and efi partitions, which
is dependent on the platform.
@rvykydal
Copy link
Contributor Author

/kickstart-test --testtype smoke

@rvykydal rvykydal mentioned this pull request Dec 15, 2023
@rvykydal rvykydal marked this pull request as ready for review December 15, 2023 09:39
@rvykydal rvykydal requested a review from KKoukiou December 15, 2023 09:39
"""Constrains (filesystem and device types allowed) for mount points required
for the installed system
"""
class MountPointConstraintsData(DBusData):
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the new name!

if p.mountpoint:
constraint = self._get_mount_point_constraints_data(p)
if p.mountpoint == "/boot":
constraint.recommended = True
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this shouldn't be actually defined in the Storage Constraints section of our configuration files. That section already contains other constraints like this one and it might be useful for Web UI and Cockpit Storage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense to me, but I hesitated to use configuration files perhaps because of the outlook of more dynamic API (like /boot being required depending on fs on /). Nevertheless the static information (/boot (a mount point) required or recommended in general) can still be in the configuration file.

Copy link
Contributor Author

@rvykydal rvykydal Dec 15, 2023

Choose a reason for hiding this comment

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

Something like

boot_partition_recommended = True

or even (not sure if it makes sense in this form) ?

recommended_mount_points = /boot
required_mount_points = /

Well, required_mount_points seems to be covered by existing must_not_be_on_root
Maybe we should wait for more use cases / requirements on required.
Actually, recommended_mount_points might become useful also for /home - seems like a good setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@poncovka would you be OK with handling it as a follow-up? We are dealing with test flakiness on webui part and it could be handy to have this merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want an issue about considering this followup? If this can be handled this year, I would not create one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, it is definitely just a suggestion. I didn't want to block this pull request.

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, please create the issue.

@KKoukiou KKoukiou merged commit 4a9948b into rhinstaller:master Dec 15, 2023
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants