New Regex flag to filter container using Regex String. #69
Conversation
|
Thanks for your contribution to Sensu plugins! Without people like you submitting PRs we couldn't run the project. I will review it shortly. |
CHANGELOG.md
Outdated
| @@ -6,6 +6,10 @@ Which is based on [Keep A Changelog](http://keepachangelog.com/) | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ## [3.1.2] - 2018 | |||
There was a problem hiding this comment.
In general the versioning and dating are left to the maintainers for several reasons:
- we may disagree on the impact of the change and therefore how its versioned. In this case it would be a minor version bump not a patch as you are adding new functionality. To better understand how we version you can read about it here
- we date the release rather then when the contribution was submitted, PR review and acceptance has no SLA and is not guaranteed to be merged within the same day
- PRs are not a FIFO queue, as such versioning them prior to acceptance leads to needing to ask people to rebase more often
CHANGELOG.md
Outdated
| @@ -6,6 +6,10 @@ Which is based on [Keep A Changelog](http://keepachangelog.com/) | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ## [3.1.2] - 2018 | |||
| ### Added | |||
| - metrics-docker-stats.rb: New Regex flag to filter container using Regex String. | |||
There was a problem hiding this comment.
let's include the flag name here --container-list-regex
bin/metrics-docker-stats.rb
Outdated
| description: 'Regex for container list to collect metrics for', | ||
| short: '-R CONTAINER_LIST_REGEX', | ||
| long: '--container-list-regex CONTAINER_LIST_REGEX', | ||
| default: '' |
There was a problem hiding this comment.
any benefit of defaulting to an empty string vs just checking when its nil?
There was a problem hiding this comment.
We can check as nil , I will refactor code to usenil.
bin/metrics-docker-stats.rb
Outdated
| def run | ||
| @timestamp = Time.now.to_i | ||
| @client = DockerApi.new(config[:docker_host]) | ||
|
|
||
| list = if config[:container] != '' | ||
| [config[:container]] | ||
| elsif config[:container_list_regex] != '' |
There was a problem hiding this comment.
this can alternatively be expressed as elsif !config[:container_list_regex].empty? but I think it might be best to remove the default empty string and check for !config[:container_list_regex].nil?
|
Overall looks like a great improvement. I took a look at the CI failures, I will work on getting those clean this weekend. |
|
So the CI failure is caused by ruby/net-telnet#14 where |
…nymore Fixed up minor PR issues. Signed-off-by: Ben Abrams <me@benabrams.it>
|
its failing for ruby 2.1. @majormoses Would you take a look into it ? I though it was dropping support for ruby 2.3? |
|
So the problem now was that Honestly at this point ruby
In the changelog you will want a section such as this: This changelog entry should go above your other changes as we want people to first read about breaking changes and then what new features or fixes they want. |
Pull Request Checklist
Is this in reference to an existing issue?
No , but Filter by regex was feature I wanted for quite some time.
General
Update Changelog following the conventions laid out on Our CHANGELOG Guidelines
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