[Review] Request from 'schubi2' @ 'yast/yast-packager/review_170928_disable_remote_installation' #287
Conversation
src/modules/Packages.rb
Outdated
| missing = remote_x11_packages.reject { |tag| pkg_will_be_installed(tag) } | ||
| unless missing.empty? | ||
| @missing_remote_packages << missing | ||
| @missing_remote_kind << "display-ip" |
There was a problem hiding this comment.
NP: the call is Linuxrc.display_ip, the install.inf setting is DISPLAY_IP so I'd stick with display_ip (with an underscore) here.
src/modules/Packages.rb
Outdated
| @missing_remote_kind.join(", "), @missing_remote_packages.join(", ")) | ||
| if Mode.auto | ||
| error_string << " \n" | ||
| error_string << _("But the AutoYaST installation will be finished offline.") |
There was a problem hiding this comment.
NP: the offline term is a bit confusing and not clear here, I'd rather say But the AutoYaST installation will be still finished automatically without any user interaction.
src/modules/Packages.rb
Outdated
| packages.concat(remote_x11_packages) if Linuxrc.display_ip | ||
| missing_remote_packages.flatten! | ||
| unless missing_remote_packages.empty? | ||
| error_string = format(_("Cannot support %s due missing packages %s. It will be disabled."), |
There was a problem hiding this comment.
Put the packages list on a separate line (add \n), it can be quite long...
And maybe use something like Cannot support %s remote access in the installed system due....
src/modules/Packages.rb
Outdated
| error_string << " \n" | ||
| error_string << _("But the AutoYaST installation will be finished offline.") | ||
| end | ||
| log.warn error_string |
There was a problem hiding this comment.
NP: The error string is translated which might be a bit tricky to read (esp. in non-Latin languages). I'd rather prefer logging just the original @missing_remote_kind and @missing_remote_packages values with some untranslated message.
src/modules/Packages.rb
Outdated
| # e.g.: [["kernel-bigsmp", :CAND, :NONE], ["kernel-default", :CAND, :CAND], | ||
| # ["kernel-default", `BOTH, :INST]] | ||
| log.info("provides: #{provides}") | ||
| if provides.any? { |p| p[2] != :NONE } |
There was a problem hiding this comment.
I'd avoid hardcoding the result values and use directly the any? result:
ret = provides.any? { |p| p[2] != :NONE }
log.info("#{tag} will #{ret ? "" : "not "}be installed")
return ret| let(:ssh_packages) { %w(openssh iproute2) } | ||
| let(:braille_packages) { %w(sbl) } | ||
|
|
||
| context "on a boring local regular installation" do |
There was a problem hiding this comment.
I do a lot of boring installations during testing... 😝
src/modules/Packages.rb
Outdated
| missing = ssh_packages.reject { |tag| pkg_will_be_installed(tag) } | ||
| unless missing.empty? | ||
| @missing_remote_packages << missing | ||
| @missing_remote_kind << "ssh" |
There was a problem hiding this comment.
As this is also displayed in the UI, using the upper case variant is better: SSH, VNC,...
Please review the following changes: