Services are not touched if firewalld is not enabled (bsc#1067915) #38
Conversation
teclator
commented
Nov 15, 2017
teclator
commented
- Fixed https://bugzilla.suse.com/show_bug.cgi?id=1067915
- Also fixed a bad method call in SuSEFirewallCMDLine
- Improved test coverage.
.rubocop.yml
Outdated
|
|
||
| # Redundant returns add legibility for developers used to other languages | ||
| Style/RedundantReturn: | ||
| Enabled: false |
There was a problem hiding this comment.
btw reason sounds a bit funny. Does we add to C++ also style that makes C++ more familiar for ruby developers? :)
There was a problem hiding this comment.
Just copied the rubocop config from what I considered the newest module and only added the exclude of old yast-firewall modules because of all the complains so. So will check the other options
.rubocop.yml
Outdated
| Style/IfUnlessModifier: | ||
| MaxLineLength: 60 | ||
|
|
||
| # some storage API have size method, but without empty? method |
There was a problem hiding this comment.
Hehe, nothing like use the current config of some new module and not modify comments 👎
.rubocop.yml
Outdated
| Style/ZeroLengthPredicate: | ||
| Enabled: false | ||
|
|
||
| # the ".freeze" attribute for the constants is not nice |
There was a problem hiding this comment.
hmm, we have this discussion also in storage and in fact, freeze is what make constant real constant
| Metrics/CyclomaticComplexity: | ||
| Max: 10 | ||
|
|
||
| AllCops: |
There was a problem hiding this comment.
this is a bit strong, but ok for me.
There was a problem hiding this comment.
Yep, it is, I will remove it gradually, by now I only wanted to cover the new code.
| @@ -45,15 +52,20 @@ def modes | |||
|
|
|||
| def write | |||
| Service.Enable("sshd") if @settings.enable_sshd | |||
| @firewalld.enable! if @settings.enable_firewall | |||
|
|
|||
| return true unless @settings.enable_firewall | |||
There was a problem hiding this comment.
so if firewall is not enabled, we did not disable it?
There was a problem hiding this comment.
Well, this client is the only one that enables the service, make sense to disable it during installation?
There was a problem hiding this comment.
well, it depends if user click on disable firewall and product have it enabled by default, I expect it will be disabled.
test/clients/firewall_test.rb
Outdated
| end | ||
| end | ||
|
|
||
| context "and the firewall-config package is installed" do |
There was a problem hiding this comment.
missing case when it is not installed.
There was a problem hiding this comment.
in fact it is two cases as user is asked to install it and he can install or cancel.
test/clients/proposal_test.rb
Outdated
| let(:client) { described_class.new } | ||
|
|
||
| describe "#initialize" do | ||
| it "instantiates a new proposal settings" do |
There was a problem hiding this comment.
well, I think it is not needed to test that it is instantiated, as it is common ruby functionality.
There was a problem hiding this comment.
well, it is part of the code and it is public, it is a simple test but even so..
There was a problem hiding this comment.
BTW, this is really a test for lib/y2firewall/clients/proposal.rb so will move all the lib tests to a folder that really match its path
|
|
||
| # [Boolean] Whether the firewalld service will be enable | ||
| attr_accessor :enable_firewall |
There was a problem hiding this comment.
as you are having more rich enable/disable variants, does it make sense to still have public enable_firewall=?
There was a problem hiding this comment.
Probably not make sense all, is preferable to not expose it, but currently it is used in load_feature although I could just modify the public_send by an instance_variable_set in bang methods and also in some tests.
There was a problem hiding this comment.
Will try to replace in next iteration over code.
