Add/fix wrong/missing "solr.home" to zkcli scripts

[laminelam]

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

Description

"solr.home" is missing in zkcli.bat and wrong in zkcli.sh (solrHome instead of solr.home)

Solution

Add/replace the correct name.

Tests

Local tests for both scripts (.sh and .bat)

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)
• 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

[epugh]

Thanks for looking at this.. one thought, would you want to add some BATS test to confirm these fixes? They have been great for verifying a lot of the SolrCLI things... Down the road, I hope to eliminate the need for the zkcli.sh completely, in favour of using bin/solr commands... Having some BATS tests would help that migration ;-).

[laminelam]

Thanks for looking at this.. one thought, would you want to add some BATS test to confirm these fixes? They have been great for verifying a lot of the SolrCLI things... Down the road, I hope to eliminate the need for the zkcli.sh completely, in favour of using bin/solr commands... Having some BATS tests would help that migration ;-).

Hi @epugh
Thanks for the great suggestion. Didn't know about BATS.
I have added a couple of BATS tests to test both 'solr.home" and "solrhome" options.
Wondering if it's worth it keeping "solr.home" option, it's redundant and confusing

[epugh]

i had no idea that behind the zkcli scripts was a Java class! I think we should only have one variable name, whichever we have been using the most.

One more thought... I've been slowly moving various scripts under the "bin/solr" SolrCLI as tools. Once this get's merged, would you be interested in migrating this over to "bin/solr"? I wonder how much of what is in the "bin/solr zk" command is duplicated htere in zkcli?

[epugh]

Might be worth a grep thorugh the ref guide re "solrHome" versus "solr.home" too!

[laminelam]

One more thought... I've been slowly moving various scripts under the "bin/solr" SolrCLI as tools. Once this get's merged, would you be interested in migrating this over to "bin/solr"? I wonder how much of what is in the "bin/solr zk" command is duplicated htere in zkcli?

Yes sure, will be more than happy to help on this.

[laminelam]

Might be worth a grep thorugh the ref guide re "solrHome" versus "solr.home" too!

The only place where "solr.home" is used is here

[laminelam]

Hi @epugh
I'd really appreciate if you could finish the review and, if approved, merge it.
I have other PRs waiting for this one before I could push them upstream. Thank you.

[epugh]

Hi @epugh I'd really appreciate if you could finish the review and, if approved, merge it. I have other PRs waiting for this one before I could push them upstream. Thank you.

are you going to change the -t to a -v for verbose and use that int he bats test? That way we don't have custom code that is ONLY used in a test.. it's a more generic feature... I'd love to see that...

[laminelam]

Hi @epugh I'd really appreciate if you could finish the review and, if approved, merge it. I have other PRs waiting for this one before I could push them upstream. Thank you.

are you going to change the -t to a -v for verbose and use that int he bats test? That way we don't have custom code that is ONLY used in a test.. it's a more generic feature... I'd love to see that...

Done.
That's a good suggestion. I can add another PR verbosing more logs. For now it's just logging solrhome as it's the subject of this PR.

[epugh]

Great. Will merge when in front of laptop.

…

On Fri, Aug 18, 2023 at 1:29 PM Lamine ***@***.***> wrote: Hi @epugh <https://github.com/epugh> I'd really appreciate if you could finish the review and, if approved, merge it. I have other PRs waiting for this one before I could push them upstream. Thank you. are you going to change the -t to a -v for verbose and use that int he bats test? That way we don't have custom code that is ONLY used in a test.. it's a more generic feature... I'd love to see that... Done. That's a good suggestion. I can add another PR verbosing more logs. For now it's just logging *solrhome* as it's the subject of this PR. — Reply to this email directly, view it on GitHub <#1798 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFO63HFGXDW34T6TXMVLLXV6Q7TANCNFSM6AAAAAA2QJKJYA> . You are receiving this because you were mentioned.Message ID: ***@***.***>