Update check-elb-sum-requests.rb to AWS SDK v2 #297

boutetnico · 2018-10-24T09:37:11Z

Disclosure: I'm not a Ruby programmer, I cannot run the test suite, please review my changes.

Pull Request Checklist

Issue related: #240

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

Purpose

Update to AWS SDK v2.

Known Compatibility Issues

This is a breaking change because it removes aws_access_key and aws_secret_access_key options per #2.

Examples

Before this PR

Check all ELB in a region having 1 LB

$ ./check-elb-sum-requests.rb -a <key> -k <secret> -r eu-west-1
264.0
264.0
CheckELBSumRequests OK: <AWS::ELB::LoadBalancer name:my_lb>; ( within 60 seconds between 2018-10-28 07:55:37 +0000 to 2018-10-28 07:56:37 +0000)

Check all ELB in a region having 1 LB with a warning status

$ ./check-elb-sum-requests.rb -a <key> -k <secret> -r eu-west-1 --warning-over 10
351.0
351.0
CheckELBSumRequests WARNING: <AWS::ELB::LoadBalancer name:my_lb>;  Sum Requests is 351.0. (expected lower than 10.0); ( within 60 seconds between 2018-10-28 07:59:22 +0000 to 2018-10-28 08:00:22 +0000)

Check all ELB in a region having 2 LB

$ ./check-elb-sum-requests.rb -a <key> -k <secret> -r eu-west-1
126.0
126.0
0
0
CheckELBSumRequests OK: 2 load balancers total; ( within 60 seconds between 2018-10-28 08:50:16 +0000 to 2018-10-28 08:51:16 +0000)

Check all ELB in a region having 2 LB with a warning status

$ ./check-elb-sum-requests.rb -a <key> -k <secret> -r eu-west-1 --warning-over 10
206.0
206.0
0
0
CheckELBSumRequests WARNING: 2 load balancers total; <AWS::ELB::LoadBalancer name:my_lb>'s Sum Requests is 206.0. (expected lower than 10.0); ( within 60 seconds between 2018-10-28 08:51:07 +0000 to 2018-10-28 08:52:07 +0000)

After this PR

Check all ELB in a region having 1 LB

$ ./check-elb-sum-requests.rb -r eu-west-1
CheckELBSumRequests OK: my_lb; (Sum within 60 seconds between 2018-10-28 08:15:58 UTC to 2018-10-28 08:16:58 UTC)

Check all ELB in a region having 1 LB with a warning status

$ ./check-elb-sum-requests.rb -r eu-west-1 --warning-over 10
CheckELBSumRequests WARNING: my_lb;  Sum Requests is 215.0. (expected lower than 10.0); (Sum within 60 seconds between 2018-10-28 08:16:11 UTC to 2018-10-28 08:17:11 UTC)

Check all ELB in a region having 2 LB

$ ./check-elb-sum-requests.rb -r eu-west-1
CheckELBSumRequests OK: 2 load balancers total; (Sum within 60 seconds between 2018-10-28 08:58:10 UTC to 2018-10-28 08:59:10 UTC)

Check all ELB in a region having 2 LB with a warning status

$ ./check-elb-sum-requests.rb -r eu-west-1 --warning-over 10
CheckELBSumRequests WARNING: 2 load balancers total; my_lb's Sum Requests is 179.0. (expected lower than 10.0); (Sum within 60 seconds between 2018-10-28 08:59:55 UTC to 2018-10-28 09:00:55 UTC)

majormoses · 2018-10-27T19:05:33Z

@boutetnico any chance you could show us the input and output before and after the change? Here is an example

I am gonna review shortly but showing that its still functional even if only manual increases the confidence of accepting changes.

majormoses

Overall this looks good, have a couple of questions and feedback. Keep up the good work your ruby skills seem just fine to me. I was also a sysadmin who learned to code.

majormoses · 2018-10-27T19:07:54Z

CHANGELOG.md

@@ -4,6 +4,8 @@ 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]
+### Breaking Changes
+- `check-elb-sum-requests.rb` was updated to aws-sdk v2 and no longer take `aws_access_key` and `aws_secret_access_key` options. (@boutetnico)


I'd split these up into two entries but I can do that for you. Basically the removal of the CLI args are the breaking change aside from that upgrading to v2 should be backwards compatible and that should go under ### Changed.

CHANGELOG.md

majormoses · 2018-10-27T19:10:03Z

