fix: skip Docker package removal when Docker is already installed - #7732
fix: skip Docker package removal when Docker is already installed#7732HamzaSwitch wants to merge 2 commits into
Conversation
|
|
WalkthroughThe 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker/formbricks.sh`:
- Around line 16-21: The current guard uses the plain "docker info" call which
can fail with a permission error even when the daemon is running; update the
conditional around the docker check so it falls back to invoking "sudo docker
info" if the unprivileged "docker info" fails (i.e., change the if condition
that currently references "command -v docker" and "docker info" to try "docker
info || sudo docker info"), ensuring the script treats Docker as running in that
fallback case and therefore does not run the removal step that calls apt-get
remove.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef124844-1a5e-4fed-bd9c-ebe8b169bc18
📒 Files selected for processing (1)
docker/formbricks.sh
| if command -v docker &> /dev/null && docker info &> /dev/null; then | ||
| echo "✅ Docker is already installed and running. Skipping removal of old packages." | ||
| else | ||
| echo "🧹 Removing old Docker installations." | ||
| sudo apt-get remove docker docker-engine docker.io containerd runc >/dev/null 2>&1 || true | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect Docker detection/removal lines:"
rg -n 'command -v docker|docker info|sudo docker info|apt-get remove docker' docker/formbricks.sh
echo
echo "Check for sudo fallback in detection:"
if rg -n 'sudo[[:space:]]+docker[[:space:]]+info' docker/formbricks.sh >/dev/null; then
echo "Found sudo fallback for docker info."
else
echo "No sudo fallback found -> possible false negative when user lacks docker group access."
fiRepository: formbricks/formbricks
Length of output: 401
🏁 Script executed:
cat -n docker/formbricks.sh | head -50Repository: formbricks/formbricks
Length of output: 2486
🏁 Script executed:
rg -B 5 -A 10 'command -v docker' docker/formbricks.shRepository: formbricks/formbricks
Length of output: 721
🏁 Script executed:
# Search for any references to issue `#7187` or related Docker safety discussions
rg -i '#7187|docker.*remove|destructive' docker/formbricks.sh
# Check if there are any related issues or tests mentioning Docker removal safety
fd -type f \( -name "*.md" -o -name "*.txt" -o -name "*.sh" \) | xargs rg -l '7187|docker.*remove.*destructive' 2>/dev/null | head -10Repository: formbricks/formbricks
Length of output: 586
🏁 Script executed:
# Verify docker info behavior: does it actually fail with non-zero exit when user lacks docker group membership?
# We can check Docker's documented behavior
echo "Checking Docker CLI documentation/behavior patterns..."
# Search the codebase for any context on Docker setup or docker group handling
rg -i 'docker.*group|usermod.*docker|docker.*permission' docker/formbricks.shRepository: formbricks/formbricks
Length of output: 299
Prevent destructive Docker removal when daemon is running but inaccessible to current user.
The guard condition at line 16 uses docker info without a sudo fallback. On fresh instances where Docker daemon is already running but the user lacks docker group membership (not yet added by the script), docker info fails with a permission error. This causes the script to incorrectly remove Docker packages, contradicting the explicit intent documented in the comment at lines 13-15 and issue #7187.
Proposed fix
-if command -v docker &> /dev/null && docker info &> /dev/null; then
- echo "✅ Docker is already installed and running. Skipping removal of old packages."
-else
- echo "🧹 Removing old Docker installations."
- sudo apt-get remove docker docker-engine docker.io containerd runc >/dev/null 2>&1 || true
-fi
+if command -v docker >/dev/null 2>&1; then
+ if docker info >/dev/null 2>&1 || sudo docker info >/dev/null 2>&1; then
+ echo "✅ Docker is already installed and running. Skipping removal of old packages."
+ else
+ echo "⚠️ Docker is installed but daemon is not reachable with current privileges. Skipping package removal to avoid deleting an existing Docker setup."
+ fi
+else
+ echo "🧹 Removing old Docker installations."
+ sudo apt-get remove docker docker-engine docker.io containerd runc >/dev/null 2>&1 || true
+fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker/formbricks.sh` around lines 16 - 21, The current guard uses the plain
"docker info" call which can fail with a permission error even when the daemon
is running; update the conditional around the docker check so it falls back to
invoking "sudo docker info" if the unprivileged "docker info" fails (i.e.,
change the if condition that currently references "command -v docker" and
"docker info" to try "docker info || sudo docker info"), ensuring the script
treats Docker as running in that fallback case and therefore does not run the
removal step that calls apt-get remove.
92b2009 to
5a68c86
Compare
BhagyaAmarasinghe
left a comment
BhagyaAmarasinghe
left a comment
There was a problem hiding this comment.
Thanks for working on this fix! The direction makes sense, and the added check is an improvement over the current behavior.
I do not think this fully closes #7187 yet, though. Right now the script only skips package removal when Docker is already installed and the daemon is reachable. If Docker is installed but not currently running, it still falls back to removing packages without confirmation. Also, even in the “skip removal” path, the script still proceeds with an unconditional Docker install later, which can still modify or replace an existing Docker setup.
I think the safer end state for this issue is: if Docker is already present, do not remove or replace it without explicit user confirmation.
A straightforward way to get there would be to branch earlier on command -v docker:
- If Docker is already installed, skip the package removal and Docker installation steps entirely.
- If the daemon is reachable, continue with the rest of the Formbricks setup using the existing Docker installation.
- If Docker is installed but not reachable, exit with a clear message telling the user to start/fix Docker first, rather than trying to remove packages automatically.
- Only run the
apt-get remove ... / apt-get install ...flow when Docker is not installed at all, or after an explicit yes/no confirmation from the user.
That would make the script treat an existing Docker setup as something to preserve by default, which seems much closer to the intent of #7187.
|
@HamzaSwitch Please take a look at the issues and concerns @BhagyaAmarasinghe pointed out so we get can this merged in a production ready stage :-) |
6e9b2d0 to
f747af5
Compare
2a16340 to
f747af5
Compare
4 participants
Fixes #7187.
The install script ran
apt-get remove docker docker-engine docker.io containerd runcunconditionally, even when Docker was already installed and running. This could destroy a user's existing Docker setup including running containers — with no prompt and no way to undo.Now checks
command -v docker && docker infofirst. If Docker is installed and the daemon is reachable, the removal is skipped entirely with a message. If Docker is not installed (or the daemon isn't running), the cleanup proceeds as before.One
if/elseadded todocker/formbricks.sh, no other files touched.