Skip to content

SOLR-18521: Unify picocli connection options into one ConnectionOptions mixin - #5063

Open
janhoy wants to merge 8 commits into
apache:mainfrom
janhoy:SOLR-18521-picocli-connection-mixin
Open

janhoy wants to merge 8 commits into
apache:mainfrom
janhoy:SOLR-18521-picocli-connection-mixin

Conversation

@janhoy

@janhoy janhoy commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Description

The now-complete picocli port of bin/solr ended up with three parallel declarations of --solr-connection, --zk-host and --solr-url:

  • ConnectionOptions, an exclusive @ArgGroup used by most tools
  • ZkConnectionOptions, a @Mixin for the zk-family tools that also re-declared --credentials
  • a private clone inside StatusTool

Because picocli applies no default-value-provider defaults to an unmatched @ArgGroup, the SOLR_CONNECTION / ZK_HOST / SOLR_URL fallback never reached the ArgGroup-based tools, so every one of them hand-rolled its own URL resolution with its own environment fallback and warning text. The zk-family tools accepted --zk-host a --solr-url b while the others rejected it, and CliDefaultValueProvider keyed on paramLabel (the Java field name), so setting a paramLabel silently disabled the environment fallback.

Solution

  • ConnectionOptions is now a single public @Mixin with resolveSolrUrl, resolveZkHost, namedConnection, resolveSolrConnection and hasExplicitConnection built in. An option passed on the command line wins over environment-populated fields, and passing more than one of the three explicitly is a usage error in every tool.
  • ZkConnectionOptions and StatusTool.TargetOptions are removed. Every tool mixes in ConnectionOptions and CredentialsOptions and drops its private resolver. CollectionNameOptions is used for -c/--name wherever it is required.
  • CliDefaultValueProvider is keyed by option name instead of paramLabel. Only the mixin's --solr-url takes the SOLR_URL default, since ApiTool's own --solr-url names a full endpoint URL.
  • ToolBase lets a ParameterException raised while running reach picocli, so it is reported like any other invalid input (exit code 2 plus usage).
  • SolrCLI.rootCommandLine builds the fully configured root command for main and for tests that drive a tool through the real root.
  • PackageTargetOptions is a new mixin for --cluster/--collections and their either-or validation, previously duplicated in package deploy and package undeploy.
  • All commons-cli deprecations introduced by the picocli port now carry since = "10.2".
  • CLI reference pages regenerated.

The commons-cli path is unchanged.

Tests

  • ConnectionOptionsTest rewritten as parsing-level tests: precedence, environment-vs-explicit, mutual exclusion, non-ZK --zk-host rejection.
  • CliDefaultValueProviderTest rewritten for option-name keying, including the ApiTool opt-out.
  • All existing *PicocliTest twins pass (org.apache.solr.cli.*, 290 tests), as does integrationTestsPicocli (BATS) and gradlew check -x test.
  • Manual checks on the built distribution: ZK_HOST, SOLR_URL and SOLR_CONNECTION fallbacks now reach zk ls, create and healthcheck; --zk-host plus --solr-url exits 2; an environment ZK_HOST together with an explicit --solr-url succeeds.

🤖 Coded entirely with Claude Code

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests cat:cli labels Oct 8, 2026
@janhoy
janhoy requested a balanced review from Copilot October 8, 2026 21:41
bin/solr version [-hv] [-u=<credentials>] [-s=<solrConnection> |
--solr-url=<solrUrl> | -z=<zkHost>]
bin/solr version [-hv] [-s=<solrConnection>] [--solr-url=<solrUrl>]
[-u=<credentials>] [-z=<zkHost>]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here you can see the only visible side effect of this refactor.

Since we don't use PicoCLI's ArgGroup anymore but a @MixIn instead we do not get the grouping of related options like in

[-s=<solrConnection> | --solr-url=<solrUrl> | -z=<zkHost>]

Instead all options are listed individually, and we enforce mutual exclusitivity in the mixin code.

[--solr-url=<solrUrl>] [-u=<credentials>] [-z=<zkHost>]

Personally I don't think that bother anyone. Not many normal CLi tools use the [ foo | bar ] syntax, it's kind of an advanced feature.

