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

Remove the storage object from TUI #2193

Closed
wants to merge 22 commits into from

Conversation

poncovka
Copy link
Contributor

DO NOT MERGE!

TODO:

  • Remove the local storage object from GUI.
  • Remove storage from method arguments and instance attributes.
  • Split the storage-related code between the Storage module and UI.

@poncovka poncovka added master Please, use the `f39` label instead. blocked Don't merge this pull request! labels Oct 21, 2019
Don't create the local storage object in the Anaconda class.
We will use the Storage module instead.
Remove the support for reset of the local storage object. Run the
DBus task of the Storage module instead.
Don't initialize and set up the local storage object during the start
of anaconda. Use the Storage module instead.
Set up the Storage module instead.
Call the method CreateWithTask of the Snapshot module to create
the pre-installation snapshot.
Some of the functions for the storage initialization are used by UI, so they
should use the Storage module instead of the local storage object.
The class DasdFormatting should use only the Storage module.
Use DBus tasks of the Storage module for the storage and bootloader configuration.
@poncovka
Copy link
Contributor Author

The pylint issues in GUI will be fixed in the next PR.

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

AFAICT it looks great to me. I just found a few nitpicks below.

@@ -473,9 +475,9 @@ def filter_disks_by_names(disks, names):

:param disks: a list of disks
:param names: a list of disk names
:return: a list of filtered disks
:return: a list of filtered names
Copy link
Member

Choose a reason for hiding this comment

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

Please write here disk names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

]
# Get ancestors.
ancestor_names = filter_disks_by_names(disks, [
a for d in selected_disks for a in device_tree.GetDeviceAncestors(d)
Copy link
Member

Choose a reason for hiding this comment

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

Could you please rewrite this part. The for...for syntax is pretty confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

:param storage: an instance of the storage
:param selected: a list of selected disks
:param disks: a list of named of selected disks
Copy link
Member

Choose a reason for hiding this comment

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

I guess you have a typo here: a list of named should probably be a list of names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Some of the functions of the storage utils are used by UI, so they should use
the Storage module instead.
Use the Storage module instead.
Remove the implementation of the function configure_storage.
All storage-related kickstart data are generated by the Storage module now.
All storage-related kickstart commands are handled by the Storage module now.
The partitioning module has to be applied in the Storage module to be used.
The Enabled property is useless now.
The UI creates the partitioning modules on demand, so there should be
no pre-created ones.
Remove the local storage object from some parts of GUI to fix the pylint issues.
@poncovka
Copy link
Contributor Author

Updated.

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.

@jkonecny12
Copy link
Member

Still looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Don't merge this pull request! master Please, use the `f39` label instead.
3 participants