-
Notifications
You must be signed in to change notification settings - Fork 456
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
UNIT_TESTS_FAIL: Fedora 18 support #176
Conversation
Can one of the admins verify this patch? |
@ecbypi the travis tests are failing on this patch: https://travis-ci.org/puppetlabs/puppetlabs-firewall/jobs/6772125 |
ok to test |
Merged build triggered. (Status: PENDING, Details: null) |
Merged build started. (Status: PENDING, Details: http://box.bob.sh:8080/job/puppetlabs-firewall-system/158/) |
Merged build finished. (Status: SUCCESS, Details: http://box.bob.sh:8080/job/puppetlabs-firewall-system/158/) |
Can one of the admins verify this patch? |
@ecbypi btw - on the system testing front, although Jenkins reported 'Success' it was never really tested on Fedora 18, as I have it in a special exclusion list until it worked. I did however run up a custom job here: http://box.bob.sh:8080/job/puppetlabs-firewall-system-fedora-18/1/RSPEC_SET=fedora-18-x64/consoleFull which shows how your code ran against Fedora 18. As you can see beyond the basic class we still have some parser bugs to solve for it to work 'out of the box', especially with existing rules. Having said that, I think the class patch you have here looks pretty good ... this is a great start to getting Fedora 18 support. |
@kbarber Ok. I'll look over this tonight. Is it possible to recreate the test run you linked to for fedora 18 on my local machine? |
@ecbypi system tests can be replicated by:
Grabbing the gems using 'bundle update' Then running:
Fedora specifically can be tested with:
The test framework is fairly new so let me know if you have issues. I've tested this on both Linux (Debian 7 in this case) and OS X 10.8.x. |
Sweet. The |
Any progress on this @ecbypi ? |
Unfortunately no. It's been a busy month and it keeps slipping my mind. I'll actually take a look at things tonight. Sorry for the delay. |
Can one of the admins verify this patch? |
@kbarber I'm still in the process of debugging failures. I noticed that on in What are your thoughts? |
Some tests have changed since you applied this patch, you might want to rebase. The current tests always test to make sure the exit_code first up is not 1 ... then the second tests for non zero. This is a pretty normal idempotence test. Unless of course, the current code you propose requires more than 1 puppet run to do everything? |
I think this is now fixed upstream. I'm going to close this for now but feel fre to reopen and rebase if this is still a problem. |
Based on the discussion in #145, this enables iptables on Fedora 18 and disabled firewalld. Determining the persistence command has been changed to compare regexes against the
operatingsystem
andoperatingsystemrelease
instead ofosfamily
since the command is no longer solely dependent onosfamily
. Special cases (Fedora 18, Fedora 15-17), go at the top of thecase
statement. These changes are structured in a way that these changes can easily be applied to RHEL 7 if desired.