-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Updates for rust-openssl 0.6.0 #435
Conversation
Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:
Guidelines are available at https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md This message was auto-generated by https://gitcop.com |
@markuskobler will you try again? Just hit the same error. |
@paulosuzart Are you hitting the error below, because apparently its working by design.
Their might be a better way but I got round it by using a submodule to checkout rust-openssl and then had a
|
ssl_context.set_verify(SslVerifyNone, None); | ||
try!(ssl_context.set_cipher_list("DEFAULT").map_err(lift_ssl_error)); | ||
try!(ssl_context.set_certificate_file(cert, X509FileType::PEM).map_err(lift_ssl_error)); | ||
try!(ssl_context.set_private_key_file(key, X509FileType::PEM).map_err(lift_ssl_error)); |
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.
Maybe we want to change the signature of this function to return HttpResult
so that we can use From
instead of map_err(lift_ssl_error)
.
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.
Happy to change that as well but that would be the first use of HttpResult net.rs
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 think we should, so that we can keep the entire error around rather than losing information like we do now.
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.
If that's what we want, it can be in a separate PR. Although, with the io::Error
changes to holding a Box<Error>
, the original SSL error is no longer really lost. It's boxed into a trait object.
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.
So had you envisaged adding adding a new HttpSslError(SslError) to the HttpError enum?
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 think you also need to change the NetworkConnector::connect
and NetworkListener::accept
signatures?
@markuskobler your fix are pertinent and useful. I tried to fix myself and ended up fixing exactly the way you did. Before submit my PR I found yours. So if you just fix your commit messages they might be accepted. |
@paulosuzart Sorry now I understand. So I thought my rebase markuskobler@660a362 fixed the gitcop warning. I think it just needs someone to kick off the travis build again now that it looks like cookie-rs has been upgraded to the latest rust openssl. |
Just restarted travis. |
Updates for rust-openssl 0.6.0
Thanks for this! |
No description provided.