janhoy commented on code in PR #4739:
URL: https://github.com/apache/solr/pull/4739#discussion_r3835772433
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -381,6 +492,64 @@ public Options getOptions() {
@Override
public int callTool() throws Exception {
- throw new UnsupportedOperationException("This tool does not yet support
PicoCli");
+ String credentials = credentialsOptions.credentials;
+ String solrUrl = resolveSolrUrl(credentials);
+ String zkHost = resolveZkHost(solrUrl, credentials);
+ String[] args = cmdArgs == null ? new String[0] : cmdArgs;
+ PackageFlags packageFlags =
+ new PackageFlags(collections, cluster, param, update, collection,
noPrompt);
+ executePackage(solrUrl, zkHost, credentials, cmd, args, packageFlags);
+ return 0;
Review Comment:
`callTool()` returns 0 unconditionally, so the exit codes advertised in
`@Command(exitCodeList = ...)` never actually occur: an `install` that prints
`printRed(pkg + " installation failed.")` still exits 0, as does a `deploy`
rejected for missing `--cluster`/`--collections`. Only a thrown exception
yields 1, via `ToolBase.call`.
That makes `bin/solr package install foo; echo $?` misleading in scripts.
Either return 1 on those failure paths (per subcommand, once they're split out)
or drop the exit-code-1 entry from the annotation.
##########
solr/solr-ref-guide/modules/deployment-guide/pages/cli/solr-package.adoc:
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+// DO NOT EDIT -- this page is auto-generated from picocli annotations.
+// To update: modify the @Command/@Option annotations in the Java source, then
run:
+// ./gradlew :solr:solr-ref-guide:generateCliDocs
+
+= bin/solr package
+:page-toclevels: 2
+
+// tag::picocli-generated-man-section-name[]
+== Name
+
+bin/solr package - Install, deploy and manage Solr packages in SolrCloud.
+
+// end::picocli-generated-man-section-name[]
+
+// tag::picocli-generated-man-section-synopsis[]
+== Synopsis
+
+....
+bin/solr package [-vy] [--cluster] [--update] [-c=COLLECTION]
+ [--collections=COLLECTIONS] [-u=<credentials>]
[--param=PARAMS]...
+ [-s=<solrConnection> | --solr-url=<solrUrl> | -z=<zkHost>]
COMMAND
+ [ARGS...]
+....
+
+// end::picocli-generated-man-section-synopsis[]
+
+// tag::picocli-generated-man-section-description[]
+== Description
+
+Install, deploy and manage Solr packages in SolrCloud.
+
+// end::picocli-generated-man-section-description[]
+
+// tag::picocli-generated-man-section-options[]
+== Options
+
+*-c*, *--collection*=_COLLECTION_::
+ The collection to apply the package to.
+
+*--cluster*::
+ Specifies that this action should affect cluster-level plugins only.
+
+*--collections*=_COLLECTIONS_::
+ Specifies that this action should affect plugins for the given collections
only, excluding cluster level plugins.
+
+*--param*=_PARAMS_::
+ List of parameters to be used with the deploy command.
+
+*-s*, *--solr-connection*=_<solrConnection>_::
+ Zookeeper or HTTP(s) connection string; unnecessary if SOLR_CONNECTION is
defined in solr.in.sh; otherwise, defaults to localhost:9983.
+
+*--solr-url*=_<solrUrl>_::
+ Base Solr URL, which can be used to determine the zk-host if that's not
known.
+
+*-u*, *--credentials*=_<credentials>_::
+ Credentials in the format username:password. Example: --credentials
solr:SolrRocks
+
+*--update*::
+ If a deployment is an update over a previous deployment.
+
+*-v*, *--verbose*::
+ Enable verbose mode.
+
+*-y*, *--no-prompt*::
+ Don't prompt for input; accept all default choices, defaults to false.
+
+*-z*, *--zk-host*=_<zkHost>_::
+ Zookeeper connection string; unnecessary if ZK_HOST is defined in
solr.in.sh; otherwise, defaults to localhost:9983.
+
+// end::picocli-generated-man-section-options[]
+
+// tag::picocli-generated-man-section-arguments[]
+== Arguments
+
+_COMMAND_::
+ Package command: add-repo, add-key, list-installed, list-available,
list-deployed, install, deploy, undeploy, uninstall.
+
+[_ARGS_...]::
+ Command-specific arguments (package name[:version], repository name/URL, key
file, etc.).
+
+// end::picocli-generated-man-section-arguments[]
+
+// tag::picocli-generated-man-section-commands[]
+// end::picocli-generated-man-section-commands[]
Review Comment:
The `== Commands` section is generated empty, because `package` exposes its
nine commands as a free-text positional rather than as picocli subcommands (see
my comment on the `COMMAND` positional in `PackageTool.java`).
That means the ref-guide page documents the entire package CLI as one option
list plus a single prose sentence listing command names — no per-command
synopsis, no indication of which options apply to which command. Once the
commands become real subcommands, this section populates itself and each
command also gets its own generated page and nav entry, matching how `zk` is
documented today.
##########
solr/core/src/test/org/apache/solr/cli/PackageToolPicocliTest.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.cli;
+
+import picocli.CommandLine;
+
+/**
+ * Runs all {@link PackageToolTest} tests through the picocli invocation path.
+ */
Review Comment:
`./gradlew tidy` hasn't been run — `./gradlew :solr:core:spotlessJavaCheck`
currently fails on this file and on `PackageToolTest.java`, so `./gradlew
check` will fail too.
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -113,185 +206,203 @@ public String getName() {
+ "don't print stack traces, hence special treatment is needed
here."
+ "Need to turn off logging, and SLF4J doesn't seem to provide
for a way.")
public void runImpl(CommandLine cli) throws Exception {
+ String solrUrl = CLIUtils.normalizeSolrUrl(cli);
+ String zkHost = CLIUtils.getZkHost(cli);
+ String credentials =
cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION);
+ String command = cli.getArgs()[0];
+ String[] cmdArgs = Arrays.copyOfRange(cli.getArgs(), 1,
cli.getArgs().length);
+ PackageFlags packageFlags =
+ new PackageFlags(
+ cli.getOptionValue(COLLECTIONS_OPTION),
+ cli.hasOption(CLUSTER_OPTION),
+ cli.getOptionValues(PARAM_OPTION),
+ cli.hasOption(UPDATE_OPTION),
+ cli.getOptionValue(COLLECTION_OPTION),
+ cli.hasOption(NO_PROMPT_OPTION));
+
+ executePackage(solrUrl, zkHost, credentials, command, cmdArgs,
packageFlags);
+ }
+
+ private void executePackage(
+ String solrUrl,
+ String zkHost,
+ String credentials,
+ String command,
+ String[] cmdArgs,
+ PackageFlags packageFlags)
+ throws Exception {
// Need a logging free, clean output going through to the user.
Level oldLevel =
LoggerContext.getContext(false).getRootLogger().getLevel();
Configurator.setRootLevel(Level.OFF);
try {
- String solrUrl = CLIUtils.normalizeSolrUrl(cli);
- String zkHost = CLIUtils.getZkHost(cli);
if (zkHost == null) {
throw new SolrException(ErrorCode.INVALID_STATE, "Package manager runs
only in SolrCloud");
}
log.info("ZK: {}", zkHost);
- String cmd = cli.getArgs()[0];
-
- try (SolrClient solrClient = CLIUtils.getSolrClient(cli, true)) {
+ try (SolrClient solrClient = CLIUtils.getSolrClient(solrUrl,
credentials, true)) {
packageManager = new PackageManager(runtime, solrClient, solrUrl,
zkHost);
try {
repositoryManager = new RepositoryManager(solrClient,
packageManager);
-
- switch (cmd) {
- case "add-repo":
- String repoName = cli.getArgs()[1];
- String repoUrl = cli.getArgs()[2];
- repositoryManager.addRepository(repoName, repoUrl);
- printGreen("Added repository: " + repoName);
- break;
- case "add-key":
- String keyFilename = cli.getArgs()[1];
- Path path = Path.of(keyFilename);
- repositoryManager.addKey(Files.readAllBytes(path),
path.getFileName().toString());
- break;
- case "list-installed":
- printGreen("Installed packages:\n-----");
- for (SolrPackageInstance pkg :
packageManager.fetchInstalledPackageInstances()) {
- printGreen(pkg);
- }
- break;
- case "list-available":
- printGreen("Available packages:\n-----");
- for (SolrPackage pkg : repositoryManager.getPackages()) {
- printGreen(pkg.name + " \t\t" + pkg.description);
- for (SolrPackageRelease version : pkg.versions) {
- printGreen("\tVersion: " + version.version);
- }
- }
- break;
- case "list-deployed":
- if (cli.hasOption(COLLECTION_OPTION)) {
- String collection = cli.getOptionValue(COLLECTION_OPTION);
- Map<String, SolrPackageInstance> packages =
- packageManager.getPackagesDeployed(collection);
- printGreen("Packages deployed on " + collection + ":");
- for (String packageName : packages.keySet()) {
- printGreen("\t" + packages.get(packageName));
- }
- } else {
- // nuance that we use an arg here instead of requiring a
--package parameter with a
- // value
- // in this code path
- String packageName = cli.getArgs()[1];
- Map<String, String> deployedCollections =
- packageManager.getDeployedCollections(packageName);
- if (!deployedCollections.isEmpty()) {
- printGreen("Collections on which package " + packageName + "
was deployed:");
- for (String collection : deployedCollections.keySet()) {
- printGreen(
- "\t"
- + collection
- + "("
- + packageName
- + ":"
- + deployedCollections.get(collection)
- + ")");
- }
- } else {
- printGreen("Package " + packageName + " not deployed on any
collection.");
- }
- }
- break;
- case "install":
- {
- Pair<String, String> parsedVersion =
parsePackageVersion(cli.getArgList().get(1));
- String packageName = parsedVersion.first();
- String version = parsedVersion.second();
- boolean success = repositoryManager.install(packageName,
version);
- if (success) {
- printGreen(packageName + " installed.");
- } else {
- printRed(packageName + " installation failed.");
- }
- break;
- }
- case "deploy":
- {
- if (cli.hasOption(CLUSTER_OPTION) ||
cli.hasOption(COLLECTIONS_OPTION)) {
- Pair<String, String> parsedVersion =
parsePackageVersion(cli.getArgList().get(1));
- String packageName = parsedVersion.first();
- String version = parsedVersion.second();
- boolean noPrompt = cli.hasOption(NO_PROMPT_OPTION);
- boolean isUpdate = cli.hasOption(UPDATE_OPTION);
- String[] collections =
- cli.hasOption(COLLECTIONS_OPTION)
- ? PackageUtils.validateCollections(
-
cli.getOptionValue(COLLECTIONS_OPTION).split(","))
- : new String[] {};
- String[] parameters = cli.getOptionValues(PARAM_OPTION);
- packageManager.deploy(
- packageName,
- version,
- collections,
- cli.hasOption(CLUSTER_OPTION),
- parameters,
- isUpdate,
- noPrompt);
- } else {
- printRed(
- "Either specify --cluster to deploy cluster level
plugins or --collections <list-of-collections> to deploy collection level
plugins");
- }
- break;
- }
- case "undeploy":
- {
- if (cli.hasOption(CLUSTER_OPTION) ||
cli.hasOption(COLLECTIONS_OPTION)) {
- Pair<String, String> parsedVersion =
parsePackageVersion(cli.getArgList().get(1));
- if (parsedVersion.second() != null) {
- throw new SolrException(
- ErrorCode.BAD_REQUEST,
- "Only package name expected, without a version.
Actual: "
- + cli.getArgList().get(1));
- }
- String packageName = parsedVersion.first();
- String[] collections =
- cli.hasOption(COLLECTIONS_OPTION)
- ? PackageUtils.validateCollections(
-
cli.getOptionValue(COLLECTIONS_OPTION).split(","))
- : new String[] {};
- packageManager.undeploy(packageName, collections,
cli.hasOption(CLUSTER_OPTION));
- } else {
- printRed(
- "Either specify --cluster to undeploy cluster level
plugins or -collections <list-of-collections> to undeploy collection level
plugins");
- }
- break;
- }
- case "uninstall":
- {
- Pair<String, String> parsedVersion =
parsePackageVersion(cli.getArgList().get(1));
- if (parsedVersion.second() == null) {
- throw new SolrException(
- ErrorCode.BAD_REQUEST,
- "Package name and version are both required. Actual: "
- + cli.getArgList().get(1));
- }
- String packageName = parsedVersion.first();
- String version = parsedVersion.second();
- packageManager.uninstall(packageName, version);
- break;
- }
- default:
- throw new RuntimeException("Unrecognized command: " + cmd);
- }
+ handleCommand(command, cmdArgs, packageFlags);
} finally {
packageManager.close();
}
}
log.info("Finished: {}", cmd);
Review Comment:
This logs the `cmd` **field**, which is only populated on the picocli path,
rather than the `command` parameter that `executePackage` was given. On the
commons-cli path it prints `Finished: null`. Should be `command`.
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -113,185 +206,203 @@ public String getName() {
+ "don't print stack traces, hence special treatment is needed
here."
+ "Need to turn off logging, and SLF4J doesn't seem to provide
for a way.")
public void runImpl(CommandLine cli) throws Exception {
+ String solrUrl = CLIUtils.normalizeSolrUrl(cli);
+ String zkHost = CLIUtils.getZkHost(cli);
+ String credentials =
cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION);
+ String command = cli.getArgs()[0];
+ String[] cmdArgs = Arrays.copyOfRange(cli.getArgs(), 1,
cli.getArgs().length);
+ PackageFlags packageFlags =
+ new PackageFlags(
+ cli.getOptionValue(COLLECTIONS_OPTION),
+ cli.hasOption(CLUSTER_OPTION),
+ cli.getOptionValues(PARAM_OPTION),
+ cli.hasOption(UPDATE_OPTION),
+ cli.getOptionValue(COLLECTION_OPTION),
+ cli.hasOption(NO_PROMPT_OPTION));
+
+ executePackage(solrUrl, zkHost, credentials, command, cmdArgs,
packageFlags);
+ }
+
+ private void executePackage(
+ String solrUrl,
+ String zkHost,
+ String credentials,
+ String command,
+ String[] cmdArgs,
+ PackageFlags packageFlags)
+ throws Exception {
// Need a logging free, clean output going through to the user.
Level oldLevel =
LoggerContext.getContext(false).getRootLogger().getLevel();
Configurator.setRootLevel(Level.OFF);
try {
- String solrUrl = CLIUtils.normalizeSolrUrl(cli);
- String zkHost = CLIUtils.getZkHost(cli);
if (zkHost == null) {
throw new SolrException(ErrorCode.INVALID_STATE, "Package manager runs
only in SolrCloud");
}
log.info("ZK: {}", zkHost);
- String cmd = cli.getArgs()[0];
-
- try (SolrClient solrClient = CLIUtils.getSolrClient(cli, true)) {
+ try (SolrClient solrClient = CLIUtils.getSolrClient(solrUrl,
credentials, true)) {
packageManager = new PackageManager(runtime, solrClient, solrUrl,
zkHost);
try {
repositoryManager = new RepositoryManager(solrClient,
packageManager);
-
- switch (cmd) {
- case "add-repo":
- String repoName = cli.getArgs()[1];
- String repoUrl = cli.getArgs()[2];
- repositoryManager.addRepository(repoName, repoUrl);
- printGreen("Added repository: " + repoName);
- break;
- case "add-key":
- String keyFilename = cli.getArgs()[1];
- Path path = Path.of(keyFilename);
- repositoryManager.addKey(Files.readAllBytes(path),
path.getFileName().toString());
- break;
- case "list-installed":
- printGreen("Installed packages:\n-----");
- for (SolrPackageInstance pkg :
packageManager.fetchInstalledPackageInstances()) {
- printGreen(pkg);
- }
- break;
- case "list-available":
- printGreen("Available packages:\n-----");
- for (SolrPackage pkg : repositoryManager.getPackages()) {
- printGreen(pkg.name + " \t\t" + pkg.description);
- for (SolrPackageRelease version : pkg.versions) {
- printGreen("\tVersion: " + version.version);
- }
- }
- break;
- case "list-deployed":
- if (cli.hasOption(COLLECTION_OPTION)) {
- String collection = cli.getOptionValue(COLLECTION_OPTION);
- Map<String, SolrPackageInstance> packages =
- packageManager.getPackagesDeployed(collection);
- printGreen("Packages deployed on " + collection + ":");
- for (String packageName : packages.keySet()) {
- printGreen("\t" + packages.get(packageName));
- }
- } else {
- // nuance that we use an arg here instead of requiring a
--package parameter with a
- // value
- // in this code path
- String packageName = cli.getArgs()[1];
- Map<String, String> deployedCollections =
- packageManager.getDeployedCollections(packageName);
- if (!deployedCollections.isEmpty()) {
- printGreen("Collections on which package " + packageName + "
was deployed:");
- for (String collection : deployedCollections.keySet()) {
- printGreen(
- "\t"
- + collection
- + "("
- + packageName
- + ":"
- + deployedCollections.get(collection)
- + ")");
- }
- } else {
- printGreen("Package " + packageName + " not deployed on any
collection.");
- }
- }
- break;
- case "install":
- {
- Pair<String, String> parsedVersion =
parsePackageVersion(cli.getArgList().get(1));
- String packageName = parsedVersion.first();
- String version = parsedVersion.second();
- boolean success = repositoryManager.install(packageName,
version);
- if (success) {
- printGreen(packageName + " installed.");
- } else {
- printRed(packageName + " installation failed.");
- }
- break;
- }
- case "deploy":
- {
- if (cli.hasOption(CLUSTER_OPTION) ||
cli.hasOption(COLLECTIONS_OPTION)) {
- Pair<String, String> parsedVersion =
parsePackageVersion(cli.getArgList().get(1));
- String packageName = parsedVersion.first();
- String version = parsedVersion.second();
- boolean noPrompt = cli.hasOption(NO_PROMPT_OPTION);
- boolean isUpdate = cli.hasOption(UPDATE_OPTION);
- String[] collections =
- cli.hasOption(COLLECTIONS_OPTION)
- ? PackageUtils.validateCollections(
-
cli.getOptionValue(COLLECTIONS_OPTION).split(","))
- : new String[] {};
- String[] parameters = cli.getOptionValues(PARAM_OPTION);
- packageManager.deploy(
- packageName,
- version,
- collections,
- cli.hasOption(CLUSTER_OPTION),
- parameters,
- isUpdate,
- noPrompt);
- } else {
- printRed(
- "Either specify --cluster to deploy cluster level
plugins or --collections <list-of-collections> to deploy collection level
plugins");
- }
- break;
- }
- case "undeploy":
- {
- if (cli.hasOption(CLUSTER_OPTION) ||
cli.hasOption(COLLECTIONS_OPTION)) {
- Pair<String, String> parsedVersion =
parsePackageVersion(cli.getArgList().get(1));
- if (parsedVersion.second() != null) {
- throw new SolrException(
- ErrorCode.BAD_REQUEST,
- "Only package name expected, without a version.
Actual: "
- + cli.getArgList().get(1));
- }
- String packageName = parsedVersion.first();
- String[] collections =
- cli.hasOption(COLLECTIONS_OPTION)
- ? PackageUtils.validateCollections(
-
cli.getOptionValue(COLLECTIONS_OPTION).split(","))
- : new String[] {};
- packageManager.undeploy(packageName, collections,
cli.hasOption(CLUSTER_OPTION));
- } else {
- printRed(
- "Either specify --cluster to undeploy cluster level
plugins or -collections <list-of-collections> to undeploy collection level
plugins");
- }
- break;
- }
- case "uninstall":
- {
- Pair<String, String> parsedVersion =
parsePackageVersion(cli.getArgList().get(1));
- if (parsedVersion.second() == null) {
- throw new SolrException(
- ErrorCode.BAD_REQUEST,
- "Package name and version are both required. Actual: "
- + cli.getArgList().get(1));
- }
- String packageName = parsedVersion.first();
- String version = parsedVersion.second();
- packageManager.uninstall(packageName, version);
- break;
- }
- default:
- throw new RuntimeException("Unrecognized command: " + cmd);
- }
+ handleCommand(command, cmdArgs, packageFlags);
} finally {
packageManager.close();
}
}
log.info("Finished: {}", cmd);
- } catch (Exception ex) {
+ } catch (Exception exception) {
// We need to print this since SolrCLI drops the stack trace in favour
// of brevity. Package tool should surely print the full stacktrace!
- ex.printStackTrace();
- throw ex;
+ exception.printStackTrace();
+ throw exception;
} finally {
// Restore the old logging level
Configurator.setRootLevel(oldLevel);
}
}
+ private void handleCommand(String command, String[] cmdArgs, PackageFlags
packageFlags)
+ throws Exception {
+ switch (command) {
+ case "add-repo":
+ String repoName = cmdArgs[0];
+ String repoUrl = cmdArgs[1];
+ repositoryManager.addRepository(repoName, repoUrl);
+ printGreen("Added repository: " + repoName);
+ break;
+ case "add-key":
+ String keyFilename = cmdArgs[0];
+ Path path = Path.of(keyFilename);
+ repositoryManager.addKey(Files.readAllBytes(path),
path.getFileName().toString());
+ break;
+ case "list-installed":
+ printGreen("Installed packages:\n-----");
+ for (SolrPackageInstance pkg :
packageManager.fetchInstalledPackageInstances()) {
+ printGreen(pkg);
+ }
+ break;
+ case "list-available":
+ printGreen("Available packages:\n-----");
+ for (SolrPackage pkg : repositoryManager.getPackages()) {
+ printGreen(pkg.name + " \t\t" + pkg.description);
+ for (SolrPackageRelease version : pkg.versions) {
+ printGreen("\tVersion: " + version.version);
+ }
+ }
+ break;
+ case "list-deployed":
+ if (packageFlags.collection() != null) {
+ String collection = packageFlags.collection();
+ Map<String, SolrPackageInstance> packages =
+ packageManager.getPackagesDeployed(collection);
+ printGreen("Packages deployed on " + collection + ":");
+ for (String packageName : packages.keySet()) {
+ printGreen("\t" + packages.get(packageName));
+ }
+ } else {
+ // nuance that we use an arg here instead of requiring a --package
parameter with a
+ // value
+ // in this code path
+ String packageName = cmdArgs[0];
+ Map<String, String> deployedCollections =
+ packageManager.getDeployedCollections(packageName);
+ if (!deployedCollections.isEmpty()) {
+ printGreen("Collections on which package " + packageName + " was
deployed:");
+ for (String collection : deployedCollections.keySet()) {
+ printGreen(
+ "\t"
+ + collection
+ + "("
+ + packageName
+ + ":"
+ + deployedCollections.get(collection)
+ + ")");
+ }
+ } else {
+ printGreen("Package " + packageName + " not deployed on any
collection.");
+ }
+ }
+ break;
+ case "install":
+ {
+ Pair<String, String> parsedVersion = parsePackageVersion(cmdArgs[0]);
+ String packageName = parsedVersion.first();
+ String version = parsedVersion.second();
+ boolean success = repositoryManager.install(packageName, version);
+ if (success) {
+ printGreen(packageName + " installed.");
+ } else {
+ printRed(packageName + " installation failed.");
+ }
+ break;
+ }
+ case "deploy":
+ {
+ if (packageFlags.cluster() || packageFlags.collections() != null) {
+ Pair<String, String> parsedVersion =
parsePackageVersion(cmdArgs[0]);
+ String packageName = parsedVersion.first();
+ String version = parsedVersion.second();
+ String[] collections =
+ packageFlags.collections() != null
+ ?
PackageUtils.validateCollections(packageFlags.collections().split(","))
+ : new String[] {};
+ packageManager.deploy(
+ packageName,
+ version,
+ collections,
+ packageFlags.cluster(),
+ packageFlags.parameters(),
+ packageFlags.update(),
+ packageFlags.noPrompt());
+ } else {
+ printRed(
+ "Either specify --cluster to deploy cluster level plugins or
--collections <list-of-collections> to deploy collection level plugins");
+ }
+ break;
+ }
+ case "undeploy":
+ {
+ if (packageFlags.cluster() || packageFlags.collections() != null) {
+ Pair<String, String> parsedVersion =
parsePackageVersion(cmdArgs[0]);
+ if (parsedVersion.second() != null) {
+ throw new SolrException(
+ ErrorCode.BAD_REQUEST,
+ "Only package name expected, without a version. Actual: " +
cmdArgs[0]);
+ }
+ String packageName = parsedVersion.first();
+ String[] collections =
+ packageFlags.collections() != null
+ ?
PackageUtils.validateCollections(packageFlags.collections().split(","))
+ : new String[] {};
+ packageManager.undeploy(packageName, collections,
packageFlags.cluster());
+ } else {
+ printRed(
+ "Either specify --cluster to undeploy cluster level plugins or
-collections <list-of-collections> to undeploy collection level plugins");
Review Comment:
Pre-existing typo now being carried over: `-collections` should be
`--collections` (the `deploy` counterpart a few lines up already says
`--collections`). Cheap to fix while this line is being touched — and if this
check becomes an `@ArgGroup` on an `Undeploy` subcommand, the message goes away
entirely.
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -93,6 +117,75 @@ public class PackageTool extends ToolBase {
.desc("Don't prompt for input; accept all default choices, defaults
to false.")
.get();
+ record PackageFlags(
+ String collections,
+ boolean cluster,
+ String[] parameters,
+ boolean update,
+ String collection,
+ boolean noPrompt) {}
+
+ // --- picocli fields ---
+
+ @picocli.CommandLine.ArgGroup(exclusive = true, multiplicity = "0..1")
+ private ConnectionOptions connectionOptions;
+
+ @picocli.CommandLine.Mixin private CredentialsOptions credentialsOptions;
+
+ @picocli.CommandLine.Parameters(
+ index = "0",
+ arity = "1",
+ paramLabel = "COMMAND",
+ description =
+ "Package command: add-repo, add-key, list-installed, list-available,
list-deployed, install, deploy, undeploy, uninstall.")
+ private String cmd;
Review Comment:
**Model the nine package commands as real picocli subcommands.**
Right now `cmd` is a plain `String` positional and everything after it lands
in the `ARGS` catch-all below, with dispatch done by a hand-written `switch`.
The branch already has the pattern we want in `ZkTool`, which declares
`subcommands = {ConfigSetDownloadTool.class, ZkCpTool.class, ...}` and gets
synopsis, arity checking and per-command help for free.
Concretely, what we lose by not doing that here:
* `bin/solr package install --help` can't produce install-specific help, and
the generated ref-guide page has an **empty** `== Commands` section (see my
comment on `solr-package.adoc`).
* No arity validation. `bin/solr package add-repo myrepo` throws
`ArrayIndexOutOfBoundsException` on `cmdArgs[1]` instead of picocli's "Missing
required parameter".
* Every option is advertised globally even though each belongs to one or two
commands: `--collections` / `--cluster` / `--param` / `--update` are
deploy/undeploy-only, and `-c/--collection` is `list-deployed`-only. Nothing
tells the user, and nothing rejects a wrong combination.
* The `printRed("Either specify --cluster ... or --collections ...")`
runtime checks in `deploy`/`undeploy` are exactly what `@ArgGroup(exclusive =
true, multiplicity = "1")` on a `deploy` subcommand expresses declaratively.
* An unknown command becomes `RuntimeException("Unrecognized command: ...")`
rather than picocli's standard unknown-subcommand usage error.
Suggestion: `@Command(name = "package", subcommands = {AddRepo.class,
AddKey.class, ListInstalled.class, ListAvailable.class, ListDeployed.class,
Install.class, Deploy.class, Undeploy.class, Uninstall.class})`, with each
nested command carrying only its own `@Parameters`/`@Option`. The rest of my
comments assume this lands.
**PS: We should probably extend the LLM prompt template to identify this
pattern of sub-commands and guide it to rework in native PicoCli fashion**
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -381,6 +492,64 @@ public Options getOptions() {
@Override
public int callTool() throws Exception {
- throw new UnsupportedOperationException("This tool does not yet support
PicoCli");
+ String credentials = credentialsOptions.credentials;
+ String solrUrl = resolveSolrUrl(credentials);
+ String zkHost = resolveZkHost(solrUrl, credentials);
+ String[] args = cmdArgs == null ? new String[0] : cmdArgs;
+ PackageFlags packageFlags =
+ new PackageFlags(collections, cluster, param, update, collection,
noPrompt);
+ executePackage(solrUrl, zkHost, credentials, cmd, args, packageFlags);
+ return 0;
+ }
+
+ private String resolveSolrUrl(String credentials) throws Exception {
Review Comment:
**These two resolvers duplicate connection logic that already exists in
three places.**
`resolveZkHost` below re-implements `ZkConnectionOptions.resolveZkHost()`
almost verbatim — same `StatusTool.reportStatus` → `cloud` → `ZooKeeper`
lookup, same `(embedded)` suffix stripping — and `resolveSolrUrl` copies the
stderr string from `CLIUtils.normalizeSolrUrl(CommandLine)` literally.
`CreateTool.callTool()` and `DeleteTool.callTool()` already carry the same
copy/paste, so this would make it the third and fourth.
Suggestion: hoist `resolveSolrUrl(credentials)` and
`resolveZkHost(credentials)` onto `ConnectionOptions` (it already has
`effectiveSolrUrl()` / `effectiveZkHost()`) and have `PackageTool`,
`CreateTool` and `DeleteTool` all call them. With subcommands this becomes even
more valuable: resolution belongs on the parent `package` command once,
inherited by all nine.
Minor while you're in here: `cloud.get("ZooKeeper").toString()` NPEs if the
key is absent; `ZkConnectionOptions` casts and null-checks instead.
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -381,6 +492,64 @@ public Options getOptions() {
@Override
public int callTool() throws Exception {
- throw new UnsupportedOperationException("This tool does not yet support
PicoCli");
+ String credentials = credentialsOptions.credentials;
+ String solrUrl = resolveSolrUrl(credentials);
+ String zkHost = resolveZkHost(solrUrl, credentials);
+ String[] args = cmdArgs == null ? new String[0] : cmdArgs;
+ PackageFlags packageFlags =
+ new PackageFlags(collections, cluster, param, update, collection,
noPrompt);
+ executePackage(solrUrl, zkHost, credentials, cmd, args, packageFlags);
+ return 0;
+ }
+
+ private String resolveSolrUrl(String credentials) throws Exception {
+ if (connectionOptions != null) {
+ String solrUrl = connectionOptions.effectiveSolrUrl();
+ if (solrUrl != null) {
+ return CLIUtils.normalizeSolrUrl(solrUrl);
+ }
+ String zkHost = connectionOptions.effectiveZkHost();
+ if (zkHost != null) {
+ return CLIUtils.solrUrlFromConnection(
+ CloudSolrClient.CloudSolrClientConnection.parse(zkHost),
credentials);
+ }
+ }
+ String zkHostProp = EnvUtils.getProperty("zkHost");
+ if (zkHostProp != null && !zkHostProp.isBlank()) {
+ return CLIUtils.solrUrlFromConnection(
+ CloudSolrClient.CloudSolrClientConnection.parse(zkHostProp),
credentials);
+ }
+ String defaultUrl = CLIUtils.getDefaultSolrUrl();
+ CLIO.err(
+ "Neither --solr-connection, --zk-host or --solr-url parameters, nor
SOLR_CONNECTION, ZK_HOST env var provided, so assuming solr url is "
Review Comment:
This message promises more than the code delivers: the only fallback
actually consulted is `EnvUtils.getProperty("zkHost")` (line above, and again
in `resolveZkHost`). `SOLR_CONNECTION` / the `solrConnection` property is never
read, so it works under commons-cli (via
`CLIUtils.resolveSolrConnectionFromCli`) but is silently ignored on the picocli
path — while this message claims it was checked.
Worth noting the underlying cause is a framework gap rather than something
to solve per-tool: picocli doesn't apply the `CliDefaultValueProvider` to
`@ArgGroup` members when the group is unmatched, which is exactly why
`connectionOptions` can be `null` here. That's the still-unchecked "Solve
value-fallback to ENV" milestone on #3254.
If you spin this into a separate PR, then add a TODO or NOCOMMIT comment
with a link so we know there is a bug here that depends on some other PR.
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -46,6 +49,27 @@
import org.slf4j.LoggerFactory;
/** Supports package command in the bin/solr script. */
+@SuppressWarnings("UnnecessarilyFullyQualified")
[email protected](
+ name = "package",
+ description = "Install, deploy and manage Solr packages in SolrCloud.",
Review Comment:
**The single-line description drops all the guidance the commons-cli help
gives today.**
`getHeader()` further down is effectively a mini-manual: a synopsis and
one-liner for each of the nine commands, plus two operational notes that aren't
documented anywhere else in the CLI:
> (a) Please add '--solr-url http://host:port' parameter if needed (usually
on Windows).
> (b) Please make sure that all Solr nodes are started with
'-solr.packages.enabled=true' parameter.
None of that reaches the picocli path, so `bin/solr package` under
`SOLR_PICOCLI=true` is meaningfully less helpful than today — note (b) in
particular is a common first-run gotcha.
Once the commands are split into subcommands, most of the per-command text
becomes each subcommand's own `description`. The two notes should move here, as
a multi-line `description = { ... }` array in the style of `ZkCpTool`.
Multi-line *command* descriptions render in full —
`SolrCLI.installFirstLineOnlyHelpFactory` only truncates *option* descriptions
— so nothing is lost in interactive help.
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -113,185 +206,203 @@ public String getName() {
+ "don't print stack traces, hence special treatment is needed
here."
+ "Need to turn off logging, and SLF4J doesn't seem to provide
for a way.")
public void runImpl(CommandLine cli) throws Exception {
+ String solrUrl = CLIUtils.normalizeSolrUrl(cli);
Review Comment:
Hoisting the connection resolution into `runImpl` moved it outside the
`Configurator.setRootLevel(Level.OFF)` window that starts in `executePackage`.
Both `normalizeSolrUrl` and `getZkHost` can hit ZooKeeper/Solr and log, so the
"logging free, clean output going through to the user" intent no longer holds
for that phase. `callTool()` has the same shape.
Either move the level switch so it also wraps resolution, or drop the
comment if the narrower window is deliberate.
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -93,6 +117,75 @@ public class PackageTool extends ToolBase {
.desc("Don't prompt for input; accept all default choices, defaults
to false.")
.get();
+ record PackageFlags(
+ String collections,
+ boolean cluster,
+ String[] parameters,
+ boolean update,
+ String collection,
+ boolean noPrompt) {}
+
+ // --- picocli fields ---
+
+ @picocli.CommandLine.ArgGroup(exclusive = true, multiplicity = "0..1")
+ private ConnectionOptions connectionOptions;
+
+ @picocli.CommandLine.Mixin private CredentialsOptions credentialsOptions;
+
+ @picocli.CommandLine.Parameters(
+ index = "0",
+ arity = "1",
+ paramLabel = "COMMAND",
+ description =
+ "Package command: add-repo, add-key, list-installed, list-available,
list-deployed, install, deploy, undeploy, uninstall.")
+ private String cmd;
+
+ @picocli.CommandLine.Parameters(
+ index = "1..*",
+ arity = "0..*",
+ paramLabel = "ARGS",
+ description =
+ "Command-specific arguments (package name[:version], repository
name/URL, key file, etc.).")
+ private String[] cmdArgs;
+
+ @picocli.CommandLine.Option(
+ names = {"--collections"},
+ paramLabel = "COLLECTIONS",
+ description =
+ "Specifies that this action should affect plugins for the given
collections only, excluding cluster level plugins.")
+ private String collections;
+
+ @picocli.CommandLine.Option(
+ names = {"--cluster"},
+ description = "Specifies that this action should affect cluster-level
plugins only.")
+ private boolean cluster;
+
+ @picocli.CommandLine.Option(
+ names = {"--param"},
+ paramLabel = "PARAMS",
+ description = "List of parameters to be used with the deploy command.")
+ private String[] param;
+
+ @picocli.CommandLine.Option(
+ names = {"--update"},
+ description = "If a deployment is an update over a previous deployment.")
+ private boolean update;
+
+ @picocli.CommandLine.Option(
+ names = {"-c", "--collection"},
+ paramLabel = "COLLECTION",
+ description = "The collection to apply the package to.")
Review Comment:
Small consistency point: the picocli and commons-cli descriptions for the
same flags have drifted, and both parsers ship simultaneously until commons-cli
is removed, so users can see two different help texts for one option.
* `-c/--collection`: "The collection to apply the package to." here vs "The
collection to apply the package to, **not required**." on `COLLECTION_OPTION`.
* `--param`: "…used with **the** deploy command." here vs "…used with deploy
command." on `PARAM_OPTION`.
The picocli wording is better in both cases — rewording commons-cli is kind
of a back-break, so perhaps let this stay, but if we get issues in BATS tests
(have not checked) we'll need to tweak the asserts to match both.
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -93,6 +117,75 @@ public class PackageTool extends ToolBase {
.desc("Don't prompt for input; accept all default choices, defaults
to false.")
.get();
+ record PackageFlags(
+ String collections,
+ boolean cluster,
+ String[] parameters,
+ boolean update,
+ String collection,
+ boolean noPrompt) {}
+
+ // --- picocli fields ---
+
+ @picocli.CommandLine.ArgGroup(exclusive = true, multiplicity = "0..1")
+ private ConnectionOptions connectionOptions;
+
+ @picocli.CommandLine.Mixin private CredentialsOptions credentialsOptions;
+
+ @picocli.CommandLine.Parameters(
+ index = "0",
+ arity = "1",
+ paramLabel = "COMMAND",
+ description =
+ "Package command: add-repo, add-key, list-installed, list-available,
list-deployed, install, deploy, undeploy, uninstall.")
+ private String cmd;
+
+ @picocli.CommandLine.Parameters(
+ index = "1..*",
+ arity = "0..*",
+ paramLabel = "ARGS",
+ description =
+ "Command-specific arguments (package name[:version], repository
name/URL, key file, etc.).")
+ private String[] cmdArgs;
+
+ @picocli.CommandLine.Option(
+ names = {"--collections"},
+ paramLabel = "COLLECTIONS",
+ description =
+ "Specifies that this action should affect plugins for the given
collections only, excluding cluster level plugins.")
+ private String collections;
+
+ @picocli.CommandLine.Option(
+ names = {"--cluster"},
+ description = "Specifies that this action should affect cluster-level
plugins only.")
+ private boolean cluster;
+
+ @picocli.CommandLine.Option(
+ names = {"--param"},
+ paramLabel = "PARAMS",
+ description = "List of parameters to be used with the deploy command.")
+ private String[] param;
Review Comment:
This silently changes `--param` arity. The commons-cli option uses
`.hasArgs()` (unlimited), so `--param a=1 b=2` worked; picocli's default arity
for a multi-value option is 1 per occurrence, so `b=2` becomes a positional
instead. Today it is then swallowed by the `ARGS` catch-all and ignored — the
user gets a half-configured deploy with no diagnostic.
Splitting into subcommands removes the catch-all and turns this into a clean
parse error, which is most of the fix. Beyond that, either set `arity = "1..*"`
to preserve the old behaviour, or keep arity 1 and document the repeated form —
`package-manager.adoc` already shows `--param a=1 --param b=2`, so the repeated
form is the documented one and I'd lean that way.
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -93,6 +117,75 @@ public class PackageTool extends ToolBase {
.desc("Don't prompt for input; accept all default choices, defaults
to false.")
.get();
+ record PackageFlags(
Review Comment:
Worth a short javadoc, matching `AuthTool.AuthParams` ("Parameters for the
auth sub-commands, independent of the command line parser") — that record also
documents its non-obvious field. Keeping the pattern self-documenting matters
as more tools copy it.
That said, once the nine commands are separate subcommands this record
likely shrinks a lot or splits per command, since most fields apply to only one
or two of them — so maybe settle its final shape first.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
