Split extra args into launch and exec #976
Conversation
📝 WalkthroughWalkthroughThis update introduces several documentation and code changes related to the handling of Docker argument environment variables in Geodesic version 4.4.0 and later. The environment variable Changes
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
rootfs/usr/local/bin/boot (1)
22-23: Trim trailing whitespace and confirm visual separator.
The new empty comment line improves readability, but the preceding line ends with an unintended space before the closing quote. Consider removing that trailing whitespace for consistency.docs/environment.md (1)
43-47: Clarify usage regarding quoting of new environment variables.
To ensure users understand that values must be left unquoted to allow proper splitting (as with the deprecatedGEODESIC_DOCKER_EXTRA_ARGS), consider adding a note such as:Starting with Geodesic version 4.4.0: - `GEODESIC_DOCKER_EXTRA_ARGS` is deprecated. Geodesic distinguishes between options for launching a container and options for exec’ing into a running one. Use `GEODESIC_DOCKER_EXTRA_LAUNCH_ARGS` for container launch and `GEODESIC_DOCKER_EXTRA_EXEC_ARGS` for exec’ing into a running container. + Note: Leave these variables unquoted when setting or using them so that multiple space-separated arguments are correctly passed.rootfs/etc/profile.d/_60-register-stray-shells.sh (1)
24-31: Redirect warning messages to stderr.
To avoid mixing error messages with regular output, consider writing to stderr. For example:- echo "# You have launched a login shell without a terminal and without a command." - echo "# This is not supported. Please run a command or attach a terminal." + echo "# You have launched a login shell without a terminal and without a command." >&2 + echo "# This is not supported. Please run a command or attach a terminal." >&2rootfs/templates/wrapper-body.sh (6)
243-243: Add contextual information to debug logs
You've added aprintfindebug()to emit messages whenVERBOSE=true. Consider enriching these logs with a timestamp or context (e.g., the calling function name) to make it easier to trace execution order in complex flows.
247-252: Capture and log exit status in debug_and_run
The newdebug_and_run()wrapper logs before running the command but doesn’t record the exit code. Capturing and emitting the exit status improves observability when a command fails.Example diff:
function debug_and_run() { debug '>>>' debug "Running: $*" - debug '<<<' - "$@" + "$@" + local status=$? + debug "Exit status: $status" + debug '<<<' + return $status }
369-380: Implement _exec_existing helper
This new helper cleanly consolidatesdocker execinvocation. A few polish items:
- Redirect the informative
echo "# Exec'ing shell…"tostderr(>&2) so it doesn’t mix with container I/O.- Quote
--env G_HOST_PID="$WRAPPER_PID"to guard against unexpected splitting (even if unlikely).- For robust word splitting, consider parsing
GEODESIC_DOCKER_EXTRA_EXEC_ARGSinto a Bash array rather than relying on unquoted expansion.
388-399: Deprecation warning for GEODESIC_DOCKER_EXTRA_ARGS
Good handling of the old variable for backward compatibility.
Nit: fix the typo in the warning message—change “igored” to “ignored”.
622-625: Consistent stream for extra launch-args notice
You echo the extra‐args banner tostdout; most warnings go tostderr. Redirect this with>&2for consistency and to avoid mixing with normal container output.
628-633: Consistent stream for detached‐mode extra args notice
Similarly, the detached‐mode branch prints tostdout. Please redirect these notices tostderr(echo >&2) to match other warning patterns.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ReleaseNotes-v4.md(1 hunks)docs/environment.md(1 hunks)rootfs/etc/profile.d/_01-launch-warning.sh(2 hunks)rootfs/etc/profile.d/_60-register-stray-shells.sh(1 hunks)rootfs/templates/wrapper-body.sh(6 hunks)rootfs/usr/local/bin/boot(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Nuru
PR: cloudposse/geodesic#961
File: rootfs/templates/wrapper-body.sh:490-490
Timestamp: 2025-01-20T04:32:40.585Z
Learning: In the Geodesic project, GEODESIC_DOCKER_EXTRA_ARGS environment variable is intentionally left unquoted in docker run commands to allow passing multiple space-separated Docker arguments through a single environment variable.
docs/environment.md (1)
Learnt from: Nuru
PR: cloudposse/geodesic#961
File: rootfs/templates/wrapper-body.sh:490-490
Timestamp: 2025-01-20T04:32:40.585Z
Learning: In the Geodesic project, GEODESIC_DOCKER_EXTRA_ARGS environment variable is intentionally left unquoted in docker run commands to allow passing multiple space-separated Docker arguments through a single environment variable.
ReleaseNotes-v4.md (1)
Learnt from: Nuru
PR: cloudposse/geodesic#961
File: rootfs/templates/wrapper-body.sh:490-490
Timestamp: 2025-01-20T04:32:40.585Z
Learning: In the Geodesic project, GEODESIC_DOCKER_EXTRA_ARGS environment variable is intentionally left unquoted in docker run commands to allow passing multiple space-separated Docker arguments through a single environment variable.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-push (debian)
🔇 Additional comments (7)
ReleaseNotes-v4.md (1)
726-730: Inconsistent version context in release notes.
The file header specifies “Geodesic v4.0.0 Release Notes”, yet this section introduces a deprecation effective in version 4.4.0. Please verify whether to:
- Update the header to reflect v4.4.0, or
- Split this content into a new ReleaseNotes-v4.4.0.md, or
- Adjust the context to clarify these are upcoming v4.4.0 changes.
rootfs/etc/profile.d/_01-launch-warning.sh (2)
14-17: Improve clarity of piped warning mechanism.
The expanded comment block clearly explains howwarn_if_pipedconstructs and erases messages on a terminal, which enhances maintainability and developer understanding. Well done.
67-67: Verify conditional invocation ofwarn_if_piped.
The guard[[ -t 0 ]] && warn_if_pipedexecutes the warning only when stdin is a terminal, which may skip it when input is piped. If the intention is to warn on piped input, this condition might be inverted. Please confirm the desired behavior and adjust the condition accordingly.rootfs/templates/wrapper-body.sh (4)
267-269: Leverage debug_and_run for on-shell-exit hook
Great use ofdebug_and_runto wrap theON_SHELL_EXIThook under verbose mode. This consistently surfaces hook execution when troubleshooting.
271-276: Leverage debug_and_run for on-container-exit hook
WrappingON_CONTAINER_EXITindebug_and_runis a clean way to unify logging. Nice improvement to trace container‐exit behavior.
328-331: Use debug_and_run to wait for container exit
Callingwait_for_container_exitviadebug_and_runmaintains consistent debug output. LGTM.
430-432: Centralize exec logic with _exec_existing
Replacing the inlinedocker execin theuse()path with a call to_exec_existingimproves maintainability by eliminating duplicate code. Approved.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
rootfs/templates/wrapper-body.sh (1)
263-269: Suppress only Docker errors in shell-pid helpers
By callingdebug_and_run --noerr docker exec…, stderr redirection is scoped to the Docker command alone, addressing earlier concerns about hiding debug output.
🧹 Nitpick comments (5)
rootfs/templates/wrapper-body.sh (3)
247-261: Introducedebug_and_runfor structured logging
Wrapping commands indebug_and_runwith pre- and post-logging greatly improves observability. Nit: the indentation of thelocal statusline (two spaces) doesn’t match the surrounding tabs—consider aligning for consistency.
378-389: Extract exec logic into_exec_existinghelper
Centralizingdocker execinvocations and combining interactive flags, env vars, and extra exec args improves modularity. Consider that unquoted${GEODESIC_DOCKER_EXTRA_EXEC_ARGS}can lead to word-splitting or injection risks—refactoring this into a proper array or validating its contents would harden the script.
629-637: Integrate new launch-args intodocker run
Logging the use ofGEODESIC_DOCKER_EXTRA_LAUNCH_ARGSand passing it unquoted todocker runfulfils the PR’s split-args goal. As with exec args, consider an array-based approach (or at least validation) to avoid subtle parsing issues when users supply complex flags.Also applies to: 639-642
rootfs/etc/profile.d/_60-register-stray-shells.sh (2)
20-23: Track stray shells on stderr
Converting the yellow warning for stray shells to>&2ensures it won’t interfere with normal stdout consumption. For consistency with other POSIX changes, you may want to replace[[ $PPID == 0 ]]with[[ $PPID = 0 ]].
25-31: Error on unsupported login shell invocation
Great addition of a clear stderr message and exit status for login shells without a TTY or command. To align with the rest of the codebase, switch[[ $$ == 1 ]]to[[ $$ = 1 ]].
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ReleaseNotes-v4.md(1 hunks)docs/environment.md(1 hunks)rootfs/etc/profile.d/_60-register-stray-shells.sh(1 hunks)rootfs/templates/wrapper-body.sh(10 hunks)rootfs/usr/local/bin/boot(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- rootfs/usr/local/bin/boot
- docs/environment.md
- ReleaseNotes-v4.md
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Nuru
PR: cloudposse/geodesic#961
File: rootfs/templates/wrapper-body.sh:490-490
Timestamp: 2025-01-20T04:32:40.585Z
Learning: In the Geodesic project, GEODESIC_DOCKER_EXTRA_ARGS environment variable is intentionally left unquoted in docker run commands to allow passing multiple space-separated Docker arguments through a single environment variable.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-push (debian)
🔇 Additional comments (6)
rootfs/templates/wrapper-body.sh (6)
13-16: Use POSIX‐compliant string comparison
Replacing==with=in theGEODESIC_SHELLcheck ensures compatibility across more shells.
242-244: Redirect debug output to stderr
Switching fromechotoprintfand sending debug messages to>&2keeps debug logs off stdout and is more reliable thanecho.
276-284: Add logging to exit-hook invocations
Usingdebug_and_runfor bothON_SHELL_EXITandON_CONTAINER_EXIThooks ensures any failures or delays are visible.
328-342: Enhancerun_exit_hookswith debug wrapper
All code paths inrun_exit_hooks, including theONE_SHELLbranch and the “no shells” scenario, now usedebug_and_runfor consistency in logging.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 334-334: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 335-335: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
374-376: Usedebug_and_runin fallback wait loop
Wrappingwait_for_container_exitin the debug helper ensures its start and exit status are always logged.
397-408: Deprecation flow for old extra-args variable
The deprecation warning and fallback assignment toGEODESIC_DOCKER_EXTRA_LAUNCH_ARGSare clear. Verify whether users of the oldGEODESIC_DOCKER_EXTRA_ARGSexpect those flags ondocker execas well—currently only launch args are carried forward.
what
GEODESIC_DOCKER_EXTRA_ARGSis deprecated. Geodesic distinguishes between options for launching a containerand options for exec’ing into a running one. Use
GEODESIC_DOCKER_EXTRA_LAUNCH_ARGSfor container launch andGEODESIC_DOCKER_EXTRA_EXEC_ARGSfor exec’ing into a running container.--verbosemodewhy