Enhance docker permission for the scripts#2209
Enhance docker permission for the scripts#2209cb-github-robot merged 1 commit intocloud-barista:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances Docker permission handling in setup scripts by adding comprehensive Docker access testing and providing clearer user guidance for permission-related issues during first-time setup.
Key changes:
- Implements Docker access verification that tests both regular and sudo-based access
- Introduces
DOCKER_NEEDS_SUDOflag to track Docker permission status - Provides conditional user instructions based on detected Docker access capabilities
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/set-tb.sh | Adds Docker access testing section and conditional permission guidance for CB-Tumblebug setup |
| scripts/set-mcmp.sh | Adds Docker access testing, conditional installAll.sh execution logic, and permission guidance for MC-Admin-CLI setup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/set-mcmp.sh
Outdated
| elif sudo docker ps &>/dev/null; then | ||
| echo "⚠️ Docker requires sudo (group membership not yet active)" | ||
| echo " → This is normal for first-time setup" | ||
| echo " → Will use 'sg docker' command to run installAll.sh" |
There was a problem hiding this comment.
The comment states "Will use 'sg docker' command to run installAll.sh" but the actual implementation at line 140 uses su - "$TARGET_USER" instead of sg docker. This inconsistency between documentation and implementation can confuse maintainers.
Either update the comment to reflect the actual implementation using su -, or update line 140 to use sg docker as documented.
| echo " → Will use 'sg docker' command to run installAll.sh" | |
| echo " → Will use 'su - \"$TARGET_USER\"' to run installAll.sh" |
scripts/set-mcmp.sh
Outdated
| echo " → Running installAll.sh (Docker commands will use sudo internally)..." | ||
| echo " → Note: mc-admin-cli's installAll.sh handles Docker permissions automatically" | ||
| # installAll.sh will detect Docker access and use sudo if needed | ||
| su - "$TARGET_USER" -c "cd $MCMP_DIR/bin && ./installAll.sh --mode dev --run background" |
There was a problem hiding this comment.
The $MCMP_DIR variable inside the command string is not quoted. If the directory path contains spaces (e.g., if TARGET_HOME contains spaces), the cd command will fail. For consistency with line 133 which properly quotes the path, this should be fixed.
Change to: su - "$TARGET_USER" -c "cd \"$MCMP_DIR/bin\" && ./installAll.sh --mode dev --run background"
or use single quotes with variable substitution:
su - "$TARGET_USER" -c 'cd "'"$MCMP_DIR"'/bin" && ./installAll.sh --mode dev --run background'
| su - "$TARGET_USER" -c "cd $MCMP_DIR/bin && ./installAll.sh --mode dev --run background" | |
| su - "$TARGET_USER" -c "cd \"$MCMP_DIR/bin\" && ./installAll.sh --mode dev --run background" |
Signed-off-by: Seokho Son <shsongist@gmail.com>
|
/approve |
No description provided.