[Review] Request from 'schubi2' @ 'yast/yast-network/review_141001_autoyast_do_not_overwrite_firewall_settings'

[schubi2]

Please review the following changes:

• efc4dc8 typo
• 0f78165 added testcase
• 1b545dd Revert "adapt Rakefile to submit to correct build service project in maintenance branch SLE-12-GA"
• 0e4e651 adapt Rakefile to submit to correct build service project in maintenance branch SLE-12-GA
• a56e86a removed not needed import
• f8bc950 moved autoyast stuff into lan_auto.rb
• 03880f6 packaging
• c441ccf Autoyast: Do not overwrite in autoinst.xml file configured firewall settings by installed firewall settings if the installed network is kept.

[ancorgs]

You don't need to explicitly name optional parameters in ruby when calling the method. Lan.Read(:cache, false) would work without the side effect of assigning a variable called read_firewall_settings in lan_auto.

[ancorgs]

This seems to always skip firewall settings, either if there are conflicting settings in autoinst.xml or not. Couldn't that lead to default settings not being applied when autoinst.xml contain no information about firewall?

[schubi2]

1. Yes, I do not need. But I think it is more readable. Otherwise you do not know what "false" means without having a look to the Lan module. Of course there will be defined a new variable.
2. As far I know, the firewall settings are empty here every time. So we do not loose any default data. But it is a real good question which I have had be my own too :-)

[schubi2]

1. A compromise: I have added a comment :-)

[jreidinger]

to be honest I do not like this way, as if we in future need to call read which skip different part of read, then we add antoher boolean and result is like in bootloader Bootloader.Read(false, true, false) which is horrible.
Better use map

def Read(cache, options = {})
...
SuSEFirewall4Network.Read if options[:avoid_reading_firewall]

and usage

Lan.read(:cache, :avoid_reading_firewall => true)

of course it is also not nice to have option for cache separated, it will be nice to have it also in options map, but it is more intrusive change.

[ancorgs]

My main reason for not suggesting that road was compatibility with Perl and publish. But if we go that way, remember that in Ruby 2 we have keyword arguments, which is even nicer than options = {}.

[jreidinger]

yes, nicer one

[schubi2]

So Ancorgs, could you please show us how it should look like ? :-)

[ancorgs]

if we want to keep the first :cache parameter as it is. It would be

def Read(cache, read_firewall: true)
  # blah
  SuSEFirewall4Network.Read if read_firewall
  # more blah
end

and calling would be Lan.Read(:cache, read_firewall: false)

[mvidner]

Please run

yardoc
xdg-open doc/autodocs/Yast/LanClass.html

to verify that you get the yard syntax right.

[schubi2]

Yes, it has been ugly. "yardoc" is magic !! :-)

[mvidner]

The Bugzilla entry is targeted for GA maintenance update, so the correct branch is SLE-12-GA. The version should be 3.1.103.x (where x depends on a race with #261)

[mchf]

The Bugzilla entry is targeted for GA maintenance update, so the correct branch is SLE-12-GA . The version should be 3.1.103.x (where x depends on a race with #261)

This comment seems to be obsolete

[jreidinger]

@mvidner @mchf what is state after new commits?

[mvidner]

master is no longer appropriate for a maintenance update bug fix. Retargeted thsi for SLE-12-GA in #275.