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.

Sign up for GitHub

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

Jump to bottom

Implement HTTPI::Auth::SSL#ciphers configuration #214

Merged
merged 3 commits into from
Nov 16, 2020
Merged

Implement HTTPI::Auth::SSL#ciphers configuration #214

merged 3 commits into from
Nov 16, 2020

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented Nov 13, 2020

This is an updated version of #180 by @faucct. That PR was never merged.

@rogerleite
Copy link
Member

Hi Christian.
First, thanks for "reopening" this PR.

Before we merge, could you please check the 2 errors HTTPI::Adapter::Curb from the tests?

Thanks,
Roger Leite

@c960657
Copy link
Contributor Author

c960657 commented Nov 15, 2020

The Curb tests were also failing on master. I fixed this by changing the certs to those used in Puma tests.

I don't know what exactly was wrong with the old certificates. The certs in Puma were recently upgraded in puma/puma#2333, but the PR just mentioned that the old certs were “outdated”.

olleolleolle
olleolleolle approved these changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@olleolleolle olleolleolle left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks so much, everyone!

♥️ 💛 💚 💙 💜 @faucct, @c960657 ♥️ 💛 💚 💙 💜

A focused improvement, and with a few naming fixes in the specs, thrown in.

spec/integration/excon_spec.rb
@@ -1,7 +1,7 @@
require "spec_helper"
require "integration/support/server"

describe HTTPI::Adapter::HTTPClient do
describe HTTPI::Adapter::Excon do
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good fix!

spec/integration/net_http_persistent_spec.rb

describe HTTPI::Adapter::NetHTTP do
describe HTTPI::Adapter::NetHTTPPersistent do
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

spec/integration/net_http_persistent_spec.rb
@@ -122,6 +123,17 @@
response = HTTPI.get(request, adapter)
expect(response.body).to eq("get")
end

it "works with ciphers" do
skip("Requires net-http-persistent 3.x") unless Net::HTTP::Persistent::VERSION.start_with? "3."
Copy link
Contributor

Choose a reason for hiding this comment

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

Good skip!

@olleolleolle
Copy link
Contributor

olleolleolle commented Nov 16, 2020 •
edited
Loading

@c960657 Perhaps you can add a CHANGELOG.md line (under a new ## Unreleased heading at the top)?

Update: Thanks!

@olleolleolle olleolleolle merged commit d3b9843 into savonrb:master Nov 16, 2020
@olleolleolle
Copy link
Contributor

Alright, all green, and now merged. Whew!

@olleolleolle olleolleolle mentioned this pull request Nov 16, 2020
Closed
@rogerleite
Copy link
Member

Thanks @c960657 and @olleolleolle
👍 good job!

@edmorley
Copy link

Hi - thank you for adding this :-)

I don't suppose a new httpi release could be published that includes this change? It's currently only on master. My understanding is that the released equivalent feature in savron (savonrb/savon#794) depends on this being fixed too.

@c960657 c960657 deleted the ssl-ciphers branch March 5, 2021 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olleolleolle olleolleolle olleolleolle approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@c960657 @rogerleite @olleolleolle @edmorley