* --solr-url b} is an error.
*/
class ConnectionOptions {
public class ConnectionOptions {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is perhaps the most important class in this PR. Read the class javadoc and understand what it does.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[pending-on-me] to review the changes inside.

@janhoy
janhoy requested a review from epugh October 8, 2026 21:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

3 open findings
What changed in this PR

Unifies Solr CLI picocli connection targeting (--solr-connection, --zk-host, --solr-url) into a single reusable mixin, fixes environment defaulting behavior, and updates tools/docs/tests to use the new shared resolution and validation logic.

Changes:

  • Replace multiple per-tool connection option declarations/resolvers with a shared ConnectionOptions mixin (and add PackageTargetOptions to dedupe deploy/undeploy targeting).
  • Fix CliDefaultValueProvider to key defaults by option name and avoid incorrectly defaulting tool-specific --solr-url options.
  • Regenerate/refactor CLI docs and tests to reflect new parsing/precedence/exclusivity behavior.
File Description
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-zk-updateacls.adoc Updates CLI option text for --solr-url description formatting.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-zk-upconfig.adoc Updates CLI option text for --solr-url description formatting.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-zk-rm.adoc Updates CLI option text for --solr-url description formatting.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-zk-mv.adoc Updates CLI option text for --solr-url description formatting.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-zk-mkroot.adoc Updates CLI option text for --solr-url description formatting.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-zk-ls.adoc Updates CLI option text for --solr-url description formatting.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-zk-downconfig.adoc Updates CLI option text for --solr-url description formatting.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-zk-cp.adoc Updates CLI option text for --solr-url description formatting.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-version.adoc Updates synopsis output after switching to mixin-based connection options.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-stream.adoc Updates synopsis output after switching to mixin-based connection options.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-status.adoc Updates synopsis/options formatting; aligns --port label and --solr-url description.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-snapshot-list.adoc Updates synopsis and -c/--name description to “collection or core”.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-snapshot-export.adoc Updates synopsis and -c/--name description to “collection or core”.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-snapshot-describe.adoc Updates synopsis and -c/--name description to “collection or core”.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-snapshot-delete.adoc Updates synopsis and -c/--name description to “collection or core”.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-snapshot-create.adoc Updates synopsis and -c/--name description to “collection or core”.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-postlogs.adoc Updates synopsis after switching to mixin-based options and shared -c/--name.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-post.adoc Updates synopsis and -c/--name label/description via CollectionNameOptions.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-package.adoc Updates synopsis formatting for shared connection options.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-package-uninstall.adoc Updates synopsis formatting for shared connection options.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-package-undeploy.adoc Updates synopsis formatting for shared connection options.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-package-list-installed.adoc Updates synopsis formatting for shared connection options.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-package-list-deployed.adoc Updates synopsis formatting for shared connection options.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-package-list-available.adoc Updates synopsis formatting for shared connection options.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-package-install.adoc Updates synopsis formatting for shared connection options.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-package-deploy.adoc Updates synopsis formatting for shared connection options.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-package-add-repo.adoc Updates synopsis formatting for shared connection options.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-package-add-key.adoc Updates synopsis formatting for shared connection options.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-healthcheck.adoc Updates synopsis and -c/--name label/description via CollectionNameOptions.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-export.adoc Updates synopsis and -c/--name description to “collection or core”.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-delete.adoc Updates synopsis and -c/--name description via CollectionNameOptions.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-create.adoc Updates synopsis/options to align with new mixins & naming.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-config.adoc Updates synopsis/options to align with new mixins & naming.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-cluster.adoc Updates --solr-url description formatting for zk resolution.
solr/​solr-ref-guide/​modules/​deployment-guide/​pages/​cli/​solr-auth.adoc Updates --solr-url description formatting for zk resolution.
solr/​core/​src/​test/​org/​apache/​solr/​cli/​SnapshotToolsPicocliTest.java Uses the new fully-configured SolrCLI.rootCommandLine(...) for tests.
solr/​core/​src/​test/​org/​apache/​solr/​cli/​ConnectionOptionsTest.java Rewrites tests to target parsing/precedence/exclusivity without spinning up a cluster.
solr/​core/​src/​test/​org/​apache/​solr/​cli/​CliDefaultValueProviderTest.java Updates tests for option-name keying and --solr-url opt-out behavior.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ZkRmTool.java Switches from ZkConnectionOptions to ConnectionOptions + CredentialsOptions.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ZkMvTool.java Switches from ZkConnectionOptions to ConnectionOptions + CredentialsOptions.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ZkMkrootTool.java Switches from ZkConnectionOptions to ConnectionOptions + CredentialsOptions.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ZkLsTool.java Switches from ZkConnectionOptions to ConnectionOptions + CredentialsOptions.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ZkCpTool.java Switches from ZkConnectionOptions to ConnectionOptions + CredentialsOptions.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ZkConnectionOptions.java Removes obsolete ZK-specific mixin now replaced by shared connection options.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​VersionTool.java Converts ArgGroup to mixin and uses new explicit-connection semantics.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​UpdateACLTool.java Switches from ZkConnectionOptions to ConnectionOptions + CredentialsOptions.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ToolBase.java Adds since="10.2" deprecations and rethrows ParameterException for picocli usage handling.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​Tool.java Adds since="10.2" to commons-cli-only deprecations.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​StreamTool.java Uses ConnectionOptions mixin and removes duplicated resolver logic.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​StatusTool.java Replaces private target ArgGroup with mixins and adds runtime exclusivity check vs --port.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​SolrCLI.java Adds rootCommandLine(...) builder and updates commons-cli deprecation annotations.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​SnapshotListTool.java Switches to ConnectionOptions.resolveSolrUrl(...) and mixin usage.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​SnapshotExportTool.java Switches to ConnectionOptions.resolveSolrUrl(...) and mixin usage.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​SnapshotDescribeTool.java Switches to ConnectionOptions.resolveSolrUrl(...) and mixin usage.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​SnapshotDeleteTool.java Switches to ConnectionOptions.resolveSolrUrl(...) and mixin usage.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​SnapshotCreateTool.java Switches to ConnectionOptions.resolveSolrUrl(...) and mixin usage.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​RunExampleTool.java Updates deprecations and ensures port option defaulting unaffected by label changes.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​PostTool.java Uses shared CollectionNameOptions and shared connection resolution.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​PostLogsTool.java Uses shared CollectionNameOptions and new connection mixin (adds runtime “missing target” check).
solr/​core/​src/​java/​org/​apache/​solr/​cli/​PackageUndeploy.java Deduplicates --cluster/--collections handling via new mixin.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​PackageTool.java Uses ConnectionOptions mixin and updated zk resolution error mapping.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​PackageTargetOptions.java Introduces reusable package deploy/undeploy targeting mixin.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​PackageSubCommand.java Adjusts inheritance/precedence rules for parent vs subcommand connection options.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​PackageDeploy.java Deduplicates --cluster/--collections handling via new mixin.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​HealthcheckTool.java Uses shared CollectionNameOptions and ConnectionOptions.resolveSolrConnection(...).
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ExportTool.java Uses shared CollectionNameOptions and new connection mixin (adds runtime “missing target” check).
solr/​core/​src/​java/​org/​apache/​solr/​cli/​DeleteTool.java Uses shared CollectionNameOptions and ConnectionOptions.resolveSolrUrl(...).
solr/​core/​src/​java/​org/​apache/​solr/​cli/​CreateTool.java Uses ConnectionOptions + CollectionNameOptions and unified URL/ZK resolution.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ConnectionOptions.java Makes connection options a public mixin with unified resolution + explicit-option exclusivity.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ConfigTool.java Uses shared CollectionNameOptions and ConnectionOptions.resolveSolrUrl(...).
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ConfigSetUploadTool.java Switches from ZkConnectionOptions to ConnectionOptions + CredentialsOptions.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ConfigSetDownloadTool.java Switches from ZkConnectionOptions to ConnectionOptions + CredentialsOptions.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​CollectionNameOptions.java Makes collection name option a public mixin and clarifies description.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ClusterTool.java Switches from ZkConnectionOptions to ConnectionOptions + CredentialsOptions.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​CliDefaultValueProvider.java Keys defaults by option long name and limits --solr-url defaulting to shared mixin only.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​CLIUtils.java Updates deprecation annotations and removes obsolete picocli URL resolver helper.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​AuthTool.java Switches from ZkConnectionOptions to ConnectionOptions + CredentialsOptions.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​AssertTool.java Adds since="10.2" to commons-cli-only deprecated options.
solr/​core/​src/​java/​org/​apache/​solr/​cli/​ApiTool.java Adds since="10.2" to commons-cli-only deprecated option.
changelog/​unreleased/​SOLR-18521-picocli-connection-mixin.yml Adds changelog entry for unified connection options/precedence/exclusivity behavior.

🧠 Review effort: Lite


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@jaykay12 jaykay12 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[pending-on-me-to-review] 1 code file & 2 test files.

@janhoy lets rebase this branch with latest main as well.

// Only the shared option means a base URL; a tool's own --solr-url (ApiTool's full endpoint
// URL) must not be filled from SOLR_URL
case "--solr-url" ->
declaredIn(option, ConnectionOptions.class) ? EnvUtils.getProperty("solr.url") : null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Q] why do we need this? separate utility function of declaredIn() ?
It was not earlier required.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is needed because the provider is now keyed by the option's long name, and --solr-url is not unique: this mixin's is a base URL, while ApiTool declares its own required = true -s/--solr-url meaning a full endpoint URL — without the guard a SOLR_URL in solr.in.sh would satisfy that required and bin/solr api with no arguments would silently GET the base URL. It wasn't needed before only because the provider keyed on paramLabel, and ApiTool happens to set one while the shared option didn't — which is the very bug this PR fixes, since under that keying adding a paramLabel to any option silently switched its environment fallback off. declaredIn() isn't extra reflection on our side either: OptionSpec.userObject() is picocli's own model object for an annotated field.

Comment on lines -28 to -29
case "<solrConnection>" -> EnvUtils.getProperty("solr.connection");
case "<solrUrl>" -> EnvUtils.getProperty("solr.url");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Q] purely for my knowledge, can you help me with example commands on how --solr-connection & --solr-url differs? & how they both serve different purpose for us.

