SOLR-17489: CLI: Deprecate variations on solr urls options #2756
Conversation
github-actions
bot
added
documentation
|
@malliaridis okay, this one is ready for another set of eyes! |
|
After merging this, we can then sort out the |
There was a problem hiding this comment.
I like the fact that we can use the same params in all tools now. I hope this change is preferred by the others as well. A few more references:
- The error message in PostTool#runImpl (line 309) could be updated to reflect the none-deprecated options
- post-tool.adoc#27: -url should be split into --solr-url and --name
- post-tool.adoc#115: -url should be split into --solr-url and --name
- post-tool.adoc#124: -url should be split into --solr-url and --name
- post-tool.adoc#131: -url should be split into --solr-url and --name
- post-tool.adoc#138: -url should be split into --solr-url and --name
- post-tool.adoc#154: -url should be split into --solr-url and --name
- post-tool.adoc#166: -url should be split into --solr-url and --name
- post-tool.adoc#173: -url should be split into --solr-url and --name
- post-tool.adoc#180: -url should be split into --solr-url and --name
- post-tool.adoc#189: -url should be split into --solr-url and --name
- post-tool.adoc#210: -url should be split into --solr-url and --name
- post-tool.adoc#219: -url should be split into --solr-url and --name
- spatal-search.adoc#74: -url should be split into --solr-url and --name
| url = SolrCLI.normalizeSolrUrl(cli) + "/solr/" + cli.getOptionValue("name"); | ||
|
|
||
| } else { | ||
| url = cli.getOptionValue("solr-collection-url"); |
There was a problem hiding this comment.
Since solr-collection-url was required before, we should check for null here and throw an error if it is also not provided.
| url = SolrCLI.normalizeSolrUrl(cli) + "/solr/" + cli.getOptionValue("name"); | ||
|
|
||
| } else { | ||
| url = cli.getOptionValue("solr-collection-url"); |
There was a problem hiding this comment.
Same null-check applies here as well.
| "--solr-update-url", | ||
| cluster.getJettySolrRunner(0).getBaseUrl() + "/" + collection + "/update", | ||
| "--solr-url", | ||
| cluster.getJettySolrRunner(0).getBaseUrl().toString(), |
There was a problem hiding this comment.
getBaseUrl() extends v1 API path /solr, and the new logic does also add /solr/ if --solr-url is used. This results to a path like /solr/solr/collection/update.
There was a problem hiding this comment.
i think I fixed it with the use of SolrCLI.normalizeSolrUrl() in the tool
| "-url", | ||
| cluster.getJettySolrRunner(0).getBaseUrl() + "/" + COLLECTION_NAME, | ||
| "--solr-url", | ||
| cluster.getJettySolrRunner(0).getBaseUrl().toString(), |
There was a problem hiding this comment.
Same issue with path here.
| "--solr-update-url", | ||
| solrUrl + "/" + testCollectionName + "/update", | ||
| "--solr-url", | ||
| solrUrl, |
There was a problem hiding this comment.
Same problem with path applies here as well, but this uses different logic.
There are some legacy -url in the tets that I am leaving.
|
Will add CHANGES.txt all at once. |
Use --solr-url and --name consistently. (cherry picked from commit 0f4a465)
Use --solr-url and --name consistently.
https://issues.apache.org/jira/browse/SOLR-17489
Description
See JIRA
Solution
Deprecate old options in favour of
--solr-urland--name. I am really liking how this looks after going through the code...Debating if we should be using
--collectioninstead of--name.... That can be a seperate ticket.Tests
bats and java