-
Notifications
You must be signed in to change notification settings - Fork 355
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
Make boot not required 2 #5381
Conversation
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.
/kickstart-test --testtype smoke |
"""Constrains (filesystem and device types allowed) for mount points required | ||
for the installed system | ||
""" | ||
class MountPointConstraintsData(DBusData): |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.