SOLR-17442: Resolve -v flag conflict (version, value, verbose) #2721

epugh · 2024-09-23T14:41:50Z

https://issues.apache.org/jira/browse/SOLR-17442

Description

Deprecating -v in favour of -d.

Solution

Please provide a short description of the approach taken to implement your solution.

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

…added options

…age --value

…olr maybe we have a "info" tool seperate from -v and --version. Tweak the help output

…ted docs

malliaridis

I forgot to mention in the ticket that introducing -d for debug may require further deprecations where -d is used for other options as well.

malliaridis · 2024-09-24T07:41:38Z

solr/core/src/java/org/apache/solr/cli/SolrCLI.java

@@ -148,9 +154,21 @@ public class SolrCLI implements CLIO {
  public static final Option OPTION_VERBOSE =


Do we mark deprecated options with @Deprecated? If so, this would be beneficial for OPTION_VERBOSE.

hey, I hadn't thought about that use.... Good catch, and we should.

malliaridis · 2024-09-24T07:45:18Z

solr/core/src/java/org/apache/solr/cli/SolrCLI.java

+          .deprecated(
+              DeprecatedAttributes.builder()
+                  .setForRemoval(true)
+                  .setSince("9.7")
+                  .setDescription("Use --solr-url instead")
+                  .get())


Leaving this as a note: Splitting url from solr-url would have a way too large impact in the rest of the tools, so simply marking both as deprecated is a good tradeoff.

interesting, I actually don't know why I added this... It doesn't seem to fit the rest of this PR...

malliaridis · 2024-09-24T07:55:55Z

solr/core/src/java/org/apache/solr/cli/SolrCLI.java

+    if (!cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt()))
+        || cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt())) {


By putting both options (verbose and debug) into an OptionGroup, this can be simplified as well.

malliaridis · 2024-09-24T07:58:37Z

solr/core/src/java/org/apache/solr/cli/SolrCLI.java

+  public static final Option OPTION_DEBUG =
+      Option.builder("d")
+          .longOpt("debug")
+          .required(false)
+          .desc("Enable additional command output.")
+          .build();


I wasn't sure if we would go for the --debug option. Shall we also deprecate the -d for all the other arguments as well in this PR? We use -d in quite a few places and providing the -d now for debug to all tools may cause unexpected behavior.

epugh · 2024-09-24T13:58:32Z

I forgot to mention in the ticket that introducing -d for debug may require further deprecations where -d is used for other options as well.

I am wondering if we should just stick to --debug, I saw a -d for directory... it's not a common command to run, so maybe having a short cut isn't that valuable.... And dealing with the -d, well that seems really tough....

epugh · 2024-09-24T14:11:12Z

I forgot to mention in the ticket that introducing -d for debug may require further deprecations where -d is used for other options as well.

I am wondering if we should just stick to --debug, I saw a -d for directory... it's not a common command to run, so maybe having a short cut isn't that valuable.... And dealing with the -d, well that seems really tough....

I checked what Docker does, just for run, and they use -D:

-D, --debug              Enable debug mode

I am kind of leaning towards avoiding the whole -d topic by removing it

malliaridis · 2024-09-24T14:22:40Z

-d is one of the conflicting flags that we have to resolve anyway. I would prefer only the long variant in that case, which may be either debug or verbose, both are fine and when the user uses either of them he is already comitted to be more explicit with the CLI, if that makes sense.

malliaridis

I think we should start sooner using OptionGroups. I realized in this review and after testing it that we could simplify a lot of logic with them.

Many comments but mostly minor or repetitive things.

solr/bin/solr.cmd

solr/bin/solr

malliaridis · 2024-09-25T11:35:22Z

solr/core/src/java/org/apache/solr/cli/AssertTool.java

+    debug =
+        cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt()))
+            || cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt());


With an OptionGroup this could be simplified to debug = cli.hasOption(SolrCLI.OPTION_DEBUG), where SolrCLI.OPTION_DEBUG is the OptionGroup that includes verbose and debug options.

Note that the help outputs may be different if using option groups.

I think I need to dig more in and write a unit test to prove that works!

malliaridis · 2024-09-25T11:37:41Z

solr/core/src/java/org/apache/solr/cli/CreateTool.java

+    if (cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt()))
+        || cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt())) {


... and here (OptionGroup).

malliaridis · 2024-09-25T11:39:43Z

solr/core/src/java/org/apache/solr/cli/SolrCLI.java

-    print("such as:    ./solr start --help or ./solr stop -h");
+    print("Global Options:");
+    print("  -v,  --version           Print version information and quit");
+    print("  -d,  --debug             Enable debug mode");


-d should probably not included here.

malliaridis · 2024-09-25T11:40:12Z

solr/core/src/java/org/apache/solr/cli/ToolBase.java

+        cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt()))
+            || cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt());


OptionGroup simplification.

malliaridis · 2024-09-25T11:40:15Z

solr/core/src/java/org/apache/solr/cli/ToolBase.java

