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

Update KafkaEventBroker to support SASL_SSL and PLAINTEXT protocols #6943

Merged
merged 12 commits into from
Oct 8, 2020

Conversation

alwx
Copy link
Contributor

@alwx alwx commented Oct 7, 2020

Proposed changes:

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@alwx alwx requested a review from ricwo as a code owner October 7, 2020 09:27
@alwx alwx removed the request for review from ricwo October 7, 2020 09:29
@alwx alwx requested a review from ricwo October 7, 2020 09:56
Copy link
Contributor

@ricwo ricwo left a comment

Choose a reason for hiding this comment

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

Looks great! Requesting changes for now as the producer/consumer variable needs to be fixed

rasa/core/brokers/kafka.py Outdated Show resolved Hide resolved
rasa/core/brokers/kafka.py Show resolved Hide resolved
rasa/core/brokers/kafka.py Outdated Show resolved Hide resolved
rasa/core/brokers/kafka.py Outdated Show resolved Hide resolved
rasa/core/brokers/kafka.py Outdated Show resolved Hide resolved
rasa/core/brokers/kafka.py Outdated Show resolved Hide resolved
rasa/core/brokers/kafka.py Outdated Show resolved Hide resolved
tests/core/test_broker.py Outdated Show resolved Hide resolved
tests/core/test_broker.py Outdated Show resolved Hide resolved
# `_create_producer()` raises `kafka.errors.NoBrokersAvailable` exception
# which means that configuration seems correct but a connection to
# broker cannot be established
("kafka_sasl_plaintext_endpoint.yml", kafka.errors.NoBrokersAvailable),
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: parametrize directly over the YAML strings instead of the filenames, and get rid of the files. I think it's a bit cleaner having the content in the test, especially for small files, but really this is up to you

@alwx alwx requested a review from ricwo October 7, 2020 12:40
Copy link
Contributor

@ricwo ricwo left a comment

Choose a reason for hiding this comment

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

💯

@rasabot rasabot merged commit 985a2fb into master Oct 8, 2020
@rasabot rasabot deleted the kafka-sasl-ssl branch October 8, 2020 14:13
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.

Update Rasa Kafka event broker producer code to support SASL_SSL and PLAINTEXT protocols
3 participants