SOLR-17697: Implement picocli for auth command / AuthTool - #4684
Conversation
github-actions
Bot
added
documentation
| } | ||
|
|
||
| private void clearSecurityJson(CommandLine cli, boolean updateIncludeFileOnly) throws Exception { | ||
| private void clearSecurityJson(boolean updateIncludeFileOnly, Callable<String> zkHostResolver) |
There was a problem hiding this comment.
Does Callable<String> zkHostResolver work better than just zkHost? Just wondering if we actually need a powerful "Callable"....
There was a problem hiding this comment.
True, both call paths can resolve zkHost lazily if needed and thus simplify.
There was a problem hiding this comment.
Good instinct — the Callable wasn't needed. I've reworked it so the ZK host is resolved once per entry point (skipped with --update-include-file-only) and passed as a plain nullable String zkHost inside the AuthParams record; the existing null checks report errors exactly where they did before.
I also rewrote the branch into two commits: a pure parser-independent refactor of AuthTool (no picocli), and the picocli layer on top. The refactor commit is up as its own PR against main: #4688 — it can land there independently of this branch.
There was a problem hiding this comment.
thanks for this... In some code bases Callables are everywhere, and it makes sense and you grok it, but in ours, we don't really do that...
…methods Restructures the monolithic handleBasicAuth(cli) into an AuthParams record and cli-free handleCommand()/enableBasicAuth()/disableBasicAuth()/ clearSecurityJson() methods. The ZooKeeper connection string is resolved once in runImpl (skipped with --update-include-file-only) and passed as a plain nullable String; error reporting stays at the existing check sites. No functional change intended.
Adds the picocli annotation, option fields and callTool() to AuthTool, building the same AuthParams and calling the same handleCommand() as the commons-cli path. Includes AuthToolPicocliTest running the inherited AuthToolTest suite through picocli, and ref-guide docs for bin/solr auth.
a0bab52 to
0e3d509
Compare
janhoy
changed the title
2 participants
Adds picocli support to
AuthToolfollowing the established dual-path pattern:handleBasicAuth(CommandLine)intoenableBasicAuth/disableBasicAuthoperating on a sharedAuthParamsrecord plus a pluggable zk-host resolver, so both parsing paths run identical code.@Commandannotation with examples and exit codes;enable|disableas a positional parameter; options mirror the commons-cli definitions, with connection/credentials options provided by theZkConnectionOptionsmixin (includes the new-s/--solr-connection).AuthToolregistered as aSolrCLIsubcommand; generatedsolr-auth.adocref-guide page included.AuthToolPicocliTestruns the whole existingAuthToolTestsuite throughpicocli.CommandLine.execute().Boolean-ish options (
--block-unknownetc.) stay string-typed so both paths emit the same validation message asserted bytest_auth.bats. Connection parsing remains string-only; no I/O happens during argument parsing.Verified:
AuthToolTest+AuthToolPicocliTestgreen,test_auth.batsgreen on the commons-cli path and the picocli path (SOLR_PICOCLI=true),check -x testwith Error Prone.