malliaridis commented on code in PR #3223:
URL: https://github.com/apache/solr/pull/3223#discussion_r1981850710
##########
solr/core/src/java/org/apache/solr/cli/AuthTool.java:
##########
@@ -75,8 +75,10 @@ public class AuthTool extends ToolBase {
.longOpt("solr-include-file")
.hasArg()
.argName("FILE")
+ .required()
Review Comment:
Looking into our source code, this is actually required and always set via
the CLI script, and if not set it would likely fail with a NPE I believe?
What I am wondering though is, should this be required? Does the file always
exist and does it contain information we need? Or should we simply handle cases
where it is not defined?
##########
solr/bin/solr:
##########
@@ -612,22 +612,11 @@ if [[ "$SCRIPT_CMD" == "auth" ]]; then
fi
fi
- if [ -z "${AUTH_PORT:-}" ]; then
- for ID in $(ps auxww | grep java | grep start\.jar | awk '{print $2}' |
sort -r)
- do
- port=$(jetty_port "$ID")
- if [ "$port" != "" ]; then
- AUTH_PORT=$port
- break
- fi
- done
- fi
-
- run_tool auth $@ --solr-url
"$SOLR_URL_SCHEME://$SOLR_TOOL_HOST:${AUTH_PORT:-8983}" --auth-conf-dir
"$SOLR_HOME" "--solr-include-file" "$SOLR_INCLUDE"
+ run_tool auth $@ --auth-conf-dir "$SOLR_HOME" "--solr-include-file"
"$SOLR_INCLUDE"
exit $?
fi
-# at this point all tools that have a custom run process, like "status" and
"auth" have been run and exited.
+# at this point the only tool that has a custom run process, "auth" have been
run and exited.
Review Comment:
```suggestion
# at this point the only tool that has a custom run process, "auth" has been
run and exited.
```
##########
solr/core/src/java/org/apache/solr/cli/AuthTool.java:
##########
@@ -96,7 +98,7 @@ public class AuthTool extends ToolBase {
.argName("FILE")
.required()
.desc(
- "This is where any authentication related configuration files,
if any, would be placed.")
+ "This is where any authentication related configuration files,
if any, would be placed. Defaults to $SOLR_HOME.")
Review Comment:
If I am not mistaken, this defaults to `SOLR_HOME` only when `AuthTool` is
used via the CLI script, but if directly used it does not default to any path.
Is that correct?
I think we had a discussion before about using the classes without the CLI
scripts, but I don't remember anymore if that was a valid scenario.
##########
solr/bin/solr:
##########
@@ -612,22 +612,11 @@ if [[ "$SCRIPT_CMD" == "auth" ]]; then
fi
fi
- if [ -z "${AUTH_PORT:-}" ]; then
- for ID in $(ps auxww | grep java | grep start\.jar | awk '{print $2}' |
sort -r)
- do
- port=$(jetty_port "$ID")
- if [ "$port" != "" ]; then
- AUTH_PORT=$port
- break
- fi
- done
- fi
-
- run_tool auth $@ --solr-url
"$SOLR_URL_SCHEME://$SOLR_TOOL_HOST:${AUTH_PORT:-8983}" --auth-conf-dir
"$SOLR_HOME" "--solr-include-file" "$SOLR_INCLUDE"
Review Comment:
This eliminates the `AUTH_PORT` and `SOLR_TOOL_HOST` environment variables,
right? I believe it is fine, as the behavior is checking the jetty port and
falls back to the default Solr port, so no need to distinguish it in the
scripts.
Still, this is a breaking change.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
