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

Remove sysctl cookbook dependency and use new native sysctl resource #228

Merged
merged 2 commits into from
Mar 12, 2019
Merged

Remove sysctl cookbook dependency and use new native sysctl resource #228

merged 2 commits into from
Mar 12, 2019

Conversation

josqu4red
Copy link
Contributor

Sysctl cookbook is deprecated and resource has been integrated to Chef as of 14.0.

@coveralls
Copy link

coveralls commented Oct 11, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 5220c3b on josqu4red:feature/remove-sysctl-cookbook into a3f1deb on dev-sec:master.

@artem-sidorenko
Copy link
Member

@josqu4red thanks for this PR!

We had already a discussion about this topic and decided to keep sysctl as we wanted to keep Chef 13 support for a while.

If I take a look to the support RFC, it looks like we can drop the Chef 13 support as 6 months are more or less over. @chris-rock what do you think?

metadata.rb Outdated
@@ -22,9 +22,9 @@
license 'Apache-2.0'
description 'Installs and configures operating system hardening'
long_description IO.read(File.join(File.dirname(__FILE__), 'README.md'))
version '3.1.0'
version '4.0.0'
Copy link
Member

Choose a reason for hiding this comment

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

can you please remove the version bump, it will be done by us during the release. Thanks!

@chris-rock
Copy link
Member

I like the simplicity, on the other hand a lot of users still use Chef 13. Making it part of the next major release sounds right to me. Therefore Chef 13 user could use the older version of the cookbook.

@artem-sidorenko
Copy link
Member

@chris-rock then lets prepare this and keep it open for a while.
I would like to go over other issues/PRs, then we can do a minor release with chef 13 support and include other PRs/fixes and after it do a major release without chef 13 support. Ok?

@josqu4red
Copy link
Contributor Author

Hi maintainers, thanks for the feedback - and the fine cookbook. Version bump removed as requested.

@bdwyertech
Copy link

bdwyertech commented Nov 28, 2018

On the native sysctl resource, it should have ignore_error set to true.

On a system with lets say IPv6 disabled, the IPv6 sysctls will trigger a chef run failure where they previously didn't.

chef/resource/sysctl.rb

@josqu4red
Copy link
Contributor Author

@bdwyertech ignore_error attribute is set to false (default) in the current implementation, so I don't believe behavior will change here.

@bdwyertech
Copy link

bdwyertech commented Dec 5, 2018 via email

@artem-sidorenko
Copy link
Member

I would like to merge this in the next days and create a new major release,

@josqu4red can you please rebase it on the latest master and add a sign-off to make the DCO checker happy?

@josqu4red
Copy link
Contributor Author

@artem-sidorenko here you go

@artem-sidorenko
Copy link
Member

@josqu4red many thanks! There are some changed needed in the unit/spec tests, would you like to fix them? If not, I would like to do that in this PR and place my commit on top of yours, what do you think?

@josqu4red
Copy link
Contributor Author

@artem-sidorenko sorry, I didn't check the tests as my change is so small. Feel free to add commits if you want to move on quickly with this.

Copy link
Member

@artem-sidorenko artem-sidorenko left a comment

Choose a reason for hiding this comment

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

@josqu4red many thanks!

@artem-sidorenko artem-sidorenko merged commit e993020 into dev-sec:master Mar 12, 2019
@bdwyertech
Copy link

bdwyertech commented Mar 12, 2019

This should have ignore_error set to true to accommodate things that may not exist (e.g. IPv6) I see theres an attrib to flag ipv6 support not but I believe theres another one or two that sometimes do not exist.... Will try again with latest and see if that still holds true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants