sanitize hostname handling (fate #319639) #101
Conversation
Note that there are also linuxrc changes that go along with this.
install system has been fixed with these 2 PRs: openSUSE/linuxrc#108 openSUSE/installation-images#101
| @@ -197,7 +190,7 @@ if grep -qi "^VNC:.*1" /etc/install.inf ; then | |||
| if test "$ec" = "0" ; then | |||
| ( | |||
| sleep 3 | |||
| /usr/bin/slptool register "service:YaST.installation.suse:vnc://${hostname}:5901" | |||
| /usr/bin/slptool register "service:YaST.installation.suse:vnc://${host_name}:5901" | |||
There was a problem hiding this comment.
hostip_from_wicked prints hostname including \n at the end. Isn't it a problem (e.g.) here?
There was a problem hiding this comment.
no, shell swallows white space
There was a problem hiding this comment.
ehm, ok. So why do you explicitly append it? I'm just curios, not blocker for me ;-)
There was a problem hiding this comment.
No idea; the code is not by me. If you say it's not necessary, I'm fine with another line.
| @@ -55,7 +55,7 @@ unset SSH_FAILED | |||
| stty sane 2>/dev/null | |||
|
|
|||
| # get hostname & hostips | |||
| hostip_from_wicked >/tmp/host_ips 2>/tmp/host_name | |||
| hostip_from_wicked /tmp/host_ips /tmp/host_name | |||
There was a problem hiding this comment.
ehm, is it possible to split / refactor this into two functions / helper scripts to have just hostip_from_wicked and e.g. hostname_from_wicked?
Moreover, I'd fix the behavior when host_name sometimes contains hostname and sometimes first ip.
There was a problem hiding this comment.
Why can't I extract two things from a single wicked run?
host_name is either a fqdn or the ip if no dns is available. I think it's named 'fqhn' sometimes. What's so bad about it?
There was a problem hiding this comment.
Why can't I extract two things from a single wicked run?
Common practice is to have one method to do just one thing. In the current state I'd expect that the function / script returns just one IP ... In reality it writes a list of ips into a file (with kind of header prepended) and hostname into another file.
host_name is either a fqdn or the ip if no dns is available. I think it's named 'fqhn' sometimes. What's so bad about it?
well, so far we has been talking about fqdn. So at least documentation should be fixed.
There was a problem hiding this comment.
@mvidner As I'm not strong in perl could you pls also review? Thanks
There was a problem hiding this comment.
Common practice is to have one method to do just one thing
I'd follow you for ruby methods or oo in general, but not shell scripts. If the name irks you, we can rename
the script to something else.
So at least documentation should be fixed.
It does exactly what is documented.
There was a problem hiding this comment.
Common practice is to have one method to do just one thing
I'd follow you for ruby methods or oo in general, but not shell scripts. If the name irks you, we > can rename
the script to something else.
then rename. Thanks
|
LGTM |
Note that there are also linuxrc changes that go along with this.