-
Notifications
You must be signed in to change notification settings - Fork 47
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 config options merging #111
Conversation
Looks great! I'd always thought that the first value wins, as with keyword lists. I added some tests to cover this, since it's surprising behaviour. Thanks for the PR! |
Pushed as |
This breaks options for gen_tcp: https://www.erlang.org/doc/man/gen_tcp#listen-2 AFAIK keyword list is an Elixir only thing. In Erlang the options (at least for gen_tcp) is a list of tuples or atoms. I'm setting the ipfamily explicitly so the list will be akin to: |
Aw shoot you're right. We had fixed that previously but I don't think it's under test, hence the regression I'm out for the evening but I can fix this up tomorrow. Sorry for the trouble |
Fix is up at #112, but there's a failing test for ssl that suggests maybe it's a bit more complicated than gen_tcp. Take a look! |
I think I'm affected by this issue. Just grabbed the latest bandit (1.2.3) which nudged thousand_island to 1.3.4 My web server was failing with:
My thousand_island_options config in my prod.exs is:
Reverting thousand_island to 1.3.3 fixed it. Hope this is helpful -- Ian. |
Yep. Fix is up in the linked PR. I ran out of time to get it over the line today but should get it out as 1.3.5 tomorrow. |
Fix published as 1.3.5! |
Thanks, working again :) |
Currently the
user_options
could overwrite the@hardcoded_options
but not thedefault_options
(gen_tcp
seems to take the last option if multiple values for 1 key are defined).This fixes the problem by allowing the
user_options
to overwrite thedefault_options
and applying the@hardcoded_options
last.