Do not raise an internal error in installation when updating /etc/hosts #512
Conversation
This issue was a regression. The Host#Updated used to accept an array of static IPs in the past. This feature / API was dropped during refactoring. However, ResolveHostnameToStaticIPs still used the old API.
src/modules/Host.rb
Outdated
|
|
||
| fqhostname = Hostname.MergeFQ(DNS.hostname, DNS.domain) | ||
|
|
||
| static_ips.reject(&:empty?).each { |sip| Update(fqhostname, fqhostname, sip) } |
There was a problem hiding this comment.
StaticIPs is defined in this file and this method is the only user. My conclusion is to instead fix that method not to return invalid data in the first place.
There was a problem hiding this comment.
OK, and now we no longer need :empty here!
Regarding this I found a comment in the original refactoring PR (#454 (comment))
It turns out the manual tests were not enough. Apparently we did have coveralls set up back then but it did not report in the PR. Even in this PR it does not report. I need to check this. And, static type checking would have caught this too. Cc @jreidinger |
|
I will check coveralls |
|
@mvidner in fact it is configured and reported also here. What exactly you missing? |
|
Duh, the coverage report was there and it is still there! I was looking in the wrong place (the check+cross marks) but it is a regular comment. Sorry for the noise. |
yes, I obviously did manual tests wrong or incomplete :-/ |
src/modules/Host.rb
Outdated
|
|
||
| fqhostname = Hostname.MergeFQ(DNS.hostname, DNS.domain) | ||
|
|
||
| static_ips.reject(&:empty?).each { |sip| Update(fqhostname, fqhostname, sip) } |
There was a problem hiding this comment.
OK, and now we no longer need :empty here!
|
empty ips was not the main / only problem. The problem (also) was that module tried to write multiple IPs for one entry - that caused the issue in the lens. you can reproduce the issue even this way - boot sp3 installation with "ifcfg=eth0=1.1.1.1 ifcfg=eth1=2.2.2.2 hostname=sles.suse.de" - and you'll get the issue too. |
|
Thanks for the reviews |
bsc#1038521