-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add support for EL8 #247
Add support for EL8 #247
Conversation
27bc7d1
to
2b58d88
Compare
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.
Nice improvement to the acceptance test and README update on how to run them.
Not too sure about linking to the REFERENCE.MD (although I'm neutral/OK with having it generated) until it's at least a little bit better.
.sync.yml does need tweaking though.
2b58d88
to
7b3371a
Compare
Tests are failing due to ruby style. Suggest fixing those or adding the rubocop rules to be ignored and then this can be merged. |
7b3371a
to
cf06b95
Compare
@@ -531,8 +531,28 @@ firewalld::direct_passthroughs: | |||
* `args`: Name of the passthroughhrough to add (e.g: -A OUTPUT -j OUTPUT_filter) | |||
|
|||
|
|||
## Testing |
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.
We've got our contributing guidelines which explain how to execute tests: https://github.com/voxpupuli/puppet-firewalld/blob/master/.github/CONTRIBUTING.md
That file is managed by modulesync: https://github.com/voxpupuli/modulesync_config/blob/master/moduleroot/.github/CONTRIBUTING.md.erb
Could you link to that file / update it instead of duplicating the information?
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.
@bastelfreak The point here is that these tests have to be run in vagrant and not docker like we might otherwise.
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.
Can't test underlying firewalls properly in Docker-land
@@ -1,6 +1,19 @@ | |||
HOSTS: |
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.
We usually purge this file within modulesync and create nodesets on demand with https://github.com/puppetlabs/beaker-hostgenerator
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.
@bastelfreak The tests are going to quickly get more complicated than this will handle so I'd rather not right now.
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.
To clarify, these will be multi-node in the near future. Via work on simp-iptables
, we have found that this is the only way that you can properly uncover bugs in the underlying system.
2a36225
to
b12c808
Compare
Dear @trevor-vaughan, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
* Added OEL to the metadata.json * Added a very basic acceptance test to test the main components of the module. This should be expanded into multi-node tests later. * Added a REFERENCE.md * Updated the README.md to point to the REFERENCE.md and added instructions on how to run the acceptance test. Closes voxpupuli#246
b12c808
to
7d63181
Compare
Pull Request (PR) description
module. This should be expanded into multi-node tests later.
instructions on how to run the acceptance test.
This Pull Request (PR) fixes the following issues
Fixes #246