bamaer commented on PR #8312: URL: https://github.com/apache/hop/pull/8312#issuecomment-5630661863
Reviewed by fetching the head (`4eb159a`), syntax-checking `load-and-execute.sh`, and running the patched block against a stub `hop-conf.sh` to compare the argument vectors before and after. The diagnosis and the fix are correct. Observed behaviour of the patched block: | `HOP_CONFIG_OPTIONS` | before | after | |---|---|---| | `--set-variable=FOO=bar --describe-variable=FOO=Example` | 1 argument (the reported bug) | 2 arguments | | `--set-variable=FOO=bar` | 1 argument | 1 argument | | empty / unset | block skipped | block skipped | `ENV HOP_CONFIG_OPTIONS=` in [docker/Dockerfile#L97](https://github.com/apache/hop/blob/4eb159a7e913c563f7a4a9966a55a3ecb29abc0d/docker/Dockerfile#L97) and [docker/unified.Dockerfile#L320](https://github.com/apache/hop/blob/4eb159a7e913c563f7a4a9966a55a3ecb29abc0d/docker/unified.Dockerfile#L320) satisfies the `set -u` at [load-and-execute.sh#L19](https://github.com/apache/hop/blob/4eb159a7e913c563f7a4a9966a55a3ecb29abc0d/docker/resources/load-and-execute.sh#L19), and expanding an empty array under `set -u` is safe on the bash 5 that the alpine images install. Line 309 was also the only quoted single-argument pass-through of an options variable under `docker/resources/` — the remaining quoted expansions there are single-value function arguments — so the change covers the reported issue completely. ### Values containing spaces One case that worked before the change no longer does: a single option whose value contains spaces. ``` HOP_CONFIG_OPTIONS='--describe-variable=FOO=An example variable' expected: hop-conf receives 1 argument actual: hop-conf receives 3 arguments <--describe-variable=FOO=An> <example> <variable> ``` That is the documented format for the option — [SetHopConfigVariables.java#L40](https://github.com/apache/hop/blob/4eb159a7e913c563f7a4a9966a55a3ecb29abc0d/engine/src/main/java/org/apache/hop/config/SetHopConfigVariables.java#L40) describes it as `VARIABLE=Description`, so descriptions containing spaces are the expected usage. The example added to the docs in this PR (`--describe-variable=FOO=Example`) uses a single-word description, so it does not exercise this path. A quote-aware split keeps that case working and removes the need for the `SC2206` suppression: ```bash # Docker env vars cannot carry a bash array; split like a shell would, # honouring quotes so option values may contain spaces. mapfile -t HOP_CONFIG_OPTION_ARRAY < <(xargs -n1 printf '%s\n' <<< "${HOP_CONFIG_OPTIONS}") ``` Verified that the busybox `xargs` in `alpine:latest` handles the quoting as required (`--describe-variable=FOO="An example variable"` yields a single argument), so this works in the runtime image. The alternative is to keep the plain word-split and state in the documentation that option values cannot contain spaces. -- 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]
