-
Notifications
You must be signed in to change notification settings - Fork 150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecation warning for ruby 2.5.0 fixed #189
Conversation
webhoernchen
commented
Jan 2, 2018
- httpi/auth/ssl.rb:13: warning: constant OpenSSL::SSL::SSLContext::METHODS deprecated
* httpi/auth/ssl.rb:13: warning: constant OpenSSL::SSL::SSLContext::METHODS deprecated
Looks good but should probably add 2.5.0 to .travis.yml to ensure test coverage. |
SSL_VERSIONS = if ssl_context.const_defined? :METHODS_MAP | ||
ssl_context.const_get(:METHODS_MAP).keys | ||
else | ||
ssl_context::METHODS.reject { |method| method.match(/server|client/) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using const_get
in one case and ::
in other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because METHODS_MAP is a private constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because METHODS_MAP is marked as private
Look at: https://github.com/ruby/ruby/blob/trunk/ext/openssl/lib/openssl/ssl.rb#L222
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it sucks that we are using private constants from other libs :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it sucks that we are using private constants from other libs :(
@tycooon I agree with you.
Someone suggests a better approach?
These values change by different OpenSSL::SSL::SSLContext
? We can't set this values on SSL_VERSIONS
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only see either fetching it from the OpenSSL lib as @webhoernchen is doing it or defining it directly SSL_VERSIONS = [:TLSv1_2, :TLSv1_1, :TLSv1, :SSLv3, :SSLv23]
but having to update it manually if anything changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webhoernchen what do you think? I see no problem with SSL_VERSIONS = [:TLSv1_2, :TLSv1_1, :TLSv1, :SSLv3, :SSLv23]
approach.
I think it makes sense to fix the deprecation warnings in the specs as well https://github.com/savonrb/httpi/blob/master/spec/httpi/auth/ssl_spec.rb#L6 Is it possible to get a release after merging? We have a lot of cronjobs executing rake tasks and I get emails for every cronjob because of the deprecation warning 😅 |
SSL_VERSIONS = if ssl_context.const_defined? :METHODS_MAP | ||
ssl_context.const_get(:METHODS_MAP).keys | ||
else | ||
ssl_context::METHODS.reject { |method| method.match(/server|client/) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it sucks that we are using private constants from other libs :(
@tycooon I agree with you.
Someone suggests a better approach?
These values change by different OpenSSL::SSL::SSLContext
? We can't set this values on SSL_VERSIONS
directly?
@bakku for sure! 👍 |
Travis is failing with jruby. https://travis-ci.org/savonrb/httpi/jobs/327656334 Seems some problem on install ...
Optional quest: should be good to check if tests are ok locally with jruby. |
@bakku next version should enter:
|
2 similar comments
Is this likely to get merged/released soon? Would like to clean the deprecation notice from our logs 😄 |
@rogerleite thanks for reviewing, does this look good now? I have permissions to merge, and can make a new release, if you'd like a hand. |
The guy who monitors a shared IT mailbox here at work will appreciate the reduction in Cron Daemon emails. Eagerly awaiting this release... |
hi guys, any update on when we will see a release? Additionally the |
Hi @bakku, this fix is already released on version https://github.com/savonrb/httpi/releases/tag/v2.4.3 Savon needs to change |
You're right, I'm sorry. I somehow thought that the mentioned release was before this pull request. Thanks @rogerleite |