mumrah commented on a change in pull request #10237:
URL: https://github.com/apache/kafka/pull/10237#discussion_r585784850
##########
File path: tests/kafkatest/services/security/security_config.py
##########
@@ -374,7 +374,8 @@ def interbroker_sasl_mechanism(self):
def enabled_sasl_mechanisms(self):
sasl_mechanisms = []
if self.is_sasl(self.security_protocol):
- sasl_mechanisms += [self.client_sasl_mechanism]
+ # .csv is supported so be sure to account for that possibility
+ sasl_mechanisms += self.client_sasl_mechanism.strip().split(',')
Review comment:
Not sure if we should bother in this PR, but the usages of
`client_sasl_mechanism` could stand to be cleaned up. In
`SecurityConfig.__init__` we default it to a simple string, but as you found
here (and as seen in `SecurityConfig.client_config`) we support it being a
comma delimited string.
https://github.com/apache/kafka/blob/58b3b1b557e9ba19cffde91bd117a89b947f1fc1/tests/kafkatest/services/security/security_config.py#L240-L253
It's probably a lot safer to declare this as a list in the class and not
worry about having to do the `split(",")` everywhere. Though maybe there's a
reason why we split it lazily.. not sure.
Either way, this change looks good. However, you might consider doing
something like:
```python
sasl_mechanisms += [mechanism.strip() for mechanism in
self.client_sasl_mechanism.split(',')]
```
since `self.client_sasl_mechanism.strip()` won't catch spaces in the middle
of the string.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
