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

Handle verify_hostname ssl option #23

Merged
merged 2 commits into from
Jul 6, 2022

Conversation

kazarin
Copy link
Contributor

@kazarin kazarin commented Jul 1, 2022

Hi,
This is support for lostisland/faraday#1428 option. Also I am thinking about adding specs, but don't know yet what can be added

@iMacTia
Copy link
Member

iMacTia commented Jul 4, 2022

Thank you @kazarin, you can add a test that checks if http was configured correctly in this file. We test other configuration fields similarly, so you should be able to take inspiration from them.

As for the failing tests in Ruby 3.0+, it seems like something changed in the way Ruby checks for structs, and is raising an error when the field you're looking for doesn't belong to the struct. This means this change will break in versions of Faraday that do not support the new SSL option.

Ideally, we should make the code work with older versions of Faraday prior to the introduction of the new field

@kazarin kazarin force-pushed the handle_verify_hostname_ssl_option branch from 4257d64 to 950f70b Compare July 5, 2022 07:32
@kazarin
Copy link
Contributor Author

kazarin commented Jul 5, 2022

Thanks for detailed explanation @iMacTia!
Added specs and support for older versions of faraday

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Thank you @kazarin, LGTM 👍!
Just a few minor Rubocop issues that need fixing before we can merge this

@kazarin kazarin force-pushed the handle_verify_hostname_ssl_option branch from 70d1791 to babd104 Compare July 6, 2022 04:54
@kazarin
Copy link
Contributor Author

kazarin commented Jul 6, 2022

Sorry for little mess.
Finally fixed rubocop warnings and modified specs for a little

@iMacTia
Copy link
Member

iMacTia commented Jul 6, 2022

No worries at all, thank you for fighting with the specs till the end 😄!
It makes sense to test only the scenario for Faraday 2.3 as you did in the last commit 👍

@iMacTia iMacTia merged commit 106be80 into lostisland:main Jul 6, 2022
@bnhassin
Copy link

bnhassin commented Aug 1, 2022

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.

None yet

3 participants