Skip to content

Conversation

@snickerjp
Copy link
Member

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:

  • Before: Hundreds of progress bar lines during image downloads
  • Visual noise: Multiple parallel layer downloads create overwhelming output
  • Poor UX: Important messages can be buried in progress output

Example of Current Cluttered Output

59e22667830b [=========>                                         ]  5.243MB/28.23MB 
3fa1124e7cf3 [=============>                                     ]  4.194MB/15.65MB
12b8240e46e9 [=============================>                     ]  2.097MB/3.514MB
ebf5995699c9 [>                                                  ]  1.049MB/521.4MB
... (hundreds more lines)

Solution

Implemented a dual-mode progress display system:

Default Mode (Clean UX)

** Initialising Redash database **
Downloading images............. Done!
Creating database...
[clean database creation output]
** Starting the rest of Redash **
Starting containers...
[clean container startup output]

Debug Mode (--debug flag)

./setup.sh --debug

Shows full Docker Compose progress output for troubleshooting.

Key Features

  • Clean default experience: Dot-based progress indication
  • Detailed debugging: Full output available with --debug flag
  • Error handling: Helpful error messages with troubleshooting guidance
  • Consistent UX: Maintains important operational feedback
  • Backward compatibility: All existing functionality preserved

Error Handling Enhancement

When installation fails in clean mode, users get actionable guidance:

❌ An error occurred during installation.
💡 For detailed output, please run: ./setup.sh --debug

Testing

  • Default mode: Clean output tested on Debian 12 and Ubuntu 22.04
  • Debug mode: Full progress output verified
  • Error scenarios: Error handling and guidance messages tested
  • Functionality: All installation steps work correctly in both modes

Benefits

  • Improved accessibility: Cleaner output for screen readers
  • Better logs: Reduced log file size and improved readability
  • Enhanced troubleshooting: Clear path to detailed output when needed
  • Professional UX: Clean, modern installation experience

This enhancement significantly improves the user experience while maintaining full debugging capabilities for advanced users and troubleshooting scenarios.

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.
Copilot AI review requested due to automatic review settings December 31, 2025 16:50
Copy link

@cubic-dev-ai cubic-dev-ai bot left a 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

Copy link

Copilot AI left a 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 --debug flag (-g) to toggle between clean progress dots and full Docker output
  • Implements background job execution for docker compose pull with dot-based progress indication
  • Introduces an error handling function that suggests using --debug for troubleshooting

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

setup.sh Outdated
Comment on lines 339 to 346
while kill -0 $PID 2>/dev/null; do
echo -n "."
sleep 2
done
wait $PID || {
echo " Failed!"
handle_error
}
Copy link

Copilot AI Dec 31, 2025

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
setup.sh Outdated
docker compose up -d || handle_error
else
echo "Starting containers..."
docker compose up -d || handle_error
Copy link

Copilot AI Dec 31, 2025

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.

Suggested change
docker compose up -d || handle_error
docker compose up -d >/dev/null 2>&1 || handle_error

Copilot uses AI. Check for mistakes.
setup.sh Outdated
Comment on lines 348 to 349
echo "Creating database..."
docker compose run --rm server create_db || handle_error
Copy link

Copilot AI Dec 31, 2025

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.

Suggested change
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!"

Copilot uses AI. Check for mistakes.
setup.sh Outdated
Comment on lines 343 to 346
wait $PID || {
echo " Failed!"
handle_error
}
Copy link

Copilot AI Dec 31, 2025

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 ...

Suggested change
wait $PID || {
echo " Failed!"
handle_error
}
wait "$PID"
STATUS=$?
if [ "$STATUS" -ne 0 ]; then
echo " Failed!"
handle_error
fi

Copilot uses AI. Check for mistakes.
setup.sh Outdated
docker compose run --rm server create_db || handle_error
else
echo -n "Downloading images"
docker compose pull >/dev/null 2>&1 &
Copy link

Copilot AI Dec 31, 2025

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.

Copilot uses AI. Check for mistakes.
setup.sh Outdated

# Error handling function
handle_error() {
if [ "$DEBUG" != "yes" ]; then
Copy link

Copilot AI Dec 31, 2025

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.

Suggested change
if [ "$DEBUG" != "yes" ]; then
if [ "x$DEBUG" != "xyes" ]; then

Copilot uses AI. Check for mistakes.
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
Copy link

@cubic-dev-ai cubic-dev-ai bot left a 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&#39;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
Copy link

@cubic-dev-ai cubic-dev-ai bot Dec 31, 2025

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&#39;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 &quot;Starting containers...&quot;
-			docker compose up -d || handle_error
+			docker compose up -d &gt;/dev/null 2&gt;&amp;1 || handle_error
 		fi
 
</file context>
Suggested change
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

Copy link

Copilot AI left a 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
Comment on lines 343 to 366
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
Copy link

Copilot AI Dec 31, 2025

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.

Copilot uses AI. Check for mistakes.
Comment on lines 360 to 380
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"
Copy link

Copilot AI Dec 31, 2025

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.

Copilot uses AI. Check for mistakes.
setup.sh Outdated
Comment on lines 337 to 363
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
Copy link

Copilot AI Dec 31, 2025

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.

Copilot uses AI. Check for mistakes.
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.
Copy link

@cubic-dev-ai cubic-dev-ai bot left a 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve Docker Compose progress output for better user experience

1 participant