Skip to content
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

Jump to bottom

Remove any lsb facts usage #135

Merged
merged 11 commits into from Apr 21, 2020
Merged

Remove any lsb facts usage #135

merged 11 commits into from Apr 21, 2020

Conversation

neomilium
Copy link
Contributor

@neomilium neomilium commented Apr 6, 2020 •
edited

This PR removes any lsb-based facts usage as discussed in #122 and #132.

@neomilium neomilium force-pushed the rework-templates branch 2 times, most recently from e77a3f4 to eadda5c Compare April 6, 2020 20:00
@neomilium neomilium mentioned this pull request Apr 6, 2020
Closed
@neomilium neomilium changed the title WIP: Rework templates WIP: Remove any lsb facts usage Apr 6, 2020
@neomilium
Copy link
Contributor Author

neomilium commented Apr 6, 2020 •
edited

@dhoppe @smortex Could you test this branch on some of your nodes (ie. CentOS, RedHat), and made a review?

@neomilium neomilium mentioned this pull request Apr 6, 2020
Closed
@vox-pupuli-tasks
Copy link

Dear @neomilium, 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

3 similar comments
@vox-pupuli-tasks
Copy link

Dear @neomilium, 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

@vox-pupuli-tasks
Copy link

Dear @neomilium, 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

@vox-pupuli-tasks
Copy link

Dear @neomilium, 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

smortex
smortex reviewed Apr 9, 2020
View reviewed changes
Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have CentOS nodes configuring fail2ban at hand, but maybe the beaker coverage is a good indication of the consequences of the change. It happens that these failures are caused by a resource and could be easily fixed.

manifests/init.pp Outdated
@@ -40,7 +44,9 @@
Array $whitelist = ['127.0.0.1/8', '192.168.56.0/24'],
Hash[String, Hash] $custom_jails = {},
String[1] $banaction = 'iptables-multiport',
) inherits ::fail2ban::params {
) {
notify { "fail2ban will use ${config_file_template} as template": }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This trigger a change, and with --detailed-exitcodes puppet will exit with 2 and not 0.

This is used in the test suite and seems to be the cause of the failure on the Beaker tests with CentOS I checked.

Instead of a resource, you can use the notice() function (always shown as far as I can recall), or maybe better info() (only shown when running with --verbose) or debug() (shown when running with --debug).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but the commit will be removed from final work. I put it here only to help user to test the branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not find a similar "notification" in the code in the master branch so I just dropped that line in 839b602. It indeed fix the beaker integration tests 😉

@smortex
Copy link
Member

smortex commented Apr 9, 2020 •
edited

Just realized I could commit to the repo to update the PR… @neomilium feel free to rework / integrate properly (fixup) my commits. I wanted to reach quickly an all-passing-test-suite 🕺

@neomilium
Copy link
Contributor Author

@smortex Done, thanks.

Any idea of the goal of the test where I put this comment: 646cfde

@bastelfreak bastelfreak added backwards-incompatible needs-work not ready to merge just yet labels Apr 10, 2020
@neomilium neomilium mentioned this pull request Apr 11, 2020
Closed
@neomilium
Copy link
Contributor Author

@smortex Thanks! Could you remove the lsb fact usage in the test you fixed? So this PR will be ready to be merged.

dhoppe
dhoppe requested changes Apr 20, 2020
View reviewed changes
spec/acceptance/class_spec.rb Outdated Show resolved Hide resolved
@dhoppe dhoppe changed the title WIP: Remove any lsb facts usage Remove any lsb facts usage Apr 21, 2020
@dhoppe dhoppe merged commit 4b75cf5 into voxpupuli:master Apr 21, 2020
This was referenced Apr 21, 2020
Closed
Closed
@neomilium neomilium deleted the rework-templates branch April 21, 2020 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smortex smortex smortex left review comments

@bastelfreak bastelfreak bastelfreak left review comments

@dhoppe dhoppe dhoppe approved these changes

Assignees
No one assigned
Labels
backwards-incompatible needs-work not ready to merge just yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@neomilium @smortex @bastelfreak @dhoppe