You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "malliaridis (via GitHub)" <gi...@apache.org> on 2024/07/20 13:44:44 UTC
[PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
malliaridis opened a new pull request, #2577:
URL: https://github.com/apache/solr/pull/2577
https://issues.apache.org/jira/browse/SOLR-16824
# Description
This PR replaces additional deprecated single-dash arguments that were depricated in previous PRs.
# 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](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) 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](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] 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](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
# Additional Notes / TODOs:
The following list contains the arguments that were looked up:
| Argument | Replaced With | `main` migrated | `branch_9x` migrated |
| --- | --- | :-: | :-: |
| `-help` | `--help` or `-h` | ✅ | ❓ |
| `-version` | `--version` or `-v`* | ❓ | ❓ |
| `-foreground` | `--foreground` | ✅ | ❓ |
| `-cloud` | `--cloud` | ✅ | ❓ |
| `-host` | `--host` | ✅ | ❓ |
| `-port` | `--port` | ✅ | ❓ |
| `-scheme` | `--scheme` | ✅ | ❓ |
| `-zkHost` | `--zk-host` | ✅ | ❓ |
| `--zkHost` | `--zk-host` | ✅ | ❓ |
| `-solrUrl` | `--solr-url` | ✅ | ❓ |
| `-solr.home` | `--solr-home` | ✅ | ❓ |
| `-solr-home` | `--solr-home` | ✅ | ❓ |
| `-data.home` | `--data-home` | ✅ | ❓ |
| `-data-home` | `--data-home` | ✅ | ❓ |
| `-confname` | `--conf-name` | ✅ | ❓ |
| `-confdir` | `--conf-dir` | ✅ | ❓ |
| `-collection` | `--collection`* | ❓ | ❓ |
| `-collections` | `--collections`* | ❓ | ❓ |
| `-verbose` | `--verbose`* | ❓ | ❓ |
| `-credentials` | `--credentials`* | ❓ | ❓ |
| `-prompt` | `--prompt`* | ✅ | ❓ |
| `-noprompt` | `--no-prompt` | ✅ | ❓ |
| `--noprompt` | `--no-prompt` | ✅ | ❓ |
| `-action` | `--action` | ✅ | ❓ |
| `-property` | `--property` | ✅ | ❓ |
| `-value` | `--value` | ✅* | ❓ |
| `-solrIncludeFile` | `--solr-include-file` | ✅ | ❓ |
| `-example` | `--example` | ✅ | ❓ |
| `-key` | `--key` | ✅ | ❓ |
| `-all` | `--all` | ✅ | ❓ |
| `-force` | `--force` | ✅ | ❓ |
| `-out` | `--out` | ✅ | ❓ |
While editing I noticed that:
- Between `-noprompt`, `--noprompt` and `--no-prompt`, which option will be maintained?
- [ ] There are occurences that have not been replaced
- Some arguments are not present in `bin/solr` nor `bin/solr.cmd` (e.g. `--collections`)
- [ ] Clarify if `-collections` is also deprecated and should be replaced with `--collections`
- [ ] Clarify the usage of `-collection` (was that a typo ir is there an argument called "collection"?)
- [ ] Clarify difference between `-V` and `-v`
- [ ] Both arguments long form is `--verbose`, is this a problem?
- [ ] `--value` for setting property value does also use `-v` as short form, is this a problem?
- [ ] `bin/solr.cmd` supports `-confdir`, but `bin/solr` does not, should this be fixed?
- [ ] `bin/solr` supports `-solr.home`, but `bin/solr.cmd` does not (same applies for -data.home), should this be fixed?
- There are overlapping short arguments
- [ ] `-d` is used for `--dir` and `--conf-dir`, is this fine?
Overlapping options:
- Overlapping of -d definition for —dir and —conf-dir, is this fine?
- Overlapping of -s definition for —solr-url and —solr-home, is this fine?
The multicharacter command url is still used as -url and resolves to —solr-udpate-url (post tool)
solr/docker/scripts/docker-entrypoint.sh
- Line 37 to 40: Is this still correct?
solr/ref-guide/modules/deployment-guide/pages/jvm-settings.adoc
- Line 66 “java -help”?
solr/ref-guide/modules/deployment-guide/pages/enabling-ssl.adoc
- Invalid character in 252
solr/ref-guide/modules/deployment-guide/pages/solr-control-script-reference.adoc
- Line 1119 Is -scheme now —scheme? (see also solr/bin/solr.cmd:1390)
solr/bin/instal_solr_service.sh
- Line 171 “-help|-usage)” -> Should this be -h|—help|—usage) instead?
solr/bin/solr
- Line 938 Should we not support —collection too?
solr/bin/solr.cmd
- Line 258 Should we also have —version as option?
- Line 295 Duplicate line (removed)
- Line 530 and follow: Do we have to cover always both (- and —) options in the list?
- Line 534 Should we also have “—help”?
- Line 539 Should we also have “-foreground”?
- Line 541 Should we also have “-verbose”?
- Line 547 Should we also have “-dir”?
- Line 545 Should we also have “—cloud”?
- Line 1409 and follow: Do we have to cover additional options (like —conf-dir and —help)?
- Line 1648 and follow: Do we have to cover additional options (—help and -h)?
solr/core/test/org.apache.solr.cli.TestSolrCLIRunExample.java
- Line 117: Should -cloud also be included in the checks?
solr/core/main/org.apache.solr.cli.RunExampleTool.java
- Line 615-620: The arguments are mixed (written out and not), should this be unified?
solr/prometheus-exporter/main/org.apache.solr.prometheus.exporter.SolrExporter.java
- class properties called ARG_***_FLAGS: Should they include the single-dash option too?
solr/core/test/org.apache.solr.cli.TestExportTool.java
- testWithBasicAuth method uses only single dashed arguments
solr/core/test/org.apache.solr.cli.DeleteToolTest.java
- testFailsToDeleteProtectedCollection method uses only single dashed arguments
- testDeleteCollectionWithBasicAuth method uses only single dashed arguments
solr/core/test/org.apache.solr.cli.CreateToolTest.java
- testCreateCollectionWithBasicAuth method uses only single dashed arguments
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #2577:
URL: https://github.com/apache/solr/pull/2577#issuecomment-2241507248
This is some great analysis... Not sure the best way to go through this... Maybe a topic for next week community meetup? Or a working sesion? I think I'll comment in the code, and lets see how that goes. One thing to keep in mind is that we are keeping in branch_9x the deprecated options, however we will be able to get rid of them in 10x.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #2577:
URL: https://github.com/apache/solr/pull/2577#issuecomment-2241516075
There are updates in `main` to `test_post.bats` and `test_help.bats` that you probably want ;-)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #2577:
URL: https://github.com/apache/solr/pull/2577#discussion_r1686856221
##########
solr/core/src/test/org/apache/solr/cli/AuthToolTest.java:
##########
@@ -70,7 +70,7 @@ public void testEnableAuth() throws Exception {
dir.toAbsolutePath().toString(),
"--solr-include-file",
solrIncludeFile.toAbsolutePath().toString(),
- "-credentials",
Review Comment:
after dinking around with the code, I realize that the `-` versus `--` is dealt with by commons-cli, and we don't need to test that logic! I will look for a place to introduce testing of deprecated Option versus current Option...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
Posted by "malliaridis (via GitHub)" <gi...@apache.org>.
malliaridis commented on code in PR #2577:
URL: https://github.com/apache/solr/pull/2577#discussion_r1688758777
##########
solr/bin/solr.cmd:
##########
@@ -292,7 +292,6 @@ IF "%FIRST_ARG%"=="-help" goto run_solrcli
IF "%FIRST_ARG%"=="-usage" goto run_solrcli
IF "%FIRST_ARG%"=="-h" goto run_solrcli
IF "%FIRST_ARG%"=="--help" goto run_solrcli
-IF "%FIRST_ARG%"=="-help" goto run_solrcli
Review Comment:
Line 295 is a duplication of 291, which is why I removed it. Should probably not have an effect?
EDIT: What exactly do you mean with "special case"? I think I don't interpret correctly your comment. 😅
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
Posted by "malliaridis (via GitHub)" <gi...@apache.org>.
malliaridis commented on code in PR #2577:
URL: https://github.com/apache/solr/pull/2577#discussion_r1688758777
##########
solr/bin/solr.cmd:
##########
@@ -292,7 +292,6 @@ IF "%FIRST_ARG%"=="-help" goto run_solrcli
IF "%FIRST_ARG%"=="-usage" goto run_solrcli
IF "%FIRST_ARG%"=="-h" goto run_solrcli
IF "%FIRST_ARG%"=="--help" goto run_solrcli
-IF "%FIRST_ARG%"=="-help" goto run_solrcli
Review Comment:
Line 295 is a duplication of 291, which is why I removed it. Should probably not have an effect?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
Posted by "malliaridis (via GitHub)" <gi...@apache.org>.
malliaridis commented on code in PR #2577:
URL: https://github.com/apache/solr/pull/2577#discussion_r1688759945
##########
solr/bin/solr.cmd:
##########
@@ -1525,10 +1524,10 @@ IF "!ZK_OP!"=="" (
set CONNECTION_PARAMS=""
IF "!ZK_OP!"=="" (
- set CONNECTION_PARAMS="-solrUrl !ZK_SOLR_URL!"
+ set CONNECTION_PARAMS="--solr-url !ZK_SOLR_URL!"
Review Comment:
For deprecation reasons I assume this should be updated to the new format, correct?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #2577:
URL: https://github.com/apache/solr/pull/2577#discussion_r1685667779
##########
solr/bin/solr.cmd:
##########
@@ -1525,10 +1524,10 @@ IF "!ZK_OP!"=="" (
set CONNECTION_PARAMS=""
IF "!ZK_OP!"=="" (
- set CONNECTION_PARAMS="-solrUrl !ZK_SOLR_URL!"
+ set CONNECTION_PARAMS="--solr-url !ZK_SOLR_URL!"
)
ELSE (
- set CONNECTION_PARAMS="-zkHost ZK_HOST!"
+ set CONNECTION_PARAMS="--zk-host ZK_HOST!"
Review Comment:
yep! similar to above issue with `-solrUrl`
##########
solr/bin/solr.cmd:
##########
@@ -314,7 +313,7 @@ goto done
@echo -f Start Solr in foreground; default starts Solr in the background
@echo and sends stdout / stderr to solr-PORT-console.log
@echo.
-@echo -c or -cloud Start Solr in SolrCloud mode; if -z not supplied and ZK_HOST not defined in
+@echo -c or --cloud Start Solr in SolrCloud mode; if -z not supplied and ZK_HOST not defined in
Review Comment:
correct! `--cloud` is what the docs should say. We need to preserve the use of `-cloud` in 9x however...
##########
solr/bin/solr.cmd:
##########
@@ -1525,10 +1524,10 @@ IF "!ZK_OP!"=="" (
set CONNECTION_PARAMS=""
IF "!ZK_OP!"=="" (
- set CONNECTION_PARAMS="-solrUrl !ZK_SOLR_URL!"
+ set CONNECTION_PARAMS="--solr-url !ZK_SOLR_URL!"
Review Comment:
good catch! I suspect because we still support `-solrUrl` that is why this works...
##########
solr/bin/solr.cmd:
##########
@@ -292,7 +292,6 @@ IF "%FIRST_ARG%"=="-help" goto run_solrcli
IF "%FIRST_ARG%"=="-usage" goto run_solrcli
IF "%FIRST_ARG%"=="-h" goto run_solrcli
IF "%FIRST_ARG%"=="--help" goto run_solrcli
-IF "%FIRST_ARG%"=="-help" goto run_solrcli
Review Comment:
`-help` is a special case (and we shoild probably comment that it is so) in the idea that folks might do -h, --help, and -help equally when trying to get to the help!
##########
solr/packaging/test/test_adminconsole_urls.bats:
##########
@@ -25,7 +25,7 @@ teardown() {
# save a snapshot of SOLR_HOME for failed tests
save_home_on_failure
- solr stop -all >/dev/null 2>&1
+ solr stop --all >/dev/null 2>&1
Review Comment:
I honestly stayed away from touching `bin/solr start` and `bin/solr stop` since I suspect lots of folks have scripts that call those and otherwise it seemed complicated. I am up for changing `-all` to `--all`, however, can we augment the bats tests to try both? Just to verify that `-all` and `-all` both work?
##########
solr/core/src/test/org/apache/solr/cli/AuthToolTest.java:
##########
@@ -70,7 +70,7 @@ public void testEnableAuth() throws Exception {
dir.toAbsolutePath().toString(),
"--solr-include-file",
solrIncludeFile.toAbsolutePath().toString(),
- "-credentials",
Review Comment:
so, @gerlowskija and I had a conversation that we ideally should be randomizing some of these, like `-credentials` and `--credentials` to make sure things continue to work with deprecated and non deprecated options. I took the short cut of just randomly picking between the two, however maybe we should introduce a method here to handle this? Alternatively, maybe a comment on why `-credentials` just to remind the next person?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
Posted by "malliaridis (via GitHub)" <gi...@apache.org>.
malliaridis commented on code in PR #2577:
URL: https://github.com/apache/solr/pull/2577#discussion_r1688763185
##########
solr/packaging/test/test_adminconsole_urls.bats:
##########
@@ -25,7 +25,7 @@ teardown() {
# save a snapshot of SOLR_HOME for failed tests
save_home_on_failure
- solr stop -all >/dev/null 2>&1
+ solr stop --all >/dev/null 2>&1
Review Comment:
I think adding tests in addition that checks that both still work is a good idea.
As far as I understand, the function "teardown" is responsible for test cleanup, so I guess it can be updated without problems? Or is there a chance that the test will be executed with a version that does not support `--all`?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #2577:
URL: https://github.com/apache/solr/pull/2577#discussion_r1685676831
##########
solr/core/src/test/org/apache/solr/cli/AuthToolTest.java:
##########
@@ -70,7 +70,7 @@ public void testEnableAuth() throws Exception {
dir.toAbsolutePath().toString(),
"--solr-include-file",
solrIncludeFile.toAbsolutePath().toString(),
- "-credentials",
Review Comment:
Here was the ticket for tracking the randomization idea: https://issues.apache.org/jira/browse/SOLR-17344
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16824: Replace deprecated single-dash arguments [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #2577:
URL: https://github.com/apache/solr/pull/2577
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org