Simplified code a bit, improved documentation, tests and added some acceptance tests.

@mateusz-gozdek-sociomantic Your changes are currently showing syntax errors. To view them locally you just have to run the command:

bundle exec rubocop

If you add a -a to the end of the command it will even autocorrect some of them.

@david22swan are you sure you're checking latest commit? I was running bundle exec rake syntax lint metadata_lint check:symlinks check:git_ignore check:dot_underscore check:test_file rubocop (the same as CI is running) and everything was fine...

@mateusz-gozdek-sociomantic I honestly have no idea, it's disappeared now. Must have been an old commit. Sorry
Also there's gonna be a wait on your pr getting merged, the pipelines for testing are down right now.

Any updates on this? I was wondering, maybe I should also add it to changelog, that it will be reported as array now. And I'm wondering if that's a breaking change or not...

@mateusz-gozdek-sociomantic Unfortunately the pipelines are still down right know.
As to your comments, a docs change would be good, to let people know of the expanded functionality. And regarding it being a breaking change, is anything changed about the output if you were to use your updated code in the same manner as before you had altered it, if that it understandable.
i.e. Can you still use the code in the same way as before without any alterations? Or do you need to update your manifest's for it?

The code and manifests should be fine, since we cast string to array now. The only thing which comes to my mind is, that if someone use output of puppet resource firewall, that changes, since it's reporting array now and not string. I'm not sure how changelogs are generated, but perhaps it could be mentioned there.

I've noticed, that I should also update README.markdown, so I'll do it now.

Ping @david22swan :)

@mateusz-gozdek-sociomantic Apologies on the wait, was moved onto some new work and lost track of everything, I am running your changes through the pipeline right now, and assuming all goes well, I should get your work merged in either today or tomorrow.
Again, sorry for the wait.

@mateusz-gozdek-sociomantic I'm sorry, but unfortunatly the following test's have failed against both redhat 5 and centos 5 when run on puppet 6:

07:58:38 rspec './spec/acceptance/firewall_spec.rb[1:10:5:2]' # firewall basics dst_type when LOCAL --limit-iface-in fail does not contains the rule
07:58:38 rspec './spec/acceptance/firewall_spec.rb[1:10:7:1]' # firewall basics dst_type when mutliple addrtype applies
07:58:38 rspec './spec/acceptance/firewall_spec.rb[1:10:7:2]' # firewall basics dst_type when mutliple addrtype contains the rule
07:58:38 rspec './spec/acceptance/firewall_spec.rb[1:11:5:2]' # firewall basics src_type when LOCAL --limit-iface-in fail does not contains the rule
07:58:38 rspec './spec/acceptance/firewall_spec.rb[1:11:7:1]' # firewall basics src_type when mutliple addrtype applies
07:58:38 rspec './spec/acceptance/firewall_spec.rb[1:11:7:2]' # firewall basics src_type when mutliple addrtype contains the rule

The error message looks the same as on your last pr, so hopefully it can be dealt with in the same way.

I'll do some tests and patch it tomorrow.

Okay, so:

• Fixed 's/mutliple/multiple/g' 🤦‍♂️
• Checked on CentOS 5, it seems that in iptables 1.3.7, you can't specify src_type and dst_type multiple times, so added a check for it, which throws exception and adapted spec.
• Changed expection message from "not supported in your version" to "supported from version", as I believe it's more helpful for end users. Other messages could be adopted to that too, but I didn't touch it.

@mateusz-gozdek-sociomantic Unfortunately that only fixed two of the failures, four of them still remain:

06:39:15 rspec './spec/acceptance/firewall_spec.rb[1:10:6:1]' # firewall basics dst_type when duplicated LOCAL fails
06:39:15 rspec './spec/acceptance/firewall_spec.rb[1:10:8:1]' # firewall basics dst_type when multiple addrtype fail fails
06:39:15 rspec './spec/acceptance/firewall_spec.rb[1:11:6:1]' # firewall basics src_type when duplicated LOCAL fails
06:39:15 rspec './spec/acceptance/firewall_spec.rb[1:11:8:1]' # firewall basics src_type when multiple addrtype fail fails

I've created a gist containing the failure output so you can get a better look:
https://gist.github.com/david22swan/106262f240723700ffa7aaf90b0ff865

• Fixed regexes for firewall basics dst_type when multiple addrtype fail fails, There was #, which shouldn't be there I believe
• firewall basics src_type when duplicated LOCAL fails disabled this test on CentOS 5, as we throw multiple addrtype first, not sure if that's the best choice

Just out of curiosity, is there a way to run this pipeline locally? I tried with vagrant, but it's difficult to set up. It would definitely speed up the process :)

@mateusz-gozdek-sociomantic Still failing the following test's on both sorry:

02:01:04 rspec './spec/acceptance/firewall_spec.rb[1:10:8:1]' # firewall basics dst_type when multiple addrtype fail fails
02:01:04 rspec './spec/acceptance/firewall_spec.rb[1:11:8:1]' # firewall basics src_type when multiple addrtype fail fails

And unfortunately you an't access are pipelines, the vmpooler images that we use are private to the company and can't be accessed by anyone outside of it.

Ugh, match reges was using test variable, not type`, I have no idea where this came from. Anyway, I fixed it. Let's hope it works this time 🤞

🤞

@mateusz-gozdek-sociomantic Adoc went green :).
Happy to merge!
Thanks for your work.

Awesome, thanks for your patient!