-
Notifications
You must be signed in to change notification settings - Fork 25
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
Added VertX health check && metrics #30
Conversation
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.
Thank you for also incorporating the CVE patches they are very much appreciated. I have a couple things to improve but I would not call blockers if you don't agree.
CHANGELOG.md
Outdated
- updated yard dependency to `~> 0.9.11` per: https://nvd.nist.gov/vuln/detail/CVE-2017-17042 (@yuri-zubov sponsored by Actility, https://www.actility.com) | ||
- updated rubocop dependency to `~> 0.51.0` per: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-8418. (@yuri-zubov sponsored by Actility, https://www.actility.com) | ||
|
||
### Breaking Changes |
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.
Generally I like to see the order of changes from most important to least important:
- breaking changes
- security
- fixed
- added
- removed
I will fix this up post acceptance prior to release if there are no other changes requested.
bin/check-vertx-health.rb
Outdated
description: 'VertX Endpoint', | ||
default: 'http://localhost:8080/rest/health' | ||
|
||
def request |
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.
not much in terms of rescuing but we can always add that later.
bin/metrics-vertx.rb
Outdated
long: '--scheme SCHEME', | ||
default: "#{Socket.gethostname}.vertx" | ||
|
||
def request |
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 we have the same logic can we break this out into a lib to avoid duplication?
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.
@majormoses move to common file.
fixed |
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.
@yuri-zubov looks good overall, just a a couple minor things to address.
bin/check-vertx-health.rb
Outdated
# Linux | ||
# | ||
# DEPENDENCIES: | ||
# gem: rest-clien |
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.
rest-client
s.add_runtime_dependency 'english', '0.6.3' | ||
s.add_runtime_dependency 'rest-client', '~> 2.0' |
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 missed this earlier but if we are now requiring rest-client
it requires a c compiler to install this plugin this constitutes a breaking change and need to be called out like this:
### Breaking Changes
- dropped ruby 2.1 support (@yuri-zubov sponsored by Actility, https://www.actility.com)
- added `rest-client` as a dependency which requires you to have a local c compiler present to install this plugin (@yuri-zubov sponsored by Actility, https://www.actility.com)
```
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.
done
Pull Request Checklist
Is this in reference to an existing issue?
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
New Plugins
Tests
Add the plugin to the README
Does it have a complete header as outlined here
Purpose
https://vertx.io/docs/vertx-health-check/java/#_http_responses_and_json_output
https://vertx.io/docs/vertx-dropwizard-metrics/java/#_data
Known Compatablity Issues
sensu-plugins/community#77
sensu-plugins/community#97