-
Notifications
You must be signed in to change notification settings - Fork 183
Revert add_certificate_chain_file changes #320
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
Conversation
|
Why can’t we fix the issues? Because this is important use case. |
|
If you did fix it with mswin environment, We can ship this feature. You should care about all of the supported platforms before merge pull-request. |
Completely agree. This PR was passing all tests at the time, so our test coverage in this repository is obviously not good enough. I'll see what I can do. |
|
Sometimes a problem cannot be associated with a particular 'space' in an organization, and hence, the blame falls on the whole organization. Re this problem, in the past, I don't think any repos were tested against a Ruby mswin build. It wasn't needed back when the stdlib was integrated in Ruby. Now that much of it is separate (thanks to a lot of work by hsbt), that need exists. The organization/community adapted, and now that CI is being done. |
f82852b to
36af4c0
Compare
|
I think it was not a good idea to merge this... the correct solution should be to find out why Windows is failing and fix that. Because now the history is very messy and in order to move forward we do actually need to fix this. |
|
If the original commit breaks the build, then reverting until it can be fixed seems right to me—failing commits don't belong in master. In the meantime, there's a lot of good changes ready to ship. |
Let's revert the changes for now, as it cannot be included in the 2.2.0 release. My comment on ruby#257: > A blocker is OpenSSL::SSL::SSLContext#add_certificate_chain_file. It > has a pending change and I don't want to include it in an incomplete > state. > > The initial implementation in commit 46e4bdb was not really > useful. The issue is described in ruby#305. ruby#309 extended it > to take the corresponding private key together. However, the new > implementation was incompatible on Windows and was reverted by ruby#320 to > the initial one. > > (The prerequisite to implement it in) an alternative way is ruby#288, and > it's still cooking. This effectively reverts the following commits: - dacd089 ("ssl: suppress test failure with SSLContext#add_certificate_chain_file", 2020-03-09) - 46e4bdb ("Add support for SSL_CTX_use_certificate_chain_file. Fixes ruby#254.", 2019-06-13)
Unfortunately,
add_certificate_chain_filechanges were broken with vs2015 to vs2019 environments. I revert all of the related commits for releasing 2.2.0.