fix: smarter container engine detection in lint-container.sh#441
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![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
🧹 Nitpick comments (2)
scripts/lint-container.sh (2)
47-47: Redundant stderr redirection and doubledocker infoinvocation.
&>/dev/nullalready redirects both stdout and stderr, so2>&1is redundant. Additionally,docker infois executed twice—once in the condition check (line 47) and again for the grep (line 48). Consider caching the output to avoid the duplicate call.♻️ Suggested improvement
- if command -v docker &>/dev/null && docker info &>/dev/null 2>&1; then - if docker info 2>/dev/null | grep -qi podman; then + if command -v docker &>/dev/null; then + local docker_info + docker_info=$(docker info 2>/dev/null) || true + if [ -n "$docker_info" ]; then + if echo "$docker_info" | grep -qi podman; then(You'll need to adjust the closing braces/
fiaccordingly.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lint-container.sh` at line 47, The conditional currently calls docker info twice and uses redundant redirection; change it to call docker info once, capture its output/status into a variable (e.g., DOCKER_INFO or docker_info) with stderr redirected once, then use command -v docker && [ -n "$DOCKER_INFO" ] (or test the captured output) and reuse that variable for the subsequent grep instead of invoking docker info again; update the if/fi block accordingly so only a single docker info call is made and remove the redundant 2>&1 redirection.
45-62: Inconsistent engine-selection priority withcontainer-base.sh.The relevant context snippet shows that
scripts/container-base.sh(sourced bybench-container.sh,fix-container.sh, andtest-container.sh) uses Podman-first logic, while this script now uses Docker-first logic. This inconsistency could surprise users who expect uniform behavior across the container scripts.If the Docker-first approach is intentional for linting due to buildx requirements, consider documenting this divergence in the script header or unifying the detection logic into a shared helper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lint-container.sh` around lines 45 - 62, The lint-container engine detection uses Docker-first while container-base uses Podman-first, causing inconsistent behavior; either make lint-container follow the same Podman-first priority (change the Docker-first detection block to the same logic used in container-base) or extract the selection logic into a shared helper function (e.g., detect_container_engine) that both lint-container and container-base call; if Docker-first is intentional for linting, add a short header comment in lint-container explaining the divergence and reference the shared helper location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/lint-container.sh`:
- Around line 48-56: When the check docker info | grep -qi podman detects
Podman-as-Docker but command -v podman fails, do not fall back to echoing
"docker"; instead emit a clear warning/error and exit non-zero so callers know
the environment is unsupported. Update the branch that currently runs after the
command -v podman check (the block following docker info | grep -qi podman) to
log a descriptive message (e.g., "Podman detected as docker CLI but no
standalone podman binary found; buildx incompatible") and exit 1 rather than
returning "docker", keeping the existing successful path (echo "podman")
unchanged.
---
Nitpick comments:
In `@scripts/lint-container.sh`:
- Line 47: The conditional currently calls docker info twice and uses redundant
redirection; change it to call docker info once, capture its output/status into
a variable (e.g., DOCKER_INFO or docker_info) with stderr redirected once, then
use command -v docker && [ -n "$DOCKER_INFO" ] (or test the captured output) and
reuse that variable for the subsequent grep instead of invoking docker info
again; update the if/fi block accordingly so only a single docker info call is
made and remove the redundant 2>&1 redirection.
- Around line 45-62: The lint-container engine detection uses Docker-first while
container-base uses Podman-first, causing inconsistent behavior; either make
lint-container follow the same Podman-first priority (change the Docker-first
detection block to the same logic used in container-base) or extract the
selection logic into a shared helper function (e.g., detect_container_engine)
that both lint-container and container-base call; if Docker-first is intentional
for linting, add a short header comment in lint-container explaining the
divergence and reference the shared helper location.
| if docker info 2>/dev/null | grep -qi podman; then | ||
| # Docker CLI is actually a Podman alias — use podman directly | ||
| if command -v podman &>/dev/null; then | ||
| echo "podman" | ||
| return | ||
| fi | ||
| fi | ||
| echo "docker" | ||
| return |
There was a problem hiding this comment.
Edge case: Podman-as-Docker detected but no standalone podman binary.
When Podman masquerades as Docker but no separate podman binary exists (line 50 fails), the code falls through to line 55 and returns "docker". This still invokes the Podman-aliased docker CLI, which the PR aims to avoid due to buildx driver networking issues (#424, #435).
Consider logging a warning or erroring out in this scenario, since the buildx compatibility problem will likely persist.
💡 Possible approach
if command -v podman &>/dev/null; then
echo "podman"
return
fi
+ echo "Warning: Docker CLI is Podman alias, but 'podman' binary not found; buildx issues may occur" >&2
fi
echo "docker"📝 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 docker info 2>/dev/null | grep -qi podman; then | |
| # Docker CLI is actually a Podman alias — use podman directly | |
| if command -v podman &>/dev/null; then | |
| echo "podman" | |
| return | |
| fi | |
| fi | |
| echo "docker" | |
| return | |
| if docker info 2>/dev/null | grep -qi podman; then | |
| # Docker CLI is actually a Podman alias — use podman directly | |
| if command -v podman &>/dev/null; then | |
| echo "podman" | |
| return | |
| fi | |
| echo "Warning: Docker CLI is Podman alias, but 'podman' binary not found; buildx issues may occur" >&2 | |
| fi | |
| echo "docker" | |
| return |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lint-container.sh` around lines 48 - 56, When the check docker info |
grep -qi podman detects Podman-as-Docker but command -v podman fails, do not
fall back to echoing "docker"; instead emit a clear warning/error and exit
non-zero so callers know the environment is unsupported. Update the branch that
currently runs after the command -v podman check (the block following docker
info | grep -qi podman) to log a descriptive message (e.g., "Podman detected as
docker CLI but no standalone podman binary found; buildx incompatible") and exit
1 rather than returning "docker", keeping the existing successful path (echo
"podman") unchanged.
1 participant
Summary
docker info | grep podmanand switches topodmandirectly to avoid buildx driver issuesCONTAINER_ENGINEenv var override still bypasses everythingCloses #435
Test plan
CONTAINER_ENGINE=podmanoverride → picks Podmanpodmandirectly (cc @taqtiqa-mark)🤖 Generated with Claude Code
Summary by CodeRabbit