-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adjust firewall_enabled test for SLE15 #3605
Conversation
Related progress issue: https://progress.opensuse.org/issues/25442
@yxususe before I saw you picked up the ticket this is what I prepared, would you like to review and test? |
LGTM. But I'm afraid I cannot test it today, because I'm testing something else and can't always switch branches. However I see the problem that SuSEfilewall2 is extensively used in our code, which means we need to do quite some work to adapt the tests to SLE15. |
assert_script_run("SuSEfirewall2 status"); | ||
if (is_jeos) { | ||
if ((is_sle && sle_version_at_least('15')) || (is_leap && is_leap_version_at_least('15'))) { | ||
assert_script_run('firewallctl state'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to test firewallctl on Tumbleweed also
Yes, I checked TW. It still has the SuSEfirewall2 command installed by default and firewalld is available for install bit not fully usable.
The testing goal of the test module should be "ensure the firewall runs by default using the standard tool"
How about another explicit test module?
|
ok, I'm going with @yxususe's LGTM here and merge this fix for SLE15. I am pretty sure that there are many missing parts regarding firewallctl which need to be implemented anyway. I recommend therefore in this case to go forward with the changes for SLE15 as we need this fix anyway and then afterwards see about TW which is still running fine so a change is not necessary there right now. |
This breaks openSUSE Leap maintenance tests:
|
Sorry about that. Fixed in 7f87096, I will retrigger the tests |
Related progress issue: https://progress.opensuse.org/issues/25442