bin/check-elb-sum-requests.rb

@@ -132,7 +121,6 @@ def check_sum_requests(elb)

    @severities.each_key do |severity|
      threshold = config[:"#{severity}_over"]
-      puts metric_value


why is this being removed?

I removed this because it looks like a debug statement that was left and forgot. See example output:

$ ./check-elb-sum-requests.rb -a <key> -k <secret> -r eu-west-1 264.0 264.0 CheckELBSumRequests OK: <AWS::ELB::LoadBalancer name:my_alb>; ( within 60 seconds between 2018-10-28 07:55:37 +0000 to 2018-10-28 07:56:37 +0000)

majormoses · 2018-10-27T19:11:55Z

bin/check-elb-sum-requests.rb

-               else
-                 "#{elbs.size} load balancers total"
-               end
+    @message = "#{elbs.size} load balancers total"


Since we are removing the check to be grammatically correct we should add a parenthesis around the plural form

balancer(s)

Noted, will change it, thanks.

boutetnico · 2018-10-28T08:22:38Z

@majormoses thank you for your review, I have updated my PR according to your feedback. Please have a look at examples in my first post.

majormoses · 2018-11-02T03:16:24Z

released: https://rubygems.org/gems/sensu-plugins-aws/versions/14.0.0

I had to manually release because we have clogged up travis by enabling https://dependabot.com to help us with a lot of the maintenance work involved in keeping this project working so I can spend more time adding features and fixing bugs.

boutetnico added 2 commits October 24, 2018 17:36

Update check-elb-sum-requests.rb to AWS SDK v2

b41ccc2

Revert removal of double pipes

d8cfd56

majormoses self-requested a review October 27, 2018 19:05

majormoses added Impact: Breaking Change Status: Needs Review Type: Enhancement Area: Check labels Oct 27, 2018

majormoses requested changes Oct 27, 2018

View reviewed changes

majormoses added Status: Awaiting Response Status: Needs Changelog and removed Status: Needs Review labels Oct 27, 2018

This was referenced Oct 27, 2018

Update check-redshift-events.rb to AWS SDK v2 #298

Merged

Update check-elb-latency.rb to AWS SDK v2 #296

Merged

Update based on feedbacks

43bf2a3

boutetnico added 3 commits October 28, 2018 16:49

Fix displaying name of faulty LB

54a5b0d

fix broken output using non-existing variable

fdb438a

[ci skip] fix typo

d11423d

majormoses removed Status: Awaiting Response Status: Needs Changelog labels Nov 2, 2018

majormoses approved these changes Nov 2, 2018

View reviewed changes

majormoses merged commit b30618b into sensu-plugins:master Nov 2, 2018

boutetnico deleted the elb_sum_requests branch May 7, 2019 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update check-elb-sum-requests.rb to AWS SDK v2 #297

Update check-elb-sum-requests.rb to AWS SDK v2 #297

boutetnico commented Oct 24, 2018 •

edited

Loading

majormoses commented Oct 27, 2018

majormoses left a comment

majormoses Oct 27, 2018 •

edited

Loading

boutetnico Oct 28, 2018

majormoses Oct 27, 2018

boutetnico Oct 28, 2018

majormoses Oct 27, 2018

boutetnico Oct 28, 2018

boutetnico commented Oct 28, 2018

majormoses commented Nov 2, 2018

Update check-elb-sum-requests.rb to AWS SDK v2 #297

Update check-elb-sum-requests.rb to AWS SDK v2 #297

Conversation

boutetnico commented Oct 24, 2018 • edited Loading

Pull Request Checklist

General

Purpose

Known Compatibility Issues

Examples

majormoses commented Oct 27, 2018

majormoses left a comment

Choose a reason for hiding this comment

majormoses Oct 27, 2018 • edited Loading

Choose a reason for hiding this comment

boutetnico Oct 28, 2018

Choose a reason for hiding this comment

majormoses Oct 27, 2018

Choose a reason for hiding this comment

boutetnico Oct 28, 2018

Choose a reason for hiding this comment

majormoses Oct 27, 2018

Choose a reason for hiding this comment

boutetnico Oct 28, 2018

Choose a reason for hiding this comment

boutetnico commented Oct 28, 2018

majormoses commented Nov 2, 2018

boutetnico commented Oct 24, 2018 •

edited

Loading

majormoses Oct 27, 2018 •

edited

Loading