+    if (cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt()))
+        || cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt())) {


OptionGroup simplification.

solr/packaging/test/test_bats.bats

Signed-off-by: Eric Pugh <[email protected]>

epugh · 2024-09-28T11:47:22Z

@malliaridis I was able to integrate OptionGroup for the --debug and --verbose options in this PR and it works. What I haven't been able to grok is how to simplify the checking for the flags. You still have to check for each it apepars!! If you find some code that avoids that, let me know.

I am going to open a fresh PR that uses optiongroups more widely.

epugh · 2024-09-28T12:29:37Z

I was at the point of merging and the --debug syntax just bugged me. I am backing it out and just keep --verbose which rings better for me, and still accomplishs the goal of making -v no longer overlap.

malliaridis · 2024-09-28T12:54:46Z

You still have to check for each it apepars!! If you find some code that avoids that, let me know.

I see that you are using the Option#getOpt() and Option#getLongOpt() when you use CommandLine#getOptionValue(String option) or CommandLine#hasOption(String option).

There is also a CommandLine#getOptionValue(Option option) and CommandLine#getOptionValue(OptionGroup optionGroup) that you could use instead. Same applies for hasOption. That way it is possible to use the OptionGroup and simplify logic as proposed before.

You can also set things like required (exactly one of the options in the group is present) at the OptionGroup via OptionGroup#setRequired(boolean required). This is useful for cases where required options are split intot two due to deprecation, made optional and checked later for presence explicitly, like in ConfigTool.java:

Option.builder("v")
    .longOpt("value")
    .deprecated(...)
    .required(false)
    //...
Option.builder()
    .longOpt("value")
    .required(false) // should be true when -v is removed.
    //...

String value = SolrCLI.getOptionWithDeprecatedAndDefault(cli, "value", "v", null);
//...
if (value == null) {
    throw new MissingArgumentException("'value' is a required option.");
}

Using OptionGroups in a wider range like you started in the other PR will probably render SolrCLI#getOptionWithDeprecatedAndDefault(...) obsolete.

Deprecate the various uses of -v, so that we don't have conflicts there. --------- Signed-off-by: Eric Pugh <[email protected]> (cherry picked from commit eda5ec3)

malliaridis · 2024-09-29T13:22:57Z

solr/core/src/java/org/apache/solr/cli/SolrCLI.java

+          .deprecated(
+              DeprecatedAttributes.builder()
+                  .setForRemoval(true)
+                  .setSince("9.8")
+                  .setDescription("Use --debug instead")
+                  .get())


@epugh could you review this file again? I think this should be undone if we won't introduce --debug and deprecate -v for version (below too).

epugh added 2 commits September 23, 2024 10:01

add debug option and use it. solve bug where -h doesnt show globally …

6143378

…added options

first pass through of deprecating -v in favour -f -d

fd64e6f

github-actions bot added the cat:cli label Sep 23, 2024

epugh added 6 commits September 23, 2024 12:47

clarify that -v is for version, not for value. deprecate -v and encou…

323da9e

…age --value

if we are using -d then echoIfDebug follows the pattern.

3cc2a77

lint

0c9779d

deprecate the -v and -V in favour --debug

657b89f

remove version as an actual tool. in the future if we want to probe s…

4cf5e7d

…olr maybe we have a "info" tool seperate from -v and --version. Tweak the help output

check various places we do solr start and solr stop to make sure upda…

650a691

…ted docs

malliaridis requested changes Sep 24, 2024

View reviewed changes

epugh added 2 commits September 24, 2024 20:19

back out deprecation on what is not deprecated, at least in this PR

02ff728

back out deprecation on what is not deprecated, at least in this PR

a8dfd23

github-actions bot added documentation Improvements or additions to documentation start-scripts labels Sep 25, 2024

remove references to -d and use --debug in places

5675a61

malliaridis requested changes Sep 25, 2024

View reviewed changes

epugh added 2 commits September 27, 2024 15:22

Remove short form of --debug

7779d19

Signed-off-by: Eric Pugh <[email protected]>

Use OptionGroup to force either deprecated or non deprecated option

8a540e5

Restore Verbose option

a313f1f

epugh added 3 commits September 28, 2024 09:14

lint

fc0120d

Merge remote-tracking branch 'upstream/main' into SOLR-17442

0003706

Track changes

722e94e

epugh merged commit eda5ec3 into apache:main Sep 28, 2024
4 of 5 checks passed

malliaridis reviewed Sep 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17442: Resolve -v flag conflict (version, value, verbose) #2721

SOLR-17442: Resolve -v flag conflict (version, value, verbose) #2721

epugh commented Sep 23, 2024

malliaridis left a comment

malliaridis Sep 24, 2024

epugh Sep 24, 2024

malliaridis Sep 24, 2024

epugh Sep 25, 2024

malliaridis Sep 24, 2024 •

edited

Loading

malliaridis Sep 24, 2024

epugh commented Sep 24, 2024

epugh commented Sep 24, 2024

malliaridis commented Sep 24, 2024

malliaridis left a comment

malliaridis Sep 25, 2024

epugh Sep 28, 2024

malliaridis Sep 25, 2024

malliaridis Sep 25, 2024

malliaridis Sep 25, 2024

malliaridis Sep 25, 2024

epugh commented Sep 28, 2024

epugh commented Sep 28, 2024

malliaridis commented Sep 28, 2024 •

edited

Loading

malliaridis Sep 29, 2024

		@@ -148,9 +154,21 @@ public class SolrCLI implements CLIO {
		public static final Option OPTION_VERBOSE =

		if (!cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt()))
		\|\| cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt())) {

		if (cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt()))
		\|\| cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt())) {

		cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt()))
		\|\| cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt());

SOLR-17442: Resolve -v flag conflict (version, value, verbose) #2721

SOLR-17442: Resolve -v flag conflict (version, value, verbose) #2721

Conversation

epugh commented Sep 23, 2024

Description

Solution

Tests

Checklist

malliaridis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malliaridis Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epugh commented Sep 24, 2024

epugh commented Sep 24, 2024

malliaridis commented Sep 24, 2024

malliaridis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epugh commented Sep 28, 2024

epugh commented Sep 28, 2024

malliaridis commented Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

malliaridis Sep 24, 2024 •

edited

Loading

malliaridis commented Sep 28, 2024 •

edited

Loading