janhoy commented on code in PR #5044: URL: https://github.com/apache/solr/pull/5044#discussion_r4205515135
########## solr/core/src/java/org/apache/solr/cli/SnapshotCreateShim.java: ########## @@ -0,0 +1,31 @@ +/* + * 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; + +/** + * The old top-level {@code snapshot-create} spelling of {@code bin/solr snapshot create}, kept so + * that existing scripts keep working. It is hidden from help and the reference guide. + * + * @deprecated Use {@code bin/solr snapshot create}; this spelling is removed in Solr 12. + */ +@Deprecated(since = "11.0") Review Comment: `since = "10.2"` — this lands in 10.2; Same for the other four shims. Removal then becomes 11.0, as part of a future JIRA to remove commons-cli on main. ########## solr/core/src/java/org/apache/solr/cli/SnapshotCreateShim.java: ########## @@ -0,0 +1,31 @@ +/* + * 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; + +/** + * The old top-level {@code snapshot-create} spelling of {@code bin/solr snapshot create}, kept so + * that existing scripts keep working. It is hidden from help and the reference guide. + * + * @deprecated Use {@code bin/solr snapshot create}; this spelling is removed in Solr 12. + */ +@Deprecated(since = "11.0") +@SuppressWarnings("UnnecessarilyFullyQualified") [email protected]( + name = "snapshot-create", + hidden = true, + description = "Deprecated; use 'snapshot create'.") +public class SnapshotCreateShim extends SnapshotCreateTool {} Review Comment: The shims are silent at runtime. Please log a notice via `org.apache.solr.logging.DeprecationLog.log("cli.snapshot-create", "'bin/solr snapshot-create' is deprecated and will be removed in Solr 11; use 'bin/solr snapshot create'.")` from a `callTool()` override (same for the other shims, or one small shared base). It logs once at WARN, which `ToolBase` keeps enabled in non-verbose mode. ########## solr/core/src/test/org/apache/solr/cli/SnapshotToolsPicocliTest.java: ########## @@ -0,0 +1,122 @@ +/* + * 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 java.util.ArrayList; +import java.util.List; +import org.junit.Test; +import picocli.CommandLine; + +/** + * Runs {@link SnapshotToolsTest} through picocli, using the {@code bin/solr snapshot <sub-command>} + * group, and checks that the old {@code snapshot-*} spellings still work as hidden, deprecated + * shims. + */ +public class SnapshotToolsPicocliTest extends SnapshotToolsTest { + + /** + * Builds the real root command, giving each tool the test's runtime so its output is captured. + */ + private static CommandLine root(CLITestHelper.TestingRuntime runtime) { + CommandLine.IFactory factory = + new CommandLine.IFactory() { + @Override + public <K> K create(Class<K> cls) throws Exception { + if (ToolBase.class.isAssignableFrom(cls)) { + try { + return cls.getDeclaredConstructor(ToolRuntime.class).newInstance(runtime); + } catch (NoSuchMethodException e) { + // a shim: it has only the default constructor + } + } + return CommandLine.defaultFactory().create(cls); + } + }; + return new CommandLine(new SolrCLI(), factory); + } + + /** Runs a commons-cli style {@code snapshot-<x> ...} command line as {@code snapshot <x> ...}. */ + static int runAsGroup(String[] args, CLITestHelper.TestingRuntime runtime) { + List<String> grouped = + new ArrayList<>(List.of("snapshot", args[0].substring("snapshot-".length()))); + grouped.addAll(List.of(args).subList(1, args.length)); + return root(runtime).execute(grouped.toArray(new String[0])); + } + + @Override + protected int runTool( + String[] args, CLITestHelper.TestingRuntime runtime, Class<? extends ToolBase> clazz) + throws Exception { + return runAsGroup(args, runtime); + } + + @Test + public void testOldSpellingsStillWork() throws Exception { + CommandLine root = root(new CLITestHelper.TestingRuntime(true)); + String url = cluster.getJettySolrRunner(0).getBaseUrl().toString(); + + assertEquals( + 0, + root.execute( + "snapshot-create", + "-c", + COLLECTION, + "--snapshot-name", + "oldSpelling", + "--solr-url", + url)); + assertTrue(run(SnapshotListTool.class, "snapshot-list").contains("oldSpelling")); + assertEquals( + 0, + root.execute( + "snapshot-delete", + "-c", + COLLECTION, + "--snapshot-name", + "oldSpelling", + "--solr-url", + url)); + assertFalse(run(SnapshotListTool.class, "snapshot-list").contains("oldSpelling")); + } + + @Test + public void testOldSpellingsAreHiddenAndDeprecated() { + CommandLine root = root(new CLITestHelper.TestingRuntime(true)); + for (String sub : List.of("create", "delete", "describe", "export", "list")) { + CommandLine shim = root.getSubcommands().get("snapshot-" + sub); + assertNotNull("snapshot-" + sub, shim); + assertTrue("snapshot-" + sub, shim.getCommandSpec().usageMessage().hidden()); + Deprecated deprecated = shim.getCommand().getClass().getAnnotation(Deprecated.class); + assertNotNull("snapshot-" + sub, deprecated); + assertEquals("11.0", deprecated.since()); Review Comment: `"10.2"` once the shims change. ########## solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-11.adoc: ########## @@ -25,6 +25,22 @@ This page highlights the most important changes including new features and chang Before starting an upgrade to this version of Solr, please be sure to review all information about changes from the version you are currently on up to this one, to include the minor version number changes as well. For example, if you are currently using Solr 10.1, you should review changes made in all subsequent 10.x releases in addition to the 11.0-specific changes on this page. +== Deprecated Features + +=== Snapshot commands are now `bin/solr snapshot` sub-commands + +The `snapshot-create`, `snapshot-delete`, `snapshot-describe`, `snapshot-export` and `snapshot-list` commands of the experimental picocli command line interface are replaced by the sub-commands of `bin/solr snapshot`: + +[source,bash] +---- +bin/solr snapshot create -c mycollection --snapshot-name snap1 +bin/solr snapshot list -c mycollection +---- + +The old spellings still work and are hidden from the help and from this guide. +They are deprecated in Solr 11.0 and are removed in Solr 12.0. Review Comment: Should read deprecated in 10.2 / removed in 11.0, and the note belongs in `major-changes-in-solr-10.adoc` under the 10.2 section. ########## solr/core/src/java/org/apache/solr/cli/SnapshotExportTool.java: ########## @@ -90,6 +124,48 @@ record SnapshotExportParams( String backupRepo, String asyncReqId) {} + // --- picocli fields --- + + @picocli.CommandLine.ArgGroup(exclusive = true, multiplicity = "0..1") + private ConnectionOptions connectionOptions; + + @picocli.CommandLine.Mixin private CredentialsOptions credentialsOptions; + + @picocli.CommandLine.Mixin private CollectionNameOptions collection; + + // Accepted only so that passing it can be rejected with an explanation; see callTool(). + @picocli.CommandLine.Option( + names = "--snapshot-name", + paramLabel = "NAME", + description = "No longer supported; passing it fails with an error.") + private String snapshotNameOpt; + + @picocli.CommandLine.Option( + names = "--dest-dir", + required = true, + paramLabel = "DIR", + description = + "Path of a temporary directory on local filesystem during snapshot export command.") + private String destDirOpt; + + @picocli.CommandLine.Option( + names = "--backup-repo-name", + paramLabel = "DIR", Review Comment: `NAME` — it is a repository name, not a directory; this shows in the synopsis. ########## changelog/unreleased/SOLR-18518-picocli-snapshot.yml: ########## @@ -0,0 +1,9 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc + +title: The snapshot commands are now available as `bin/solr snapshot create`, `delete`, `describe`, `export` and `list` in the experimental picocli command line interface. The `snapshot-create` and the other `snapshot-*` spellings still work there, hidden and deprecated, and are removed in Solr 12. Review Comment: Generic changelog comment, see other PR (add author/link to the main picocli yml file) ########## solr/core/src/test/org/apache/solr/cli/SnapshotToolsPicocliTest.java: ########## @@ -0,0 +1,122 @@ +/* + * 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 java.util.ArrayList; +import java.util.List; +import org.junit.Test; +import picocli.CommandLine; + +/** + * Runs {@link SnapshotToolsTest} through picocli, using the {@code bin/solr snapshot <sub-command>} + * group, and checks that the old {@code snapshot-*} spellings still work as hidden, deprecated + * shims. + */ +public class SnapshotToolsPicocliTest extends SnapshotToolsTest { + + /** + * Builds the real root command, giving each tool the test's runtime so its output is captured. + */ + private static CommandLine root(CLITestHelper.TestingRuntime runtime) { + CommandLine.IFactory factory = + new CommandLine.IFactory() { + @Override + public <K> K create(Class<K> cls) throws Exception { + if (ToolBase.class.isAssignableFrom(cls)) { + try { + return cls.getDeclaredConstructor(ToolRuntime.class).newInstance(runtime); + } catch (NoSuchMethodException e) { + // a shim: it has only the default constructor + } + } + return CommandLine.defaultFactory().create(cls); + } + }; + return new CommandLine(new SolrCLI(), factory); + } + + /** Runs a commons-cli style {@code snapshot-<x> ...} command line as {@code snapshot <x> ...}. */ + static int runAsGroup(String[] args, CLITestHelper.TestingRuntime runtime) { + List<String> grouped = + new ArrayList<>(List.of("snapshot", args[0].substring("snapshot-".length()))); + grouped.addAll(List.of(args).subList(1, args.length)); + return root(runtime).execute(grouped.toArray(new String[0])); + } + + @Override + protected int runTool( + String[] args, CLITestHelper.TestingRuntime runtime, Class<? extends ToolBase> clazz) + throws Exception { + return runAsGroup(args, runtime); + } + + @Test + public void testOldSpellingsStillWork() throws Exception { + CommandLine root = root(new CLITestHelper.TestingRuntime(true)); + String url = cluster.getJettySolrRunner(0).getBaseUrl().toString(); + + assertEquals( + 0, + root.execute( + "snapshot-create", + "-c", + COLLECTION, + "--snapshot-name", + "oldSpelling", + "--solr-url", + url)); + assertTrue(run(SnapshotListTool.class, "snapshot-list").contains("oldSpelling")); + assertEquals( + 0, + root.execute( + "snapshot-delete", + "-c", + COLLECTION, + "--snapshot-name", + "oldSpelling", + "--solr-url", + url)); + assertFalse(run(SnapshotListTool.class, "snapshot-list").contains("oldSpelling")); + } + + @Test + public void testOldSpellingsAreHiddenAndDeprecated() { Review Comment: Can a test method be deprecated? -- 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]
