You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "mimaison (via GitHub)" <gi...@apache.org> on 2024/11/20 16:53:44 UTC
[PR] KAFKA-14587: Move AclCommand to tools [kafka]
mimaison opened a new pull request, #17880:
URL: https://github.com/apache/kafka/pull/17880
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14587: Move AclCommand to tools [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #17880:
URL: https://github.com/apache/kafka/pull/17880#issuecomment-2513976084
@chia7712 @showuon Can you take a look if you have time? Thanks
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14587: Move AclCommand to tools [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #17880:
URL: https://github.com/apache/kafka/pull/17880#discussion_r1850681333
##########
tools/src/test/java/org/apache/kafka/tools/AclCommandTest.java:
##########
@@ -303,7 +294,7 @@ public void testUseBootstrapServerOptWithBootstrapControllerOpt() {
@Test
public void testUseWithoutBootstrapServerOptAndBootstrapControllerOpt() {
assertInitializeInvalidOptionsExitCodeAndMsg(
- Collections.emptyList(),
+ Collections.singletonList(ADD),
Review Comment:
Actually if you don't provide any arguments on the CLI, just the help message is printed.
The previous test was expecting a different behavior as `maybePrintHelpOrVersion()` used to be called in `main()` while I moved it to `AclCommandOptions.checkArgs()` for consistency with the other tools, and consistency with the CLI behavior.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14587: Move AclCommand to tools [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #17880:
URL: https://github.com/apache/kafka/pull/17880#discussion_r1850686209
##########
tools/src/test/java/org/apache/kafka/tools/AclCommandTest.java:
##########
@@ -355,7 +346,7 @@ public void testInvalidArgs() {
"Option \"[list]\" can't be used with option \"[producer]\""
);
assertInitializeInvalidOptionsExitCodeAndMsg(
- Arrays.asList(BOOTSTRAP_SERVER, LOCALHOST, ADD, PRODUCER, OPERATION),
+ Arrays.asList(BOOTSTRAP_SERVER, LOCALHOST, ADD, PRODUCER, OPERATION, "all"),
Review Comment:
Running the same command on the CLI did not produce that error message, so this test was effectively testing internal logic that would not be reachable for users.
```
./bin/kafka-acls.sh --bootstrap-server localhost:9092 --add --producer --operation
Exception in thread "main" joptsimple.OptionMissingRequiredArgumentException: Option operation requires an argument
```
To produce that error message from the CLI you need to specify an operation.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14587: Move AclCommand to tools [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison merged PR #17880:
URL: https://github.com/apache/kafka/pull/17880
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-14587: Move AclCommand to tools [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #17880:
URL: https://github.com/apache/kafka/pull/17880#issuecomment-2542660696
@mimaison could you please fix conflicts?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org