Checking semantic while importing AY settings #61
Conversation
src/lib/y2firewall/clients/auto.rb
Outdated
| @@ -88,6 +89,18 @@ def import(profile) | |||
| enable if profile.fetch("enable_firewall", settings.enable_firewall) | |||
| start if profile.fetch("start_firewall", false) | |||
| importer.import(profile) | |||
|
|
|||
| # Checking if an interface has been defined for different zones | |||
There was a problem hiding this comment.
Nitpicking: we expect this to be extended in the future, a separate function, e.g., check_profile_for_errors might be better
schubi2
changed the title
package/yast2-firewall.changes
Outdated
| ------------------------------------------------------------------- | ||
| Fri Apr 27 16:35:19 CEST 2018 - schubi@suse.de | ||
|
|
||
| - AutoYaST: Reporting warning if an interface has been defined |
There was a problem hiding this comment.
I would say just warn or report. Something like Report whether an interface....
src/lib/y2firewall/clients/auto.rb
Outdated
| def check_profile_for_errors | ||
| # Checking if an interface has been defined for different zones | ||
| zones = firewalld.export["zones"] || [] | ||
| all_interfaces = zones.collect { |zone| zone["interfaces"] || [] } |
There was a problem hiding this comment.
We usually prefer #map to #collect (although they are just aliases). And, in this case, you might want to use flat_map (removing the #flatten! call):
all_interfaces = zones.flat_map { |z| z["interfaces"] || [] }Take the #flat_map comment just as a suggestion, please, and feel free to ignore it :)
| end | ||
| end | ||
|
|
||
| let(:i_list) { IssueList.new } |
There was a problem hiding this comment.
You can use a double here (or a instance_double):
let(:i_list) { double("IssuesList") }See https://relishapp.com/rspec/rspec-mocks/v/3-7/docs/basics/test-doubles or https://relishapp.com/rspec/rspec-mocks/v/3-7/docs/verifying-doubles.
| expect(firewalld).to receive(:export) | ||
| .and_return("zones" => [{ "interfaces" => ["eth0"], "name" => "public" }, | ||
| { "interfaces" => ["eth0", "eth0"], "name" => "trusted" }]) | ||
| expect(Yast::AutoInstall).to receive(:issues_list).and_return(i_list) |
There was a problem hiding this comment.
np: I would move this line to a before block because it is somehow a precondition.
before do
allow(Yast::AutoInstall).to receive(:issues_list).and_return(i_list) # We use `allow` instead of `expect`.
end| .and_return("zones" => [{ "interfaces" => ["eth0"], "name" => "public" }, | ||
| { "interfaces" => ["eth0", "eth0"], "name" => "trusted" }]) | ||
| expect(Yast::AutoInstall).to receive(:issues_list).and_return(i_list) | ||
| expect_any_instance_of(IssueList).to receive(:add) |
There was a problem hiding this comment.
Given that, in the previous line, you set Yast::AutoInstall#issues_list to return i_list, this should work:
expect(i_list).to receive(:add)
.with(...)
* checking semantic while importing AY settings
No description provided.