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

Fix ssl_client_cert and ssl_client_key #33

Merged
merged 3 commits into from
Jul 6, 2023

Conversation

karissekjw
Copy link
Contributor

@karissekjw karissekjw commented Jul 6, 2023

Issue

There is a bug when adding client_cert and client_key into the SSL options where an error is raised due to undefined method.

image

image

What this PR does

  • This PR uses the correct method (is_a?) to prevent the error from being raised. is_a? only accepts a single argument, this has been fixed too.
  • Added specs to verify that the changes work as expected

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.

Well, that was an embarrassing miss 😅
Thank you for fixing it @karissekjw and adding tests 💯 🙌

@iMacTia iMacTia changed the base branch from master to test July 6, 2023 10:42
@iMacTia iMacTia changed the base branch from test to master July 6, 2023 10:43
@iMacTia iMacTia merged commit f43bce1 into lostisland:master Jul 6, 2023
@karissekjw
Copy link
Contributor Author

@iMacTia thanks for the swift response ⚡ seems like CI failed due to rubocop. Should I open another PR to fix this?

@iMacTia
Copy link
Member

iMacTia commented Jul 6, 2023

I meant to fix it myself (also because it's unrelated to your changes), but I've been in back-to-back meetings all day.
If you'd like to fix them I can definitely merge your fix and cut a release 🙌

karissekjw added a commit to karissekjw/faraday-http that referenced this pull request Jul 6, 2023
@karissekjw
Copy link
Contributor Author

Opened a PR here! #34

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.

2 participants