-
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
adding cert option to some checks and metrics #101
Conversation
Let me know if you need help working through the rubocop errors. |
Sure, some help would be great. I tried running the autofix flag, but it just created some more issues. I'm not very familiar with Ruby and don't know all the best practices |
I went through the cops one by one, in some cases they were rather opinionated other places we should certainly refactor at a later date. |
Thanks for your contribution to Sensu plugins! Without people like you submitting PRs we couldn't run the project. I will review it shortly. |
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.
Overall this looks great, there are a few things that can be improved as well as some missing bits.
Missing bits:
- changelog entry explaining the new feature
- a testing artifact
As per my comment here we should probably add some documentation about the dangers of providing secrets directly to the CLI.
@@ -36,4 +36,4 @@ task :check_binstubs do | |||
end | |||
end | |||
|
|||
task default: [:spec, :make_bin_executable, :yard, :rubocop, :check_binstubs] | |||
task default: %i(spec make_bin_executable yard rubocop check_binstubs) |
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.
pretty sure this changes in a newer version of rubocop, but that change is I believe breaking so we can change that back when we update it.
# rubocop:disable LineLength | ||
used_in_bytes = nodes_being_used.map { |node| node['fs']['data'].map { |data| data['total_in_bytes'] - data['available_in_bytes'] }.flatten }.flatten.inject { |sum, x| sum + x } | ||
# rubocop:enable LineLength | ||
# TODO: come back and cleanup all these rubocop disables with a little refactor |
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 know this is not the code that was changed but somehow rubocop started complaining. This is super ugly and hard to read, we should come back and refactor this later.
@csoleimani any chance on coming back to get this cleaned up and over the line? |
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.
As requested by @majormoses I have just tested the --cert
option against an Elasticsearch cluster that has HTTPS enabled and it works as expected.
[riley:...sensu-plugins-elasticsearch]$ bundle exec bin/check-es-cluster-status.rb --host elastic.example.com --port 9200 --https true --user 'admin' --password 'password' --cert ~/Downloads/Root.crt
ESClusterStatus OK: Cluster is green
@thomasriley thanks for testing I will review this tonight or tomorrow and hopefully merge and release this. |
Looks like we need a rebase. |
I rebased for you, sorry for the long delay. |
To avoid confusion I am gonna rename the option from |
Hmm one more thought is that |
@majormoses I agree on both using the |
…itical thresholds in other checks in this repo as well as that being a standard across plugins.
Why was this not added to all checks? |
@jwsomis cant say but you are welcome to open up a PR for any missing ones. |
I think I had some problems with the ones I didn't add it to, but it was so long ago that I don't remember what they are. Feel free to give it a shot, though! |
I started trying to, but am rather new to sensu plugins and ran into issues
trying to figure out what the correct build steps are for bonsai assets.
Could you enlighten me?
Wes
…On Wed, Dec 4, 2019, 6:08 PM Cameron Soleimani ***@***.***> wrote:
I think I had some problems with the ones I didn't add it to, but it was
so long ago that I don't remember what they are. Feel free to give it a
shot, though!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#101?email_source=notifications&email_token=AA2P6TFOOD66IMFHTXQLC2LQXBPABA5CNFSM4EF2PHDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF7GZMI#issuecomment-561933489>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA2P6TDH6AYIQPOHDOVO3ELQXBPABANCNFSM4EF2PHDA>
.
|
This is the best guide I have seen around sharing assets: https://discourse.sensu.io/t/contributing-assets-for-existing-ruby-sensu-plugins/1165 |
Thanks, Cameron! I was missing the TravisCI portion.
…On Wed, Dec 4, 2019 at 8:53 PM Cameron Soleimani ***@***.***> wrote:
This is the best guide I have seen around sharing assets:
https://discourse.sensu.io/t/contributing-assets-for-existing-ruby-sensu-plugins/1165
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#101?email_source=notifications&email_token=AA2P6TD6ZV2E4AB7B5D4SH3QXCCLBA5CNFSM4EF2PHDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF7PH7Y#issuecomment-561968127>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA2P6TBDBCCCTTY6IHGCYPTQXCCLBANCNFSM4EF2PHDA>
.
|
Pull Request Checklist
Is this in reference to an existing issue?
kind of. #100
it's better to pass in a cert than not check for validation
General
Update Changelog following the conventions laid out here
Update README with any necessary configuration snippets
Binstubs are created if needed
RuboCop passes
Existing tests pass
New Plugins
Tests
Add the plugin to the README
Does it have a complete header as outlined here
Purpose
Known Compatibility Issues