Add aws-sdk-version check to patch #433
Conversation
|
@miq-bot assign @blomquisg cc @jrafanie |
|
Checked commit NickLaMuro@df7fce4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
LGTM. It's very pessimistic but it's probably better to not try to monkey patch a changed version of the code. Note, I'm still debating if we should do this or wait until we have any movement on the upstream PR to see if we have any idea when the proper fix will land. |
|
@jrafanie That's cool, and I agree. You were just mentioning in the previous PR about potentially backporting this to other releases, and I think this should be in place if that was something we were going to consider. Otherwise I am okay with sitting on this one for now. |
| @@ -1,3 +1,27 @@ | |||
| if Aws::VERSION != "2.9.44" | |||
| raise <<-ERROR.gsub(/ {4}/, '') | |||
There was a problem hiding this comment.
I don't like the raise when combined with an absolute version comparison. The reason is that every aws release is going to potentially cause a red master, and since we don't control that release it will happen effectively at random, which will frustrate people who have open PRs. It won't be able to be fixed until we change the number here, which, in effect will also enforce a minimum version number.
While I understand the need to want to remove the patch, I think it's cleaner to change this to logging an error to STDOUT, and perhaps to the log as well. This way, hopefully, it will be seen by a developer, who can open an issue to do the research, but not block that developer.
There was a problem hiding this comment.
@Fryguy yeah that is a good point.
I had some counter points with this, but after doing some research myself on the seahorse lib within the aws-sdk gem, it looks like it hasn't been updated significantly for a couple of years now, which lessen's my original concerns.
My biggest fear is that the devs would be perfectly okay with changing the underlying API for Seahorse (for bug fixes, refactoring, etc.), as long as it doesn't break the public API of the gem. But by the looks of things, the seahorse/client/net_http/connection_pool.rb file has remained relatively static over the years:
- https://github.com/aws/aws-sdk-ruby/commits/master/gems/aws-sdk-core/lib/seahorse/client/net_http/connection_pool.rb
- https://github.com/aws/aws-sdk-ruby/commits/dfd175f2/aws-sdk-core/lib/seahorse/client/net_http/connection_pool.rb
- https://github.com/aws/aws-sdk-ruby/commits/65852fd3/lib/seahorse/client/net_http/connection_pool.rb
- https://github.com/aws/aws-sdk-ruby/commits/b563fdbc/vendor/seahorse/lib/seahorse/client/net_http/connection_pool.rb
- https://github.com/aws/aws-sdk-ruby/commits/af911654/lib/seahorse/client/net_http/connection_pool.rb
- https://github.com/aws/aws-sdk-ruby/commits/8dedfa56/lib/seahorse/client/plugins/net_http/connection_pool.rb
- https://github.com/aws/aws-sdk-ruby/commits/e5c154c9/lib/seahorse/client/net_http_connection_pool.rb
So basically since 2013, the implementation for this class has remained un-changed, with maybe one addition added to this method that I monkey patched since then.
That said, probably wouldn't hurt to add a test that checks the implementation of that method, line by line, and if something changes then we yell " 🔥 🔥 FIRE!!1! 🔥 🔥 " in the tests.
I can look at doing something like that and open up a separate PR to compare.
There was a problem hiding this comment.
Welp, here is attempt number 2: #434
| @@ -1,3 +1,27 @@ | |||
| if Aws::VERSION != "2.9.44" | |||
| raise <<-ERROR.gsub(/ {4}/, '') | |||
There was a problem hiding this comment.
|
Closing in favor of #434 |
Includes a version check as a poor man's way of validating that the monkey patch from #432 is still valid. Instructions included in the raise, and this should cause the automated tests to fail alerting the team that changes might be necessary.
Links