Pls take in consideration of any of the package tool subcommand, so that i am able to corelate better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

--solr-url is a single Solr base URL and --zk-host is a ZooKeeper quorum. --solr-connection is the universal string parsed by CloudSolrClient.CloudSolrClientConnection.parse() — contains :// means a comma-separated list of Solr base URLs, anything else is a ZK quorum with optional chroot — so it is a superset, and the only one of the three that can name more than one host:

# ZK quorum, same as --zk-host
bin/solr package deploy mypkg:1.0 --collections c1 -s zk1:2181,zk2:2181/solr

# HTTP quorum of two nodes, which --solr-url cannot express
bin/solr package deploy mypkg:1.0 --collections c1 -s http://node1:8983/solr,http://node2:8983/solr

For the package tool all three end up in the same place, since runWithManagers() needs both a URL and a ZK host and resolves whichever is missing. The env vars map one-to-one (SOLR_CONNECTION, ZK_HOST, SOLR_URL), and the point of this PR is that all three now reach every tool.

Comment on lines -167 to -187
String zkHostArg =
(connectionOptions != null)
? connectionOptions.effectiveZkHost()
: EnvUtils.getProperty("zkHost");
String solrUrlArg = (connectionOptions != null) ? connectionOptions.effectiveSolrUrl() : null;
String credentials = (credentialsOptions != null) ? credentialsOptions.credentials : null;

