-
Notifications
You must be signed in to change notification settings - Fork 7
AMW-162 Add support for KRaft #131
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
AMW-162 Add support for KRaft #131
Conversation
16ec91e to
b656230
Compare
a63d488 to
a9f9ab0
Compare
|
For molecule testing purpose I have changed the github workflow ci.yml branch from main to AMW-162, once the review is done I'll change it back to main again. so please ignore that. |
| {% if amq_streams_broker_listeners is defined %} | ||
| listeners={{ amq_streams_broker_listeners | join(",") }} | ||
| {% elif amq_streams_broker_listener_port is defined %} | ||
| {% elif amq_streams_broker_listener_port is defined %} |
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.
Wrong indentation
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.
Corrected.
| {% else %} | ||
| # Legacy ZK Mode Advertised Listeners | ||
| {% if amq_streams_broker_advertised_listeners is defined %} | ||
| advertised.listeners={{ amq_streams_broker_advertised_listeners | join(",") }} |
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.
Wrong indentation
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.
Corrected.
| # KRaft Mode Security Map (Must include Controller) | ||
| listener.security.protocol.map=PLAINTEXT:PLAINTEXT,CONTROLLER:PLAINTEXT,SSL:SSL,SASL_PLAINTEXT:SASL_PLAINTEXT,SASL_SSL:SASL_SSL | ||
| {% else %} | ||
| {% if amq_streams_broker_auth_enabled and amq_streams_broker_auth_listeners is defined %} |
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.
Wrong indentation
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.
Corrected.
| # The list of SASL mechanisms enabled in the Kafka server | ||
| sasl.enabled.mechanisms={{ amq_streams_broker_auth_sasl_mechanisms | join(",") }} | ||
| {% if amq_streams_broker_inter_broker_auth_sasl_mechanisms is defined %} | ||
| {% if amq_streams_broker_inter_broker_auth_sasl_mechanisms is defined %} |
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.
Wrong indentation
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.
Corrected. Please review once.
iweiss
left a comment
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.
Wrong indentation in some places
a9f9ab0 to
6ccde12
Compare
iweiss
left a comment
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.
LGTM
rmarting
left a comment
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.
Great job!!! LGTM!
hcherukuri
left a comment
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.
LGTM
Issue: https://issues.redhat.com/browse/AMW-162
GitHub issue: #124