[CVE-2017-8418] - updating rubocop dependency. #3
Conversation
Breaking Changes: - removed ruby `< 2.1` support Misc: - fixed bug in handler which prevented string interpolation - typos - changelog guidelines Signed-off-by: Ben Abrams <me@benabrams.it>
majormoses
force-pushed
the
fix/CVE-2017-8418
branch
from
9a44b0f
to
a0d4b4f
Compare
There was a problem hiding this comment.
This is great, thanks @majormoses. Given that you fixed a bug in the handler which prevented string interpolation, could you provide a test artifact or test to verify functionality? While these rubocop updates are helpful, we don't want to have a behavioral change slip in without testing.
Thanks.
|
@mbbroberg unfortunately I do not have a zendesk account to test with. |
|
No problem. There are two other ways we could feel confident about the behavior:
Let's holding off on merging for now while we aim to for one or both of those. Thanks! |
|
Writing a meaningful (integration) test would still require an account of some kind while most of the plugins are for open source software some of them are behind paywalls and require either an account or significant effort to stub, require specific tech knowledge, and realistically provide limited value. I am OK holding off but I do not have time to stub ~200 plugins right now in order to get through the hundreds of PRs needed to update rubocop and yard for security vulnerabilities I feel like these get a pass. If you can find someone in a timely manner with a zendesk to test I am all for waiting but I don't want this to sit open for more than a week or two waiting. Looking at the changelog this plugin has barely been touched so I don't think there are likely a lot of users leveraging this so it may be difficult to find someone. Does sensu internally use zendesk? |
|
Hey @majormoses, no need for you to feel like you need to do anything across the 200 plugins! We're discussing only this PR right now. Since the behavior has changed, it's our goal (that you inspired!) to include test artifacts with changes. It's only fair to hold ourselves to that same standard.
We don't. I agree that, due to low traffic on this repo, we can leave it open for a max of 2 weeks while we see if others have Zendesk and would be able to help. |
|
I consider the bug fix to fall under the "obvious fix" rule anyways but I am happy to leave it open for a week or two. |
|
Looks like we'll move forward with this over #2. |
| @@ -82,7 +82,7 @@ def handle | |||
| end | |||
| end | |||
| rescue Timeout::Error | |||
| puts 'zendesk -- timed out while attempting to create a ticket for #{ticket_subject} --' | |||
| puts "zendesk -- timed out while attempting to create a ticket for #{ticket_subject} --" | |||
There was a problem hiding this comment.
Here is a testing artifact demonstrating the change.
Before:
$ irb
irb(main):001:0> a = "Ay"
=> "Ay"
irb(main):002:0> b = "Bee"
=> "Bee"
irb(main):003:0> p 'a #{b}'
"a \#{b}"
=> "a \#{b}"
After:
$ irb
irb(main):001:0> a = "Ay"
=> "Ay"
irb(main):002:0> b = "Bee"
=> "Bee"
irb(main):004:0> p "a #{b}"
"a Bee"
=> "a Bee"
Breaking Changes:
< 2.1supportMisc:
Signed-off-by: Ben Abrams me@benabrams.it
Pull Request Checklist
sensu-plugins/community#77
General
Update Changelog following the conventions laid out on Keep A Changelog
Update README with any necessary configuration snippets
Binstubs are created if needed
RuboCop passes
Existing tests pass
Purpose
Address CVE, see parent issue for details
Known Compatability Issues
Removes ruby
< 2.1support