Skip to content
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

HTTP.request(): infer socket_type_tls from sslconfig, if provided #1106

Merged
merged 4 commits into from
Apr 6, 2024

Conversation

cmcaine
Copy link
Contributor

@cmcaine cmcaine commented Sep 7, 2023

Fixes #1104.

This PR only affects client requests, because those were the only ones broken. For now OpenSSL is not supported in HTTP.jl servers.

I didn't just revert the change to SOCKET_TYPE_TLS[] because that change has been released for months and users may now be relying on sslconfig accepting an OpenSSL.SSLContext value, just as I was relying on it accepting an MbedTLS.SSLConfig value.

Fixes JuliaWeb#1104.

This PR only affects client requests, because those were the only ones
broken. For now OpenSSL is not supported in HTTP.jl servers (and,
honestly, maybe that's a good thing, OpenSSL is kinda crap).

I didn't just revert the change to SOCKET_TYPE_TLS[] because that change
has been released for months and users may now be relying on `sslconfig`
accepting an `OpenSSL.SSLContext` value, just as I was relying on it
accepting an `MbedTLS.SSLConfig` value.
@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2023

Codecov Report

Merging #1106 (acc4286) into master (8f35185) will increase coverage by 0.46%.
Report is 8 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1106      +/-   ##
==========================================
+ Coverage   82.34%   82.81%   +0.46%     
==========================================
  Files          32       32              
  Lines        3042     3072      +30     
==========================================
+ Hits         2505     2544      +39     
+ Misses        537      528       -9     
Files Coverage Δ
src/clientlayers/ConnectionRequest.jl 81.03% <100.00%> (+4.74%) ⬆️

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmcaine
Copy link
Contributor Author

cmcaine commented Apr 4, 2024

Bump. I think this is still relevant.

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM; thanks for going through the compat/refactor hoops here to get this working.

@quinnj quinnj merged commit 4693a9b into JuliaWeb:master Apr 6, 2024
10 of 11 checks passed
@cmcaine
Copy link
Contributor Author

cmcaine commented Apr 6, 2024

You're welcome! Thanks for writing HTTP.jl :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unintentional(?) breaking change to type of sslconfig
3 participants