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

Get list of required mount points for mount point assignment from backend #5121

Merged

Conversation

vojtechtrefny
Copy link
Contributor

Follow up for #5066 I've added a new backend function which uses modules/storage/platform.py to get the list of required boot-related required mount points.
I opening this as a draft to be able to discuss the backend changes, I plan to make some more changes in the webUI with the additional information we cna get from the platform module (like checking for correct filesystem and device types for the mount points).

@vojtechtrefny vojtechtrefny changed the title Master boot efi backend Get list of required mount points for mount point assignment from backend Sep 4, 2023
Copy link
Contributor

@rvykydal rvykydal 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, thank you.

Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

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

Thank you, for the backend structure/archtecture/form, LGTM.

I don't want to pressure you into renaming all the things, but we don't want to propagate legacy anaconda internals names as API.

pyanaconda/modules/common/structures/storage.py Outdated Show resolved Hide resolved
ui/webui/src/components/AnacondaWizard.jsx Show resolved Hide resolved
pyanaconda/modules/common/structures/storage.py Outdated Show resolved Hide resolved
ui/webui/src/components/storage/MountPointMapping.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

I am not a fan of the PartSpec class and it is definitely a candidate for refactorization, but there was never a good time to do that. So instead of replicating the PartSpec class in the DBus API, it would be better to modernize the DBus structure and eventually apply these changes to the PartSpec class (or replace it completely).

pyanaconda/modules/common/structures/storage.py Outdated Show resolved Hide resolved
pyanaconda/modules/common/structures/storage.py Outdated Show resolved Hide resolved
pyanaconda/modules/common/structures/storage.py Outdated Show resolved Hide resolved
pyanaconda/modules/common/structures/storage.py Outdated Show resolved Hide resolved
@@ -453,3 +453,61 @@ def get_root_device(self):
:return: a device name or None
"""
return self.mount_points.get("/")


class PlatformMountPointData(DBusData):
Copy link
Contributor

Choose a reason for hiding this comment

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

The same structure can eventually used to expose the autopart specification. Please, take it into account when choosing a name of this structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what would be the best name for this, I changed it to RequiredMountPointData and added a better docstring (or at least I hope it is better).

@pep8speaks
Copy link

pep8speaks commented Sep 20, 2023

Hello @vojtechtrefny! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 858:100: E501 line too long (100 > 99 characters)

Comment last updated at 2023-09-22 15:09:33 UTC

@vojtechtrefny vojtechtrefny marked this pull request as ready for review September 21, 2023 08:56
@vojtechtrefny
Copy link
Contributor Author

I hope I addressed all the review comments so I think this is ready for a final review. Thank you for all the suggestions for better function/argument names.

Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

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

I think this is fine.

This uses the Platform class to get list of required partitions/
mount points needed for manual partitioning. Root filesystem is
manually added to the list provided by Platform.partitions.
We now have a backend function to get list of mount points
required for mount point assignment.
The only mount point with a special file system requirement is
currently /boot/efi, but on some systems the requirement can be
different than EFI boot (e.g. hfs+ for older Apple hardware) so we
should use the fstype from the backend instead of hardcoding EFI
in the WebUI.
@vojtechtrefny
Copy link
Contributor Author

/kickstart-test --testtype smoke

@vojtechtrefny vojtechtrefny merged commit e71c198 into rhinstaller:master Sep 25, 2023
14 checks passed
adamkankovsky pushed a commit to adamkankovsky/anaconda that referenced this pull request Sep 25, 2023
…i-backend

Get list of required mount points for mount point assignment from backend
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.

6 participants