Feature: add docker installation #14
Conversation
|
Warning Rate limit exceeded@imsudiproy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Runner as auto_verify.sh
participant Container
participant PM as Package Manager
participant Dockerd as Docker daemon
participant Build as Build Script
User->>Runner: trigger verification
Runner->>Container: start & prepare container
alt install_docker == "yes"
Container->>Container: read /etc/os-release -> distro
Container->>PM: add repo(s) & install docker packages
Container->>Container: add host $USER to docker group
Container->>Dockerd: start dockerd (background)
Note right of Dockerd: (wait for readiness)
else
Note right of Container: skip docker install
end
Container->>Build: execute build script (apply patch if present)
Build-->>Container: logs & exit status
Container-->>Runner: collect artifacts & status
Runner-->>User: report outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
auto_verify.sh (2)
10-12: Defaultinstall_dockerto avoid SC2154 and undefined var behavior.ShellCheck warns because the var may be unset. Provide a safe default after sourcing config.
source "$config_file" +# Safe defaults for optional config +: "${install_docker:=no}" +
136-141: Ensure target user exists beforesu - test.Most base images do not have a
testuser. Create it when requested, and add todockergroup if Docker was installed.- if [ "$user" == "test" ]; then + if [ "$user" == "test" ]; then + docker exec "$container_id" bash -c 'id -u test >/dev/null 2>&1 || useradd -m test' + if [ "$install_docker" == "yes" ]; then + docker exec "$container_id" bash -c 'getent group docker >/dev/null 2>&1 || groupadd docker; usermod -aG docker test' + fi docker exec "$container_id" su - test -c "bash $script_path -$build_arg" &> "$log_file"
🧹 Nitpick comments (13)
config.txt (1)
7-7: Defaultinstall_dockerto "no" in committed config.Checking in
"yes"will slow every run and surprise users. Make "no" the default in the template; override locally when needed.-install_docker="yes" #set yes or no +install_docker="no" # set yes to install Docker inside the containerREADME.md (5)
13-13: Tighten prerequisite wording.Minor grammar tweak for consistency.
-- Permissions to execute scripts and create Docker containers +- Permission to execute scripts and create Docker containers
32-41: Document privileged requirement and distro caveats.Installing/running dockerd inside a container requires
--privileged. Also call out CentOS 7/yum and openSUSE vs SLES differences.| `install_docker` | Install Docker inside the container before running script | `no` or `yes` | +> Note: Requires containers to be started with `--privileged`. CentOS 7 uses `yum` (not `dnf`); openSUSE differs from SLES repos.
65-66: Mention--privilegedin the flow.Readers need to know why dockerd can start in the container.
- - Optionally installs Docker inside the container (`install_docker="yes"`) + - Optionally installs Docker inside the container (`install_docker="yes"`, container started with `--privileged`)
72-84: Minor list formatting/grammar."3. The script will:" reads a bit off in context; also keep tense consistent.
-3. The script will: +Then the script will: - Validate the patch file exists - Copy the patch to the container - - The patch will be placed in the same directory as the build script + - Place the patch in the same directory as the build script
116-118: Troubleshooting entry is good; add CentOS/openSUSE note.Helps set user expectations.
-6. **"Unsupported distro for Docker install"** - - The script only supports Docker installation for Ubuntu, RHEL/CentOS, and SLES/SUSE. Ensure your Docker image is based on one of these distros if you want to use the Docker installation feature. +6. **"Unsupported distro for Docker install"** + - The script supports Ubuntu, RHEL/CentOS, and SLES/openSUSE. CentOS 7 may require `yum` instead of `dnf`; openSUSE uses different repos than SLES.auto_verify.sh (7)
55-59: Pre-clean any existing container with the same name.Avoid "Conflict. The container name is already in use" on reruns.
- container_id=$(docker run -d --privileged --name "$container_name" "$image_name") + # Best-effort cleanup if a stale container exists + docker rm -f "$container_name" >/dev/null 2>&1 || true + container_id=$(docker run -d --privileged --name "$container_name" "$image_name")
131-135: Remove dead/commented code or explain why it stays.Keep the script lean unless you intend to toggle between strategies.
91-129: Optional: split distro cases more precisely.
rhel|centosandsles|susecollapse repos that diverge (e.g., CentOS 7 vs RHEL, openSUSE vs SLES). Consider separate cases to reduce failures.
25-28: Host logs path under/rootmay require root.If non-root runs this script, writing to
/root/logsfails. Consider$PWD/logs.-log_dir="/root/logs" +log_dir="${LOG_DIR:-$PWD/logs}"
162-165: Parallelize images to speed runs (optional).For many images, background the jobs and wait.
-for image_name in "${images[@]}"; do - echo "Testing image: $image_name" - run_verification "$image_name" -done +for image_name in "${images[@]}"; do + echo "Testing image: $image_name" + run_verification "$image_name" & +done +wait
39-45: Nit:build_argvalues are opaque.
yvsytis cryptic. Consider--run-testsflag pass-through for clarity.
85-135: Alternative approach: prefer socket bind-mount over DinD.Consider bind-mounting the host Docker socket (
-v /var/run/docker.sock:/var/run/docker.sock) for most cases; reserve DinD for true daemon isolation (e.g., buildx/qemu). This reduces flakiness and install time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
README.md(5 hunks)auto_verify.sh(1 hunks)config.txt(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
auto_verify.sh
[warning] 86-86: install_docker is referenced but not assigned.
(SC2154)
🪛 LanguageTool
README.md
[grammar] ~11-~11: There might be a mistake here.
Context: ...quisites - Docker installed and running - Bash shell environment - Permissions to ...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...led and running - Bash shell environment - Permissions to execute scripts and creat...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...ecify the path to your patch file using patch_path 3. The script will: - Validate the patch...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ...e using patch_path 3. The script will: - Validate the patch file exists - Copy...
(QB_NEW_EN)
🔇 Additional comments (3)
README.md (3)
7-7: Good overview update.Clear callout that Docker install inside containers is optional. LGTM.
52-53: Match the template default with the committed config.Keep defaults consistent: both should show
install_docker="no".
85-90: Nice new section.Clear scope and supported distros are helpful.
| #install docker | ||
| if [ "$install_docker" == "yes" ]; then | ||
| echo "Installing Docker inside container: $container_id" | tee -a "$log_file" | ||
|
|
||
| distro=$(docker exec "$container_id" sh -c 'grep "^ID=" /etc/os-release | cut -d= -f2' | tr -d '"') | ||
|
|
||
| case "$distro" in | ||
| ubuntu) | ||
| docker exec "$container_id" bash -c " | ||
| apt-get update && | ||
| apt-get install -y ca-certificates curl gnupg sudo && | ||
| install -m 0755 -d /etc/apt/keyrings && | ||
| curl -fsSL https://download.docker.com/linux/ubuntu/gpg -o /etc/apt/keyrings/docker.asc && | ||
| chmod a+r /etc/apt/keyrings/docker.asc && | ||
| echo \"deb [arch=\$(dpkg --print-architecture) signed-by=/etc/apt/keyrings/docker.asc] https://download.docker.com/linux/ubuntu \$(. /etc/os-release && echo \${UBUNTU_CODENAME:-\$VERSION_CODENAME}) stable\" > /etc/apt/sources.list.d/docker.list && | ||
| apt-get update && | ||
| apt-get install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin && | ||
| sudo usermod -aG docker $USER && newgrp docker | ||
| sudo dockerd & && | ||
| sleep 30 | ||
| " | ||
| ;; | ||
| rhel|centos) | ||
| docker exec "$container_id" bash -c " | ||
| sudo dnf -y install dnf-plugins-core sudo && | ||
| sudo dnf config-manager --add-repo https://download.docker.com/linux/rhel/docker-ce.repo && | ||
| sudo dnf install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin && | ||
| sudo usermod -aG docker $USER && newgrp docker | ||
| sudo dockerd & && | ||
| sleep 30 | ||
| " | ||
| ;; | ||
| sles|suse) | ||
| docker exec "$container_id" bash -c " | ||
| sudo zypper addrepo https://download.docker.com/linux/sles/docker-ce.repo && | ||
| sudo zypper install docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin sudo && | ||
| sudo usermod -aG docker $USER && newgrp docker | ||
| sudo dockerd & && | ||
| sleep 30 | ||
| " | ||
| ;; | ||
| *) | ||
| echo "Unsupported distro for Docker install: $distro" | tee -a "$log_file" | ||
| ;; | ||
| esac | ||
|
|
||
| # # Start Docker daemon | ||
| # docker exec -d "$container_id" sh -c "dockerd &" | ||
| # sleep 5 | ||
| fi | ||
|
|
There was a problem hiding this comment.
Fix: $USER expands on the host, newgrp breaks non-interactive scripts, and sleeps are brittle.
$USERinside the quoted here-doc expands on the host, not in the container. That user often doesn't exist inside the container.newgrp dockeropens a new shell and can hang/skip subsequent lines.- Use a deterministic wait loop on
docker infoinstead of fixedsleep 30. sudoinside containers is unnecessary (commands run as root by default).
Apply this minimal patch in all three distro branches.
- apt-get install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin &&
- sudo usermod -aG docker $USER && newgrp docker
- sudo dockerd & &&
- sleep 30
+ apt-get install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin &&
+ target_user='"${user:-}"'; if [ -n "$target_user" ] && id -u "$target_user" >/dev/null 2>&1; then usermod -aG docker "$target_user"; fi
+ nohup dockerd > /var/log/dockerd.log 2>&1 & disown
+ i=0; until docker info >/dev/null 2>&1 || [ $i -ge 30 ]; do i=$((i+1)); sleep 1; done
@@
- sudo dnf -y install dnf-plugins-core sudo &&
- sudo dnf config-manager --add-repo https://download.docker.com/linux/rhel/docker-ce.repo &&
- sudo dnf install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin &&
- sudo usermod -aG docker $USER && newgrp docker
- sudo dockerd & &&
- sleep 30
+ dnf -y install dnf-plugins-core sudo || yum -y install yum-utils sudo &&
+ (dnf config-manager --add-repo https://download.docker.com/linux/rhel/docker-ce.repo || \
+ yum-config-manager --add-repo https://download.docker.com/linux/centos/docker-ce.repo) &&
+ (dnf install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin || \
+ yum install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin) &&
+ target_user='"${user:-}"'; if [ -n "$target_user" ] && id -u "$target_user" >/dev/null 2>&1; then usermod -aG docker "$target_user"; fi
+ nohup dockerd > /var/log/dockerd.log 2>&1 & disown
+ i=0; until docker info >/dev/null 2>&1 || [ $i -ge 30 ]; do i=$((i+1)); sleep 1; done
@@
- sudo zypper addrepo https://download.docker.com/linux/sles/docker-ce.repo &&
- sudo zypper install docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin sudo &&
- sudo usermod -aG docker $USER && newgrp docker
- sudo dockerd & &&
- sleep 30
+ zypper -n addrepo https://download.docker.com/linux/sles/docker-ce.repo docker-ce || \
+ zypper -n addrepo https://download.docker.com/linux/opensuse/docker-ce.repo docker-ce &&
+ zypper -n install docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin sudo ||
+ zypper -n install docker docker-cli containerd sudo &&
+ target_user='"${user:-}"'; if [ -n "$target_user" ] && id -u "$target_user" >/dev/null 2>&1; then usermod -aG docker "$target_user"; fi
+ nohup dockerd > /var/log/dockerd.log 2>&1 & disown
+ i=0; until docker info >/dev/null 2>&1 || [ $i -ge 30 ]; do i=$((i+1)); sleep 1; doneCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 86-86: install_docker is referenced but not assigned.
(SC2154)
🤖 Prompt for AI Agents
In auto_verify.sh lines 85-135, the branch installs use host-expanded $USER,
unnecessary sudo, blocking newgrp, and fixed sleep; fix by removing sudo, avoid
host $USER by resolving the container username inside the exec (e.g. use $(id
-un) or run usermod against the container's user via docker exec "$(id -un)"
resolved inside the container), remove newgrp entirely, start dockerd in
background non-interactively (e.g. nohup dockerd >/dev/null 2>&1 & or run
dockerd & disowned inside the container), and replace sleep 30 with a
deterministic wait loop that checks docker readiness (until docker info
>/dev/null 2>&1; do sleep 1; done); apply same changes to ubuntu, rhel|centos,
and sles|suse branches.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
auto_verify.sh (4)
93-105: Blocking dockerd, host-expanded $USER, and unnecessary sudo/newgrp on Ubuntu path
- dockerd is started in the foreground and will block the script.
- $USER expands on the host, not inside the container.
- newgrp spawns a subshell and breaks non-interactive flows.
- sudo is redundant inside the container (root by default).
- docker exec "$container_id" bash -c " - apt-get update && - apt-get install -y ca-certificates curl gnupg sudo && - install -m 0755 -d /etc/apt/keyrings && - curl -fsSL https://download.docker.com/linux/ubuntu/gpg -o /etc/apt/keyrings/docker.asc && - chmod a+r /etc/apt/keyrings/docker.asc && - echo \"deb [arch=\$(dpkg --print-architecture) signed-by=/etc/apt/keyrings/docker.asc] https://download.docker.com/linux/ubuntu \$(. /etc/os-release && echo \${UBUNTU_CODENAME:-\$VERSION_CODENAME}) stable\" > /etc/apt/sources.list.d/docker.list && - apt-get update && - apt-get install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin && - sudo usermod -aG docker $USER && newgrp docker - " - docker exec "$container_id" sh -c "sudo dockerd" + docker exec "$container_id" bash -lc ' + set -e + apt-get update && + apt-get install -y ca-certificates curl gnupg && + install -m 0755 -d /etc/apt/keyrings && + curl -fsSL https://download.docker.com/linux/ubuntu/gpg -o /etc/apt/keyrings/docker.asc && + chmod a+r /etc/apt/keyrings/docker.asc && + echo "deb [arch=$(dpkg --print-architecture) signed-by=/etc/apt/keyrings/docker.asc] https://download.docker.com/linux/ubuntu $(. /etc/os-release && echo ${UBUNTU_CODENAME:-$VERSION_CODENAME}) stable" > /etc/apt/sources.list.d/docker.list && + apt-get update && + apt-get install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin && + target_user='"${user:-}"'; if [ -n "$target_user" ] && id -u "$target_user" >/dev/null 2>&1; then usermod -aG docker "$target_user"; fi + ' 2>&1 | tee -a "$log_file" + docker exec -d "$container_id" sh -c 'nohup dockerd > /var/log/dockerd.log 2>&1 &'
106-114: RHEL/CentOS: handle yum fallback, drop sudo/newgrp, prevent dockerd blockingdnf may not exist on CentOS 7; add yum-utils fallback. Also same $USER/newgrp/blocking issues as above.
- docker exec "$container_id" bash -c " - sudo dnf -y install dnf-plugins-core sudo && - sudo dnf config-manager --add-repo https://download.docker.com/linux/rhel/docker-ce.repo && - sudo dnf install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin && - sudo usermod -aG docker $USER && newgrp docker - " - docker exec "$container_id" sh -c "sudo dockerd" + docker exec "$container_id" bash -lc ' + set -e + dnf -y install dnf-plugins-core || yum -y install yum-utils && + (dnf config-manager --add-repo https://download.docker.com/linux/rhel/docker-ce.repo || \ + yum-config-manager --add-repo https://download.docker.com/linux/centos/docker-ce.repo) && + (dnf install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin || \ + yum install -y docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin) && + target_user='"${user:-}"'; if [ -n "$target_user" ] && id -u "$target_user" >/dev/null 2>&1; then usermod -aG docker "$target_user"; fi + ' 2>&1 | tee -a "$log_file" + docker exec -d "$container_id" sh -c 'nohup dockerd > /var/log/dockerd.log 2>&1 &'
115-123: SLES/SUSE: repo/install fallbacks, drop sudo/newgrp, prevent dockerd blockingSLES/Leap repo names vary; add opensuse fallback and fall back to docker/docker-cli if docker-ce is unavailable.
- docker exec "$container_id" bash -c " - sudo zypper addrepo https://download.docker.com/linux/sles/docker-ce.repo && - sudo zypper install docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin sudo && - sudo usermod -aG docker $USER && newgrp docker - " - docker exec "$container_id" sh -c "sudo dockerd" + docker exec "$container_id" bash -lc ' + set -e + zypper -n addrepo https://download.docker.com/linux/sles/docker-ce.repo docker-ce || \ + zypper -n addrepo https://download.docker.com/linux/opensuse/docker-ce.repo docker-ce && + zypper -n install docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin || \ + zypper -n install docker docker-cli containerd && + target_user='"${user:-}"'; if [ -n "$target_user" ] && id -u "$target_user" >/dev/null 2>&1; then usermod -aG docker "$target_user"; fi + ' 2>&1 | tee -a "$log_file" + docker exec -d "$container_id" sh -c 'nohup dockerd > /var/log/dockerd.log 2>&1 &'
128-131: Replace brittle sleeps with deterministic readiness checkWait for the daemon to become responsive instead of sleeping. This also centralizes readiness after distro-specific setup.
- # # Start Docker daemon - # docker exec -d "$container_id" sh -c "dockerd &" - # sleep 5 + # Wait for Docker in the container to be ready (max ~30s) + i=0 + until docker exec "$container_id" docker info >/dev/null 2>&1 || [ $i -ge 30 ]; do + i=$((i+1)) + sleep 1 + done + if [ $i -ge 30 ]; then + echo "Docker in container did not become ready in time." | tee -a "$log_file" + docker rm -f "$container_id" + return 1 + fi
🧹 Nitpick comments (2)
auto_verify.sh (2)
89-92: Broaden distro detection (Rocky/Alma/OpenSUSE Leap) or use ID_LIKESeveral popular downstreams won’t match the current case labels. Consider matching rhel|centos|rocky|almalinux and sles|suse|opensuse*, or parse ID_LIKE to route families.
Example:
- rhel-family: rhel|centos|rocky|almalinux
- suse-family: sles|suse|opensuse*
133-139: Capture Docker install output to logs and quote script_pathPipe docker exec output to the same log file and quote the path to avoid issues with whitespace.
- echo "Started executing the provided script..." + echo "Started executing the provided script..." | tee -a "$log_file" @@ - docker exec "$container_id" bash $script_path -$build_arg &> "$log_file" + docker exec "$container_id" bash "$script_path" -$build_arg &> "$log_file"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
auto_verify.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
auto_verify.sh
[warning] 86-86: install_docker is referenced but not assigned.
(SC2154)
| if [ "$install_docker" == "yes" ]; then | ||
| echo "Installing Docker inside container: $container_id" | tee -a "$log_file" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against unset install_docker (SC2154) and prefer = over ==
Avoid unbound var warnings and make the check robust when the key is missing in config.txt.
- if [ "$install_docker" == "yes" ]; then
+ if [ "${install_docker:-no}" = "yes" ]; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "$install_docker" == "yes" ]; then | |
| echo "Installing Docker inside container: $container_id" | tee -a "$log_file" | |
| if [ "${install_docker:-no}" = "yes" ]; then | |
| echo "Installing Docker inside container: $container_id" | tee -a "$log_file" |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 86-86: install_docker is referenced but not assigned.
(SC2154)
🤖 Prompt for AI Agents
In auto_verify.sh around lines 86 to 87, the if condition uses an unguarded
variable and the non-POSIX == operator; change the test to guard against unset
install_docker and use = by replacing the condition with a quoted
parameter-expansion default, e.g. if [ "${install_docker:-}" = "yes" ]; then, so
the script won't emit unbound-var warnings and will use the portable
string-equality operator.
|
This PR fixes #15 |
This PR will fix #15
Summary by CodeRabbit
New Features
Documentation
Chores