Skip to content

Also build rdkafka with ssl if ssl feature is enabled#1

Merged
pavdmyt merged 1 commit into
masterfrom
sigmaris-reenable-kafka-ssl
Jan 12, 2022
Merged

Also build rdkafka with ssl if ssl feature is enabled#1
pavdmyt merged 1 commit into
masterfrom
sigmaris-reenable-kafka-ssl

Conversation

@sigmaris

@sigmaris sigmaris commented Dec 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

This restores support for SSL for Kafka, removed upstream in getsentry#889 - we need SSL support to securely communicate with Aiven for Apache Kafka services. In getsentry#971 (comment) Sentry folks suggested forking and reverting the upstream removal, if Kafka SSL support is needed, hence taking this approach.

#skip-changelog

This reverts commit 69399c8.

@sigmaris sigmaris force-pushed the sigmaris-reenable-kafka-ssl branch from 8037f68 to 6443877 Compare December 10, 2021 10:43
@sigmaris sigmaris force-pushed the sigmaris-reenable-kafka-ssl branch 2 times, most recently from 1fb7446 to 14b0851 Compare January 7, 2022 10:27
@sigmaris sigmaris marked this pull request as ready for review January 7, 2022 10:28
pavdmyt
pavdmyt reviewed Jan 11, 2022
View reviewed changes

@pavdmyt pavdmyt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking that cargo update was executed as well to keep Cargo.lock up-to-date.
SSL was disabled in the upstream more than 1 year ago, we have to ensure that deps are updated.

@sigmaris sigmaris force-pushed the sigmaris-reenable-kafka-ssl branch from 14b0851 to 237ba25 Compare January 11, 2022 15:26
@sigmaris

sigmaris commented Jan 11, 2022

Copy link
Copy Markdown
Member Author

Checking that cargo update was executed as well to keep Cargo.lock up-to-date.
SSL was disabled in the upstream more than 1 year ago, we have to ensure that deps are updated.

Thought about this a bit, really we don't want to affect Cargo.lock with this revert at all. We want to adopt the same Cargo.lock from the upstream sentry/relay repo, master branch, and not modify that when this PR is merged. So I have removed any changes to Cargo.lock (which as you say were from >1 year ago) from this revert commit.

@sigmaris
Revert "Revert "fix: Also build rdkafka with ssl if ssl feature is en…
7a3220f 
…abled" (getsentry#889)"

This reverts commit 69399c8.
(modified to avoid any changes to Cargo.lock)
@sigmaris sigmaris force-pushed the sigmaris-reenable-kafka-ssl branch from 237ba25 to 7a3220f Compare January 11, 2022 16:20
pavdmyt
pavdmyt approved these changes Jan 12, 2022
View reviewed changes

@pavdmyt pavdmyt left a comment •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; seemingly I don't have enough permissions to merge this PR

@sigmaris

sigmaris commented Jan 12, 2022

Copy link
Copy Markdown
Member Author

LGTM; seemingly I don't have enough permissions to merge this PR

Sorry - added the Aiven Developers team with write access, it should be possible now.

@pavdmyt pavdmyt merged commit 26da3b5 into master Jan 12, 2022
@pavdmyt pavdmyt deleted the sigmaris-reenable-kafka-ssl branch January 12, 2022 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@pavdmyt pavdmyt pavdmyt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sigmaris @pavdmyt