ctubbsii commented on code in PR #5035:
URL: https://github.com/apache/accumulo/pull/5035#discussion_r1831498178
##########
assemble/bin/accumulo-cluster:
##########
@@ -590,7 +626,22 @@ function main() {
accumulo_cmd="${bin}/accumulo"
SSH='ssh -qnf -o ConnectTimeout=2'
- case "$1" in
+ # Copy input arguments into new array
+ # removing any options
+ DEBUG=""
+ i=0
+ declare -a program_args
+ for arg in "$@"; do
+ if [[ $arg == "--dry-run" ]]; then
+ DEBUG="true"
+ else
+ program_args[i++]="$arg"
+ fi
+ done
Review Comment:
This parsing becomes a lot simpler if the dry-run flag is expected to be
first. Then, you can just use `shift` to strip it off the front.
If the case block below were put into its own function, it would also
simplify the use of the `program_args` array, allowing you to work directly
with `$@`, `$1`, `$2`, etc. inside the called method.
##########
assemble/bin/accumulo-cluster:
##########
@@ -157,16 +161,33 @@ function control_service() {
# using the value of $host
#
if [[ $# -gt 3 ]]; then
- ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}"
"${bin}/accumulo-service" "$service" "$control_cmd" "-a" "$host" "${@:4}"
+ EXTRA_ARGS="${*:4}"
+ if [[ -n $DEBUG ]]; then
+ echo "ACCUMULO_SERVICE_INSTANCE=\"${ACCUMULO_SERVICE_INSTANCE}\"
\"${bin}/accumulo-service\" \"$service\" \"$control_cmd\" \"-a\" \"$host\"
$EXTRA_ARGS"
+ else
+ ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}"
"${bin}/accumulo-service" "$service" "$control_cmd" "-a" "$host" "$EXTRA_ARGS"
Review Comment:
It's easy for these two lines to diverge over time. It would probably be
useful to create a function that passes the arguments to execute, and
optionally does echo instead. That way, you don't have to worry about making
sure they stay the same.
Something like:
```bash
function dryRunOrExec() {
if [[ -n $DEBUG ]]; then
echo "${@@Q}"
else
"$@"
fi
}
```
Then, you just use it like:
```bash
dryRunOrExec ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}"
"${bin}/accumulo-service" "$service" "$control_cmd" "-a" "$host" "${@:4}"
```
If that `else` block doesn't work as-is, it might need to use `eval` in some
way to handle the `ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}"`
part. Also, this requires bash4 for the `@Q` quoting, so you can copy/paste the
output for manual execution. If you prefer without quotes, for readability, and
don't care about the quoting for copy/paste safety, then you can just use `echo
"$*"`, which will keep things compatible with bash3. However, I'm pretty sure
we require bash4 elsewhere in our scripts, and I don't think it's a goal to
stay bash3 compatible (Mac OS users can just install bash4 using homebrew or
similar).
##########
assemble/bin/accumulo-cluster:
##########
@@ -157,16 +161,33 @@ function control_service() {
# using the value of $host
#
if [[ $# -gt 3 ]]; then
- ACCUMULO_SERVICE_INSTANCE="${ACCUMULO_SERVICE_INSTANCE}"
"${bin}/accumulo-service" "$service" "$control_cmd" "-a" "$host" "${@:4}"
+ EXTRA_ARGS="${*:4}"
Review Comment:
I'm not sure this `EXTRA_ARGS` is working the same as it did before. Here,
it is flattened to a scalar and used as a single argument, whereas before,
`"${@:4}"` was a sliced array and every extra argument would be considered its
own argument. This will probably break things like using `-o key1=value1 -o
key2=value2`, etc.
This is very different than the other place where `EXTRA_ARGS` is used in a
very carefully quoted way when passing over SSH, so that each argument is
treated as separate.
##########
assemble/bin/accumulo-cluster:
##########
@@ -590,7 +626,22 @@ function main() {
accumulo_cmd="${bin}/accumulo"
SSH='ssh -qnf -o ConnectTimeout=2'
- case "$1" in
+ # Copy input arguments into new array
+ # removing any options
+ DEBUG=""
Review Comment:
It's common to use variables like this by setting them to a numeric value, 0
or 1. Instead of checking that it is non-zero length (`-n $DEBUG`), it'd
probably be better to check that it has been been explicitly set to `1`
(`$DEBUG == 1`; this is string equality, not numeric equality, but that's fine,
since it's just a toggle, and `==` looks better than `eq` for this comparison),
and initializing it to `0` here.
```suggestion
DEBUG=0
```
You could also create another function to use to check the global variable
instead of comparing the variable each time:
```bash
function isDebug() {
[[ $DEBUG == 1 ]]
}
isDebug && echo "..."
if ! isDebug; then
# do something if debug is not on
fi
```
This isn't a huge benefit, but might improve readability in a few places.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
