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

Allow merging of SSL opts #655

Merged
merged 3 commits into from
Mar 16, 2021

Conversation

spydon
Copy link
Contributor

@spydon spydon commented Aug 26, 2020

Related to #654
Since it isn't currently possible to merge your custom ssl opts with the default ones it could be a first step to expose the default ones so that the merging can be done client side.

@benoitc
Copy link
Owner

benoitc commented Aug 26, 2020

wouldn't exposing the partial_chain function just be enough?

@spydon
Copy link
Contributor Author

spydon commented Aug 26, 2020

It would work, but then the client still needs to duplicate the full list of default opts when setting custom ones.

@spydon
Copy link
Contributor Author

spydon commented Aug 26, 2020

What do you think @benoitc?
And also, do you know why the checks are failing on some of the erlang versions?

@benoitc
Copy link
Owner

benoitc commented Aug 26, 2020

What do you think @benoitc?
And also, do you know why the checks are failing on some of the erlang versions?

don't worry about the syntax issue. It's one of these usual bugs of Travis...

The issue with this change is that some people will still want maybe to use some other settings imo. Maybe instead of replacing ssl options we could merge them or provide a way to merge defaults and the one in options ? this could be done there: https://github.com/benoitc/hackney/blob/master/src/hackney_connection.erl#L125

Thoughts?

@spydon
Copy link
Contributor Author

spydon commented Aug 26, 2020

Yeah, that would be a better solution. But that would either be a breaking change, or to introduce a new method there?
With the current change it will work the same as it does currently for those who want to use other settings, since the merging of the settings will need to be done client side and you would have to consciously call the new function to get the default opts.

@spydon
Copy link
Contributor Author

spydon commented Aug 27, 2020

@benoitc would something like this be better maybe?

@spydon spydon changed the title Expose default SSL opts Allow merging of SSL opts Aug 27, 2020
@spydon
Copy link
Contributor Author

spydon commented Sep 4, 2020

Sorry to be a bother, have you had any time to look at this yet @benoitc? :)

@benoitc
Copy link
Owner

benoitc commented Sep 7, 2020

sorry for the late answer. That would probably work. I am doing a release today possibly including it :)

@benoitc benoitc added this to the 1.17.0 milestone Sep 11, 2020
@spydon
Copy link
Contributor Author

spydon commented Nov 9, 2020

@benoitc is the plan to get this out on the 14th of November like it says on the milestone date? :)

@spydon
Copy link
Contributor Author

spydon commented Jan 11, 2021

@benoitc any plan for when the new release will be released?

@benoitc
Copy link
Owner

benoitc commented Jan 12, 2021

@spydon next release will happen next week on thursday. this feature should be part of it normally.

@benoitc
Copy link
Owner

benoitc commented Jan 18, 2021

@spydon next release will happen next week on thursday. this feature should be part of it normally.

that has been slightly delayed. It will happen this week, coming with a new documentation website.

@spydon
Copy link
Contributor Author

spydon commented Mar 16, 2021

Any updates on the release? :)
We are seeing several teams having this problem now.

@benoitc benoitc merged commit f82382d into benoitc:master Mar 16, 2021
@benoitc
Copy link
Owner

benoitc commented Mar 16, 2021

Any updates on the release? :)
We are seeing several teams having this problem now.

i will merge a minor release with it later today

This was referenced Mar 17, 2021
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