String resolvedSolrUrl;
if (solrUrlArg != null) {
resolvedSolrUrl = CLIUtils.normalizeSolrUrl(solrUrlArg);
} else if (zkHostArg != null) {
resolvedSolrUrl =
CLIUtils.solrUrlFromConnection(
CloudSolrClient.CloudSolrClientConnection.parse(zkHostArg), credentials);
} else {
resolvedSolrUrl = CLIUtils.getDefaultSolrUrl();
CLIO.err(
"Neither --zk-host or --solr-url parameters, nor ZK_HOST env var provided, so assuming solr url is "
+ resolvedSolrUrl
+ ".");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

impact of this PR can be sensed by this refactoring, so many lines of code being getting replaced with a shared utility function, kudos 🚀

[pending-on-me] to do local testing of this particular tool between main branch & this branch.

Comment on lines +75 to +79
@picocli.CommandLine.Option(
names = {"-p", "--port"},
paramLabel = "PORT",
description = "Port on localhost to check status for")
private Integer portOpt;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Q] any specific reason, why we don't keep port under ConnectionOptions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

--port is not a connection target but a way of addressing a Solr process on this machine: status --port 8983 goes through SolrProcessManager.processForPort() and inspects local processes, and with no port and no connection option it scans them all rather than contacting a URL. It is also not shared the way the connection options are — start, stop and run example declare -p/--port as "the port to bind the listener to" and mix in no ConnectionOptions at all, and -p is already --param in package deploy. What status does need is --port being mutually exclusive with the connection options, and that is wired in StatusTool.callTool() reusing explicitOptionNames()/mutuallyExclusiveMessage() so the error text matches the other tools.


