-
Notifications
You must be signed in to change notification settings - Fork 18
[Merged by Bors] - Add SSL support to kafka-sink #404
Conversation
fa8103e
to
77aa29f
Compare
@@ -30,7 +30,9 @@ fluvio = { git = "https://github.com/infinyon/fluvio", branch = "mas | |||
fluvio-smartengine = { git = "https://github.com/infinyon/fluvio", branch = "master" } | |||
fluvio-protocol = { git = "https://github.com/infinyon/fluvio", branch = "master" } | |||
fluvio-spu-schema = { git = "https://github.com/infinyon/fluvio", branch = "master" } | |||
sasl2-sys = { git = "https://github.com/infinyon/rust-sasl/", branch = "fix-cross-compiling" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we fork instead of depend on branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean have it point to master
or a revision? Yeah, that should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you create a PR in upstreaming repo to see if they can merge this change
…lay/rdkafka-ssl-sasl-2nd-try
@@ -30,6 +30,8 @@ fluvio = { git = "https://github.com/infinyon/fluvio", branch = "mas | |||
fluvio-smartengine = { git = "https://github.com/infinyon/fluvio", branch = "master" } | |||
fluvio-protocol = { git = "https://github.com/infinyon/fluvio", branch = "master" } | |||
fluvio-spu-schema = { git = "https://github.com/infinyon/fluvio", branch = "master" } | |||
sasl2-sys = { git = "https://github.com/infinyon/rust-sasl/", branch = "fix-cross-compiling" } | |||
krb5-src = { git = "https://github.com/infinyon/rust-krb5-src", branch = "fix-cross-compiling" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you create a PR in upstreaming repo to see if they can merge this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kafka = "0.9.0" | ||
# There is some difference between rev v0.26.0 and the 0.26.0 on crates.io. | ||
# Changing it to use the crates.io version breaks the musl cross compile. | ||
rdkafka = { git="https://github.com/fede1024/rust-rdkafka", rev="v0.26.0", features = ["cmake-build", "libz-static", "tokio", "ssl-vendored" , "gssapi-vendored"], default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. This is still required. fede1024/rust-rdkafka#446 (comment) it's not ideal but has to do with the openssl runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we need integration tests, maybe we can use docker-compose for a local kafka instance. WIll approve to see if we can merge to test using a docker hub image (unable to build with macos)
bors r+ |
Pull request successfully merged into main. Build succeeded:
|
No description provided.