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
[SLE-15-SP1] New Storage UI #614
Conversation
* It uses AutoinstProfile classes as source of information. * It does not allow any change (yet).
* Keep the current page after redrawing. * Refreshes the tree with up-to-date information.
* For the time being, we will keep the original behaviour.
* The handle_all_events was accidentally removed.
950c5c9
to
2904ee8
Compare
2904ee8
to
64a5120
Compare
@@ -303,6 +298,9 @@ rmdir $RPM_BUILD_ROOT/%{_prefix}/share/doc/packages/autoyast2/html/autoyast | |||
%dir %{yast_libdir}/autoinstall/dialogs | |||
%{yast_libdir}/autoinstall/dialogs/*.rb | |||
|
|||
%{yast_libdir}/autoinstall/widgets | |||
%{yast_libdir}/autoinstall/presenters | |||
|
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.
This is done in a different way than the dialogs
and clients
directories, that have a dir
entry for the dir and then the *.rb
line for the files. I don't know if any of the methods is better than the other, but the difference looks strange.
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 realize about that, but instead of "duplicating" the lines that already are in the src/Makefile.am, I preferred to wait for comments in the PR review. And it worked.
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.
As discussed in IRC, let's keep it as it is.
Using the @jreidinger words,
as general rule use minimal working one. Only advantage of two lines approach is that it catches for you if you add to git some thrash like y2log file as it does not match *.rb pattern
src/lib/autoinstall/widgets.rb
Outdated
# @!macro [new] seeDialog | ||
# @see http://www.rubydoc.info/github/yast/yast-yast2/CWM%2FDialog:${0} | ||
# @!macro [new] seeComboBox | ||
# @see http://www.rubydoc.info/github/yast/yast-yast2/CWM%2FComboBox:${0} |
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.
Only macro definitions?
I would have expected that require "autoinstall/widgets"
would load the Autoinstall::Widgets
namespace and would give me access to all the classes defined there (similar to require "autoinstall/presenters"
).
If this is just a collection of yardoc macros it likely does not deserves to be called like the namespace.
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.
As discussed offline, widgets.rb will be renamed to yard_doc.rb since it is there only for fix a YARD limitation. Also I'll include the clarification about why it works
because YARD sorts by filename size(!)
Thanks a lot for the help.
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.
LGTM
This PR revamps the AutoYaST storage UI, adapting it to the storage-ng features.
Actually, we started working from the
SLE-15-SP2
codebase, but in order to follow the maintenance branches workflow, I picked commits from the branch and adapted them properly when needed. Mainly differences are:luks1
is the only encryption method available inSLE-15-SP1
. So, it usescrypt_fs
(true or false) attribute to set the partition encryption, instead of thecrypt_method
selector.Changes fixes https://bugzilla.suse.com/show_bug.cgi?id=1136454
For more info, see