Changes to maintain ruby 1.8.7 compatability#15
Changes to maintain ruby 1.8.7 compatability#15lstephen wants to merge 3 commits intospreedly:masterfrom
Conversation
|
Thanks @lstephen. Would it be possible to isolate the Ruby version part of this PR from the consrui.yml addition? I'm sympathetic to build tools, but also don't want to have everybody's preferred tool config in the repo either. Sorry! |
|
Sure, I will do that. The reason for including it was for ease of testing (e.g., rbenv struggles to install ruby 1.8.7 on OS X) |
rwdaigle
left a comment
rwdaigle
left a comment
There was a problem hiding this comment.
@lstephen Thanks for this!
Quick question, mainly as it relates to future plans. Given that Ruby 1.8.7 is no longer supported, I'd be hesitant to introduce a fork where we now have to maintain two paths of development.
How do you see future updates happening - do you think backwards compatibility can be maintained in a single line of dev or will we need to fork? Also, are you planning on upgrading Ruby anytime soon :) ?
| decipher = OpenSSL::Cipher::AES128.new(:CTR) | ||
| decipher.decrypt | ||
| decipher.key = symmetric_key | ||
| decipher.auth_data = "" |
There was a problem hiding this comment.
This change will, I believe, break forward Ruby compatibility?
There was a problem hiding this comment.
The current tests in place all pass with this line removed. I must admit I didn't look any further than that. If it does break forward ruby compatibility we should be able to put together a test that confirms this.
There was a problem hiding this comment.
This is safe to remove, AES-CTR is not an AEAD mode so it didn't make sense anyway. Older OpenSSL gems just didn't complain about it. Starting with OpenSSL gem 2.0 (bundled with Ruby 2.4) it started getting stricter (e.g. raising error on too long keys instead of truncating).
| rescue LoadError | ||
| # Ignore, byebug is not installed for older ruby versions | ||
| end | ||
|
|
There was a problem hiding this comment.
I would love to see dev/debug-specific tooling like pry removed all together. Any thoughts on that?
There was a problem hiding this comment.
I'm ok with this, because I'm used to working with older ruby versions where we can't use these tools anyway.
|
Regarding Ruby 1.8.7, I don't think a fork should be necessary. When we have upgraded services we've found we can maintain a code base that works with older and newer versions. I certainly wouldn't hold dropping Ruby 1.8.7 support against you and couldn't even call it a bad decision. We do have plans to upgrade and we have done some work towards Ruby 2.2.5. But, we do not have a deadline or schedule for this change. |
👍 We have a few things in flight currently, mainly #13. My recommendation, @deedeelavinder, would be for us to consider 1.8.7 support after Google Pay support has settled. |
3 participants
This PR suggests the following changes: