Add ssl_options to consumers
#413
Conversation
|
Failing tests from |
|
Giving this a bump since it's pretty minor, could you take a look? |
| @@ -24,6 +24,26 @@ defmodule KafkaEx.ConsumerGroup.Test do | |||
| assert consumer_group == :no_consumer_group | |||
| end | |||
|
|
|||
| test "create_worker allows us to pass in use_ssl and ssl_options options" do | |||
| Application.put_env(:kafka_ex, :use_ssl, true) | |||
There was a problem hiding this comment.
You'd need to reset this to the previous value at the end of your test.
| custom_ssl_options = [ | ||
| cacertfile: File.cwd!() <> "/ssl/ca-cert-custom", | ||
| certfile: File.cwd!() <> "/ssl/cert-custom.pem", | ||
| keyfile: File.cwd!() <> "/ssl/key-custom.pem" |
There was a problem hiding this comment.
Can we just reuse the files we already have? I'm also not sure how the test would pass if we don't configure the test broker to use these.
There was a problem hiding this comment.
Yah, these are the same as the existing files we already have, just the filename is different so it can be validated in the test that a different file is being used.
There was a problem hiding this comment.
Ahh that makes sense. It would be nice if we could document that somehow (even if it's just in a comment in this test). There's about a 100% chance that some day someone will wonder why we have a duplicate set of keys with different names and get very confused. Probably me because I'll forget 😂. Another option might be to create a copy of the files as part of the test run and then delete them during clean up.
There was a problem hiding this comment.
Added some documentation, and realized it would be even more explicit to use a symbolic file links to prevent duplicate files combined with a comment. Would that work?
There was a problem hiding this comment.
On second though, I also really like your proposal @dantswain of creating and deleting the files to not confuse the repo since others may look at those certs for examples of how to configure client.
|
Could you give it another round of review @dantswain ? |
markusfeyh
force-pushed
the
add-ssl-options-consumers
branch
from
2ae170d
to
a87b192
Compare
| name | ||
| ) | ||
| |> Enum.count() | ||
| wait_for(fn -> |
There was a problem hiding this comment.
This will explicitly raise an error if it the comparison does not return true which fails the test
1) test can create a topic (KafkaEx.Server0P10P1AndLater.Test)
test/integration/server0_p_10_and_later_test.exs:9
** (RuntimeError) too many tries waiting for condition
code: wait_for(fn ->
stacktrace:
test/test_helper.exs:165: TestHelper.wait_for_value/5
test/integration/server0_p_10_and_later_test.exs:27: (test)
|
Giving this a bump going into next week, could you take a look? |
|
I saw @joshuawscott provided an option to specify the worker and then pass that in as part of the fetch_options. The downside of that approach is that we would lose the benefit of the The impact of that is that we would have to maintain more code around using the library, as a pose to this PR which would allow different SSL options to be passed through as with other consumers in the library. I'm trying to think of a strong reason why this is a bad idea, and see mostly positives. Could you provide feedback? |
|
I think the SSL options do more properly go into the worker rather than fetch options, since there are other interactions (metadata, commit) that aren't fetch. |
|
Hi @dantswain / @joshuawscott |
This allows the pass-through of
ssl_optionswhen starting workers starting a consumer or consumer group.