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

Added metrics strongswan connection #8

Merged
merged 1 commit into from
Mar 8, 2018
Merged

Added metrics strongswan connection #8

merged 1 commit into from
Mar 8, 2018

Conversation

yuri-zubov
Copy link
Contributor

@yuri-zubov yuri-zubov commented Mar 3, 2018

Pull Request Checklist

Is this in reference to an existing issue?

General

  • Update Changelog following the conventions laid out on 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

Copy link

@mbbroberg mbbroberg left a comment

Choose a reason for hiding this comment

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

Looks good! We'll merge this one along with your other PRs as we prep for the next release. Thank you again @yuri-zubov - these PRs are awesome 👍

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.

LGTM.
Just some minor grammatical fixups.

def run
ipsec_status = run_ipsec_listcounters

founded = false
Copy link
Member

Choose a reason for hiding this comment

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

can we use found?

result = line.match(/(?<name>.*)\{.*(?<bytes_i>\d+)\ bytes_i.*\ (?<bytes_o>\d+)\ bytes_o/)
next unless result

founded = true
Copy link
Member

Choose a reason for hiding this comment

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

can we use found?

output "#{config[:scheme]}.#{result[:name]}.bytes_i", result[:bytes_i]
output "#{config[:scheme]}.#{result[:name]}.bytes_o", result[:bytes_o]
end
if founded
Copy link
Member

Choose a reason for hiding this comment

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

can we use found?

@majormoses
Copy link
Member

Any chance we could get a testing artifact?

@yuri-zubov
Copy link
Contributor Author

@majormoses, yes I will write a small test, please merge this #7 PR.

@mbbroberg
Copy link

#7 merged 👍

@yuri-zubov
Copy link
Contributor Author

recheck again, please

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.

Overall looks great, just a couple of minor things.

@@ -4,8 +4,6 @@ cache:
install:
- bundle install
rvm:
- 2.1
- 2.2
Copy link
Member

Choose a reason for hiding this comment

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

hmm we normally keep support for all versions of !EOL? versions per: https://github.com/sensu-plugins/documentation/blob/master/docs/FAQ.md#what-is-the-policy-on-supporting-end-of-lifeeol-ruby-versions

Per the ruby support dates: https://www.ruby-lang.org/en/downloads/branches/

Ruby 2.2
status: security maintenance
release date: 2014-12-25
EOL date: scheduled for 2018-03-31

In this case I am willing to make an exception because of the low volume this repo has had and it's close to an official EOL date. We will still version this as a major so that anyone who might be using this and pinning their gems will be unaffected until they are ready for it.

Copy link
Contributor Author

@yuri-zubov yuri-zubov Mar 6, 2018

Choose a reason for hiding this comment

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

@majormoses <<~ operator in heredoc is supported by ruby 2.3

CHANGELOG.md Outdated
@@ -4,6 +4,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
This CHANGELOG follows the format listed [here](https://github.com/sensu-plugins/community/blob/master/HOW_WE_CHANGELOG.md)

## [Unreleased]
- Added new metrics-strongswan-connection.rb plugin for getting connection metrics (@yuri-zubov)
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 put this under a ### Added header?

CHANGELOG.md Outdated
@@ -4,6 +4,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
This CHANGELOG follows the format listed [here](https://github.com/sensu-plugins/community/blob/master/HOW_WE_CHANGELOG.md)

## [Unreleased]
- Added new metrics-strongswan-connection.rb plugin for getting connection metrics (@yuri-zubov)
### Breaking Changes
Dropping ruby `< 2.3` support (@yuri-zubov)
Copy link
Member

Choose a reason for hiding this comment

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

missing the - to make it an item in an unordered list.

@@ -39,6 +39,6 @@ Gem::Specification.new do |s|
s.add_development_dependency 'rake', '~> 10.0'
s.add_development_dependency 'redcarpet', '~> 3.2'
s.add_development_dependency 'rspec', '~> 3.1'
s.add_development_dependency 'rubocop', '~> 0.40.0'
s.add_development_dependency 'rubocop', '~> 0.53'
Copy link
Member

Choose a reason for hiding this comment

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

We are trying to update all the plugins to the same version:

 ~> 0.51.0

sensu-plugins/community#77

test/fixtures.rb Outdated
@@ -1,27 +1,56 @@
# rubocop:disable Layout/TrailingWhitespace
Copy link
Member

Choose a reason for hiding this comment

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

Hmm I don't believe a heredoc should trigger this I will double check on this though.

@yuri-zubov
Copy link
Contributor Author

yuri-zubov commented Mar 6, 2018

@majormoses fixed comments

@majormoses majormoses merged commit b90f5a2 into sensu-plugins:master Mar 8, 2018
@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.

3 participants