public class ConnectionOptionsTest extends SolrCloudTestCase {
/** Parsing-level tests of {@link ConnectionOptions}; nothing here contacts Solr or ZooKeeper. */
public class ConnectionOptionsTest extends SolrTestCase {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[pending-on-me] to review these tests.


public class CliDefaultValueProviderTest extends SolrTestCase {

@CommandLine.Command(name = "shared")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[pending-on-me] to review these tests.

* --solr-url b} is an error.
*/
class ConnectionOptions {
public class ConnectionOptions {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[pending-on-me] to review the changes inside.

…ns mixin

The picocli path had three parallel declarations of --solr-connection,
--zk-host and --solr-url (ConnectionOptions as an exclusive ArgGroup,
ZkConnectionOptions as a mixin that also re-declared --credentials, and a
private clone in StatusTool), and because picocli applies no default-value
provider defaults to an unmatched ArgGroup, every ArgGroup-based tool
hand-rolled its own URL resolution with its own environment fallback and
warning text.

ConnectionOptions is now a single public mixin with resolveSolrUrl,
resolveZkHost, namedConnection, resolveSolrConnection and
hasExplicitConnection built in. An option passed on the command line wins
over the environment-populated fields, and passing more than one of the
three explicitly is a usage error for every tool. ZkConnectionOptions and
StatusTool.TargetOptions are gone; every tool mixes in ConnectionOptions and
CredentialsOptions and drops its private resolver. CollectionNameOptions is
used for -c/--name wherever it is required.

CliDefaultValueProvider is keyed by option name instead of paramLabel, so
setting a paramLabel no longer disables the environment fallback. Only the
mixin's --solr-url takes the SOLR_URL default, since ApiTool's own --solr-url
names a full endpoint. ToolBase lets a ParameterException raised while
running reach picocli so it is reported like any other invalid input.
SolrCLI.rootCommandLine builds the configured root for main and for tests
that drive a tool through the real root.
…undeploy

PackageDeploy and PackageUndeploy declared the same two options and the same
either-or validation; PackageTargetOptions is now the mixin for both.
The picocli port deprecated the commons-cli option constants, Tool/ToolBase
methods and SolrCLI helpers in 10.2, so every one of them carries that
release; the one that said 10.1 was wrong.
Error Prone (CI only) rejects the fully qualified name since the class
already imports picocli.CommandLine.
The sweep ships with the picocli CLI feature, so it is linked from that
entry instead of having its own.
Review feedback: ExportTool and PostLogsTool threw IllegalArgumentException,
which ToolBase turns into a plain exit 1 with no usage. ConnectionOptions
now raises the ParameterException itself via its mixee spec, so these tools
exit 2 with usage like every other invalid input. Also reattach a stray
Javadoc block to propagateCommandSettings.
Rename the PackageTargetOptions field to targetOptions in PackageDeploy
and PackageUndeploy, and the ConnectionOptions parameter of
PackageTool.runWithManagers() to connectionOptions, matching how the
other option mixins are named throughout the CLI.
The either-or validation shared by package deploy and undeploy had no
direct coverage: PackageToolPicocliTest only re-runs PackageToolTest
through picocli, and testDeployValidationMessages always passes
--collections, so the case where neither target is given was never
exercised.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cat:cli documentation Improvements or additions to documentation tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants