-
Notifications
You must be signed in to change notification settings - Fork 396
Improve Docker Compose progress output for better user experience #97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Replace cluttered Docker Compose progress output with clean dot-based progress indication by default. Add --debug (-g) option to show detailed Docker progress when needed. Default behavior: - Shows user-friendly messages with dot progress - Clean terminal output Debug mode (--debug): - Shows full Docker Compose progress output - Useful for troubleshooting installation issues
When installation fails in default (clean) mode, show a helpful error message suggesting users run with --debug flag for detailed output. Features: - Clear error indication with emoji - Actionable troubleshooting advice - Maintains clean UX while providing debugging path - Applied to critical Docker Compose operations This helps users quickly identify and resolve installation issues.
Fix minor formatting inconsistencies in the newly added code sections: - Standardize redirection spacing (>/dev/null instead of > /dev/null) This maintains consistency with the existing codebase formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 1 file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the Redash setup script by implementing a dual-mode progress display system that provides cleaner terminal output during Docker image downloads by default, while maintaining full debugging capabilities via a new --debug flag.
- Adds a
--debugflag (-g) to toggle between clean progress dots and full Docker output - Implements background job execution for
docker compose pullwith dot-based progress indication - Introduces an error handling function that suggests using
--debugfor troubleshooting
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
setup.sh
Outdated
| while kill -0 $PID 2>/dev/null; do | ||
| echo -n "." | ||
| sleep 2 | ||
| done | ||
| wait $PID || { | ||
| echo " Failed!" | ||
| handle_error | ||
| } |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential process leak: If the script is interrupted (e.g., Ctrl+C) while waiting for the background docker compose pull process (lines 339-342), the background process may continue running orphaned. The script uses set -eu which doesn't trap signals.
Consider adding a trap to clean up the background process: trap 'kill $PID 2>/dev/null' EXIT INT TERM before starting the background job.
| while kill -0 $PID 2>/dev/null; do | |
| echo -n "." | |
| sleep 2 | |
| done | |
| wait $PID || { | |
| echo " Failed!" | |
| handle_error | |
| } | |
| trap 'kill "$PID" 2>/dev/null || true' EXIT INT TERM | |
| while kill -0 "$PID" 2>/dev/null; do | |
| echo -n "." | |
| sleep 2 | |
| done | |
| wait "$PID" || { | |
| echo " Failed!" | |
| handle_error | |
| } | |
| trap - EXIT INT TERM |
setup.sh
Outdated
| docker compose up -d || handle_error | ||
| else | ||
| echo "Starting containers..." | ||
| docker compose up -d || handle_error |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docker compose up -d command on line 357 (non-debug mode) may also produce progress output when pulling images that weren't already pulled. This is similar to the issue with line 349 where the database creation might pull images. The -d flag runs containers in detached mode but doesn't suppress pull progress.
For consistency with the clean output goal, consider adding similar output redirection or ensuring all images are pulled in the earlier docker compose pull step.
| docker compose up -d || handle_error | |
| docker compose up -d >/dev/null 2>&1 || handle_error |
setup.sh
Outdated
| echo "Creating database..." | ||
| docker compose run --rm server create_db || handle_error |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The progress output suppression logic has a critical flaw: docker compose pull only downloads images, but the actual database creation step with docker compose run --rm server create_db on line 349 will still download any missing images (like the server image) with full progress output visible to the user. This defeats the purpose of the clean progress indication since users will still see Docker's default progress bars during database creation.
Consider moving the docker compose pull to download all required images before the database creation step, or redirect output for the docker compose run command as well in non-debug mode.
| echo "Creating database..." | |
| docker compose run --rm server create_db || handle_error | |
| echo -n "Creating database" | |
| docker compose run --rm server create_db >/dev/null 2>&1 & | |
| PID=$! | |
| while kill -0 $PID 2>/dev/null; do | |
| echo -n "." | |
| sleep 2 | |
| done | |
| wait $PID || { | |
| echo " Failed!" | |
| handle_error | |
| } | |
| echo " Done!" |
setup.sh
Outdated
| wait $PID || { | ||
| echo " Failed!" | ||
| handle_error | ||
| } |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Race condition in exit status handling: the wait $PID command may succeed even if the background process failed, because the error status check happens in the OR clause after wait completes successfully. The current construct wait $PID || { ... } will only execute the error block if wait itself fails (e.g., PID doesn't exist), not if the background process exits with a non-zero status.
The correct pattern should capture the exit status explicitly: wait $PID; STATUS=$?; if [ $STATUS -ne 0 ]; then ...
| wait $PID || { | |
| echo " Failed!" | |
| handle_error | |
| } | |
| wait "$PID" | |
| STATUS=$? | |
| if [ "$STATUS" -ne 0 ]; then | |
| echo " Failed!" | |
| handle_error | |
| fi |
setup.sh
Outdated
| docker compose run --rm server create_db || handle_error | ||
| else | ||
| echo -n "Downloading images" | ||
| docker compose pull >/dev/null 2>&1 & |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output redirection >/dev/null 2>&1 suppresses all output including error messages from docker compose pull. If the command fails due to network issues, authentication problems, or invalid image references, users won't see any helpful error messages even in the "Failed!" output. This makes troubleshooting difficult without re-running with --debug.
Consider redirecting only stdout while preserving stderr, or at least logging errors to a temporary file that can be displayed when the command fails.
setup.sh
Outdated
|
|
||
| # Error handling function | ||
| handle_error() { | ||
| if [ "$DEBUG" != "yes" ]; then |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent comparison operators: The code uses [ "$DEBUG" != "yes" ] on line 15 but [ "$DEBUG" = "yes" ] on lines 333 and 353. While both are valid POSIX shell syntax, the script elsewhere consistently uses = for equality checks (e.g., line 327 with "x$DONT_START" != "xyes"). For consistency with the existing codebase pattern that prefixes with "x", consider using [ "x$DEBUG" != "xyes" ] on line 15.
| if [ "$DEBUG" != "yes" ]; then | |
| if [ "x$DEBUG" != "xyes" ]; then |
Address all 6 issues identified in Copilot review: 1. Add signal traps to prevent process leaks on interruption 2. Fix race condition in exit status handling with explicit STATUS check 3. Apply consistent progress suppression to database creation step 4. Suppress docker compose up output in non-debug mode for consistency 5. Use consistent comparison operators (x = xyes pattern) 6. Preserve error messages using temporary files while suppressing progress Key improvements: - Robust background process management with proper cleanup - Complete progress output suppression in clean mode - Error details automatically shown on failure (--debug less critical) - Maintains clean UX while providing excellent troubleshooting info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="setup.sh">
<violation number="1" location="setup.sh:389">
P2: Inconsistent error handling: `docker compose up -d` discards stderr, so if it fails users won't see any error details. The previous operations (`docker compose pull`, `docker compose run`) capture and display errors, but this one silently discards them. Consider capturing stderr for consistency with the other docker operations.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
setup.sh
Outdated
| docker compose up -d || handle_error | ||
| else | ||
| echo "Starting containers..." | ||
| docker compose up -d >/dev/null 2>&1 || handle_error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Inconsistent error handling: docker compose up -d discards stderr, so if it fails users won't see any error details. The previous operations (docker compose pull, docker compose run) capture and display errors, but this one silently discards them. Consider capturing stderr for consistency with the other docker operations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At setup.sh, line 389:
<comment>Inconsistent error handling: `docker compose up -d` discards stderr, so if it fails users won't see any error details. The previous operations (`docker compose pull`, `docker compose run`) capture and display errors, but this one silently discards them. Consider capturing stderr for consistency with the other docker operations.</comment>
<file context>
@@ -330,31 +330,63 @@ startup() {
else
echo "Starting containers..."
- docker compose up -d || handle_error
+ docker compose up -d >/dev/null 2>&1 || handle_error
fi
</file context>
| docker compose up -d >/dev/null 2>&1 || handle_error | |
| ERROR_LOG=$(mktemp) | |
| if ! docker compose up -d >/dev/null 2>"$ERROR_LOG"; then | |
| if [ -s "$ERROR_LOG" ]; then | |
| echo "Error details:" | |
| cat "$ERROR_LOG" | |
| fi | |
| rm -f "$ERROR_LOG" | |
| handle_error | |
| fi | |
| rm -f "$ERROR_LOG" |
✅ Addressed in 99420b6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
setup.sh
Outdated
| sleep 2 | ||
| done | ||
| wait "$PID" | ||
| STATUS=$? | ||
| trap - EXIT INT TERM | ||
| if [ "$STATUS" -ne 0 ]; then | ||
| echo " Failed!" | ||
| if [ -s "$ERROR_LOG" ]; then | ||
| echo "Error details:" | ||
| cat "$ERROR_LOG" | ||
| fi | ||
| rm -f "$ERROR_LOG" | ||
| handle_error | ||
| fi | ||
| rm -f "$ERROR_LOG" | ||
| echo " Done!" | ||
| echo -n "Creating database" | ||
| ERROR_LOG=$(mktemp) | ||
| docker compose run --rm server create_db >/dev/null 2>"$ERROR_LOG" & | ||
| PID=$! | ||
| trap 'kill "$PID" 2>/dev/null || true; rm -f "$ERROR_LOG"' EXIT INT TERM | ||
| while kill -0 "$PID" 2>/dev/null; do | ||
| echo -n "." | ||
| sleep 2 |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sleep interval of 2 seconds creates a noticeable delay in showing progress to users. For long-running operations like image downloads, this results in infrequent updates (one dot every 2 seconds), which can make the script appear unresponsive.
Consider reducing the sleep interval to 0.5 or 1 second to provide more responsive visual feedback while still avoiding excessive CPU usage from the polling loop.
| ERROR_LOG=$(mktemp) | ||
| docker compose run --rm server create_db >/dev/null 2>"$ERROR_LOG" & | ||
| PID=$! | ||
| trap 'kill "$PID" 2>/dev/null || true; rm -f "$ERROR_LOG"' EXIT INT TERM | ||
| while kill -0 "$PID" 2>/dev/null; do | ||
| echo -n "." | ||
| sleep 2 | ||
| done | ||
| wait "$PID" | ||
| STATUS=$? | ||
| trap - EXIT INT TERM | ||
| if [ "$STATUS" -ne 0 ]; then | ||
| echo " Failed!" | ||
| if [ -s "$ERROR_LOG" ]; then | ||
| echo "Error details:" | ||
| cat "$ERROR_LOG" | ||
| fi | ||
| rm -f "$ERROR_LOG" | ||
| handle_error | ||
| fi | ||
| rm -f "$ERROR_LOG" |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same temporary file cleanup issue exists here as in the docker compose pull section. The ERROR_LOG temporary file may not be cleaned up properly if the script is interrupted between setting the trap (line 363) and removing it (line 370), or if an unexpected exit occurs before the cleanup on line 380.
Apply the same fix as recommended for the docker compose pull section to ensure proper cleanup in all scenarios.
setup.sh
Outdated
| ERROR_LOG=$(mktemp) | ||
| docker compose pull >/dev/null 2>"$ERROR_LOG" & | ||
| PID=$! | ||
| trap 'kill "$PID" 2>/dev/null || true; rm -f "$ERROR_LOG"' EXIT INT TERM | ||
| while kill -0 "$PID" 2>/dev/null; do | ||
| echo -n "." | ||
| sleep 2 | ||
| done | ||
| wait "$PID" | ||
| STATUS=$? | ||
| trap - EXIT INT TERM | ||
| if [ "$STATUS" -ne 0 ]; then | ||
| echo " Failed!" | ||
| if [ -s "$ERROR_LOG" ]; then | ||
| echo "Error details:" | ||
| cat "$ERROR_LOG" | ||
| fi | ||
| rm -f "$ERROR_LOG" | ||
| handle_error | ||
| fi | ||
| rm -f "$ERROR_LOG" | ||
| echo " Done!" | ||
| echo -n "Creating database" | ||
| ERROR_LOG=$(mktemp) | ||
| docker compose run --rm server create_db >/dev/null 2>"$ERROR_LOG" & | ||
| PID=$! | ||
| trap 'kill "$PID" 2>/dev/null || true; rm -f "$ERROR_LOG"' EXIT INT TERM |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ERROR_LOG variable is reused between the docker compose pull section and the create_db section without proper scoping. While this works because the first ERROR_LOG is cleaned up before the second one is created, the trap statements reference "$ERROR_LOG" which gets evaluated when the trap fires, not when it's set. This means if a trap fires from the first section after ERROR_LOG has been reassigned, it could reference the wrong file.
Consider using unique variable names (e.g., ERROR_LOG_PULL and ERROR_LOG_DB) or ensuring each section's trap is completely cleared before the next section begins to avoid potential confusion and bugs.
Address cubic-dev-ai feedback: docker compose up -d now captures and displays error details consistently with other docker operations. Previously: - docker compose pull/run: captured errors to temp file, displayed details - docker compose up -d: discarded all error output (>/dev/null 2>&1) Now all docker operations consistently capture and display error details while maintaining clean progress output in non-debug mode.
Address Copilot feedback on code duplication: - Extract repeated progress display pattern into run_with_progress() function - Eliminates 40+ lines of duplicated code between pull and create_db operations - Fixes ERROR_LOG variable reuse issue with proper scoping - Improves maintainability and reduces bug risk - Each operation now uses independent ERROR_LOG variables This refactoring maintains identical functionality while significantly improving code quality and maintainability.
Address remaining Copilot feedback: - Reorder help text to match usage line order for better readability - Capture script name at startup to handle symlinks and different invocations - Use SCRIPT_NAME variable in error messages and help text for consistency These improvements enhance user experience and ensure correct script references regardless of how the script is invoked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="setup.sh">
<violation number="1" location="setup.sh:27">
P2: The `local` keyword is not POSIX-compliant. Since this script uses `#!/usr/bin/env sh`, it should avoid bashisms for maximum portability. While `local` works in dash (Debian/Ubuntu) and bash, it may fail on strict POSIX shells. Consider removing `local` and using unique variable names, or change the shebang to `#!/usr/bin/env bash` if bash features are acceptable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Address cubic-dev-ai feedback on POSIX compliance: - Remove 'local' keyword which is a bashism, not POSIX-compliant - Use unique variable names (RWP_MESSAGE, RWP_COMMAND) instead - Maintains compatibility with strict POSIX shells - Script continues to use #!/usr/bin/env sh as intended This ensures the script works on all POSIX-compliant shells, not just bash and dash.
Improve Docker Compose progress output for better user experience
Fixes #96
This PR enhances the setup script's user experience by providing cleaner terminal output during Docker image downloads while maintaining full debugging capabilities when needed.
Problem
The current setup script produces cluttered terminal output during Docker image downloads, making it difficult to follow installation progress:
Example of Current Cluttered Output
Solution
Implemented a dual-mode progress display system:
Default Mode (Clean UX)
Debug Mode (
--debugflag)Shows full Docker Compose progress output for troubleshooting.
Key Features
--debugflagError Handling Enhancement
When installation fails in clean mode, users get actionable guidance:
Testing
Benefits
This enhancement significantly improves the user experience while maintaining full debugging capabilities for advanced users and troubleshooting scenarios.