-
Notifications
You must be signed in to change notification settings - Fork 16
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
rewrite chef checks to use Ridley instead of deprecated chef rest api. #26
Conversation
bin/check-chef-node.rb
Outdated
@@ -65,12 +70,16 @@ def run | |||
critical "Node #{config[:node_name]} not found - #{e.message}" | |||
end | |||
|
|||
private | |||
private |
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.
was there a reason for this whitespace?
@annshah thanks for the contribution overall this looks good, if you can kill that extra whitespace and fix the gem dependency issues after some quick verification tests I would love to get this merged. |
bin/check-chef-node.rb
Outdated
@@ -50,13 +50,18 @@ class ChefNodeChecker < Sensu::Plugin::Check::CLI | |||
short: '-K CLIENT-KEY', | |||
long: '--keys CLIENT-KEY' | |||
|
|||
option :ignore_ssl_warnings, |
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.
I think this would be more appropriately named ignore_ssl_verification
bin/check-chef-nodes.rb
Outdated
@@ -68,19 +68,24 @@ class ChefNodesStatusChecker < Sensu::Plugin::Check::CLI | |||
long: '--exclude-nodes EXCLUDE-NODES', | |||
default: '^$' | |||
|
|||
option :ignore_ssl_warnings, |
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.
I think this would be more appropriately named ignore_ssl_verification
sensu-plugins-chef.gemspec
Outdated
@@ -40,7 +40,7 @@ Gem::Specification.new do |s| | |||
s.add_runtime_dependency 'rack', '~> 1.6.5' | |||
end | |||
|
|||
s.add_runtime_dependency 'ridley', '4.1.2' | |||
s.add_runtime_dependency 'ridley', '5.1.0' | |||
s.add_runtime_dependency 'sensu-plugin', '~> 1.2' | |||
s.add_runtime_dependency 'varia_model', '0.4.1' |
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.
looks like this needs to be bumped per travis: https://travis-ci.org/sensu-plugins/sensu-plugins-chef/jobs/230068843
…space, changed varia_model to 0.6
I see the issue, basically ridley 5.x requires buff-extension 2.x which requires >= ruby 2.2. @eheydrick did sensu ever ship with ruby 2.1? In this case I think we should consider dropping support and do a major bump as this solves issues for lots of users of this gem and with a rewrite I would hate to force using an old version of ridley that we are gonna have to update agains shortly anyways. Thoughts? |
I was facing compatibility issues with earlier versions of ridley. So I believe using latest version might be a necessity. |
Ya I looked at it and im favor of this breaking change but would like to hear from another maintainer before merging and releasing this one. |
@cwjohnston thoughts? |
@majormoses per sensu-build commit history I don't think we've shipped a Sensu release with Ruby 2.1. I find these changes agreeable for the most part. I also like the idea of implementing a module library like you mentioned in #25 (comment). Would that be reasonable here to dry things up and make it easier to move to https://github.com/sethvargo/chef-api when that becomes a stable option? |
@cwjohnston awesome thats what I was looking for! While I would like to force the connection information out right now to me the priority is to get this fixed and nice cleanup later. We should keep the original issue open to track that but I really want this merged asap. |
@majormoses @annshah let's remove ruby 2.1 and add ruby 2.4 to the travis matrix, if those tests pass we can shipit edit: with a major version bump 👍 |
@annshah I see you did not give maintainers access to your fork can you please add this:
Also add 2.4 as requested by @cwjohnston |
- removed ruby 2.1 support - documented breaking change, impact, and upgrade path that this breaks their setup. - added ruby 2.4 testing
@majormoses : I have added all maintainers and Ruby 2.4.1. |
@annshah thanks I will squash and merge this |
Pull Request Checklist
Is this in reference to an existing issue?
rewrite plugin checks to use ridley issue #25
General
Update Changelog following the conventions laid out on Keep A Changelog
Update README with any necessary configuration snippets
Binstubs are created if needed
RuboCop passes
Existing tests pass
Purpose
Known Compatablity Issues