check-ec2-cpu_balance.rb: Make instance families configurable. - #320
Conversation
72c2c7d to
1c225b5
Compare
|
Thanks for your contribution to Sensu plugins! Without people like you submitting PRs we couldn't run the project. I will review it shortly. |
majormoses
left a comment
majormoses
left a comment
There was a problem hiding this comment.
I think this looks good after fixing the typo. Could you please also include a changelog entry?
| instances.reservations.each do |reservation| | ||
| reservation.instances.each do |instance| | ||
| next unless instance.instance_type.start_with? 't2.' | ||
| next unless instance.instance_type.start_with?(*config[:instance_families].split(',')) |
There was a problem hiding this comment.
I think * is a typo? Also it might make sense to do this when passed in the var, for example: https://github.com/sensu-plugins/sensu-plugins-aws/blob/16.1.0/bin/check-cloudwatch-alarms.rb#L53 this makes it easier to use in other locations in this check if that makes sense.
There was a problem hiding this comment.
I think * is a typo?
No, it's the splat operator because start_with does not take an array as argument but as many strings as we want.
Also it might make sense to do this when passed in the var, for example: https://github.com/sensu-plugins/sensu-plugins-aws/blob/16.1.0/bin/check-cloudwatch-alarms.rb#L53 this makes it easier to use in other locations in this check if that makes sense.
Totally, I was not aware of this proc attribute, I'll do that !
There was a problem hiding this comment.
oh this might be a splat operator just saw the error with CI and assumed it was a typo.
There was a problem hiding this comment.
Ah thanks for the explanation sorry I didnt see it, was not refreshed when I was looking.
majormoses
added
Status: Awaiting Response
Status: Needs Changelog
and removed
Status: Needs Review
labels
@majormoses I already did. Or maybe I missed something ? |
|
@cyrilgdn nope I must have been multitasking too much. |
|
I will look into the CI failure this evening. |
@majormoses Yes I was about to asking your help because it does not seem to be linked to what I've done.
I moved the split in the proc attr. |
Ya looks unrelated, I will let you know what I find and will probably commit a fix to your branch assuming you dont have an issue with that. |
There's a new CPU burstable instance family: t3 I added it in the default value but thought it may be a good idea to add an option for that. (So no need to wait for a plugin update when AWS will add another new burstable family.) # Conflicts: # CHANGELOG.md
f45618c to
d80b1c9
Compare
|
@majormoses Done ! (but for now Travis build is pending since hours) |
| short: '-f t2,t3', | ||
| long: '--instance-families t2,t3', | ||
| proc: proc { |x| x.split(',') }, | ||
| default: ['t2', 't3'] |
There was a problem hiding this comment.
since you are doing a split(',') on input the default should be t2,t3
Ya unfortunately we get so many concurrent builders from travis and I made a major release of |
5d51ce0 to
c0fb668
Compare
c0fb668 to
510ac58
Compare
|
@majormoses Travis is green 👍 |
2 participants
There's a new CPU burstable instance family: t3
I added it in the default value but thought it may be a good idea to add an option for that.
(So no need to wait for a plugin update when AWS will add another new burstable family.)