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

support multiple metric output formats #55

Merged

Conversation

bergerx
Copy link
Contributor

@bergerx bergerx commented Feb 10, 2018

Pull Request Checklist

Is this in reference to an existing issue?
No

General

  • Update Changelog following the conventions laid out here

  • Update README with any necessary configuration snippets
    Not needed

  • Binstubs are created if needed
    Not needed

  • RuboCop passes

  • Existing tests pass
    No tests written for this package, and writing tests for bin/* files is not straightforward. You can see my own tests results against our sensu server here: https://gist.github.com/bergerx/3d026a6888006914bc068b01dd6b83d3

Purpose

Following the sensu-plugins/sensu-plugin#185 released in sensu-plugin 2.4.0 this change introduces the multiple metrics output format support.

Known Compatibility Issues

Previously the bin/metric-* scripts were only supporting graphite format, now they support various output formats but the default behaviour and output stays same, so the change is backward compatible.

Bumped dependency of sensu-plugin to 2.x you can read about it here

@@ -76,12 +81,12 @@ def api_request(resource)
end
req = Net::HTTP::Get.new(resource)
r = http.request(req)
JSON.parse(r.body)
::JSON.parse(r.body)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to prevent conflicting class name resolution with name JSON. This will ensure that the json package will be used rather than the imported Sensu::Plugin::Metric::CLI::JSON class.

Copy link
Member

Choose a reason for hiding this comment

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

yup I have had to do similar things in chef as well.

Copy link

@luisdavim luisdavim left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@majormoses majormoses left a comment

Choose a reason for hiding this comment

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

There is a breaking change by bumping sensu-plugin to 2.x so we need to call it out and I will version it as a major post acceptance.

@@ -76,12 +81,12 @@ def api_request(resource)
end
req = Net::HTTP::Get.new(resource)
r = http.request(req)
JSON.parse(r.body)
::JSON.parse(r.body)
Copy link
Member

Choose a reason for hiding this comment

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

yup I have had to do similar things in chef as well.

@@ -28,7 +28,7 @@ Gem::Specification.new do |s|
s.test_files = s.files.grep(%r{^(test|spec|features)/})
s.version = SensuPluginsSensu::Version::VER_STRING

s.add_runtime_dependency 'sensu-plugin', '~> 1.2'
s.add_runtime_dependency 'sensu-plugin', '~> 2.4'
Copy link
Member

Choose a reason for hiding this comment

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

This is a ### Breaking Change and needs to be called out as such in the changelog. Something like this:

### Breaking Change
- bumped dependency of `sensu-plugin` to 2.x you can read about it  [here](https://github.com/sensu-plugins/sensu-plugin/blob/master/CHANGELOG.md#v145---2017-03-07)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I didn't realize a change in dependency would cause a breaking change, updated changelog as you suggested.

Copy link
Member

@jaredledvina jaredledvina left a comment

Choose a reason for hiding this comment

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

👍

@majormoses majormoses merged commit 9684994 into sensu-plugins:master Mar 17, 2018
@majormoses
Copy link
Member

majormoses commented Mar 17, 2018

Sorry for dropping the ball on this one I did not see the push and the pr monitoring tool had a bug that got fixed yesterday so we got notified.

@majormoses
Copy link
Member

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

Successfully merging this pull request may close these issues.

None yet

5 participants