new features for 0.1.2 - #3
Conversation
🤖 Augment PR SummarySummary: Prepares Changes:
Technical Notes: 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| } | ||
|
|
||
| if !dockerDaemon.OK { | ||
| offerDockerHelp() |
There was a problem hiding this comment.
internal/cmd/doctor.go:48-50 — Calling offerDockerHelp() can make versola doctor block waiting on stdin when Docker isn’t installed (e.g., non-interactive/scripted runs), even though doctor otherwise looks like a pure diagnostic command.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| // title, so a URL passed there directly gets misinterpreted | ||
| // instead of opened, especially one containing "&" or other | ||
| // characters cmd treats specially. | ||
| c = exec.Command("cmd", "/c", "start", "", url) |
There was a problem hiding this comment.
internal/browser/browser.go:26 — On Windows, cmd /c start ... url can still misinterpret URLs containing shell metacharacters like & unless the URL is quoted, so Open() may fail to open some valid URLs despite the comment suggesting otherwise.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| // else is meant to live there. | ||
| versolaDir := filepath.Dir(stateDir) | ||
| _, statErr := os.Stat(versolaDir) | ||
| dirExists := statErr == nil |
There was a problem hiding this comment.
internal/cmd/uninstall.go:62-63 — dirExists := statErr == nil treats any os.Stat error (including permission/IO errors) as “doesn’t exist”, which can cause uninstall to skip removing local state or incorrectly report “Nothing to remove.”
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| // on the Docker side this command could remove anyway, and | ||
| // `versola doctor` already covers diagnosing an unreachable | ||
| // daemon. | ||
| return nil, nil |
There was a problem hiding this comment.
internal/cmd/uninstall.go:129-134 — versolaImages() returns (nil, nil) for any docker images error, so non-reachability failures (e.g., permissions/misconfigured context) will be silently ignored and may leave Versola images behind while uninstall reports success.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
augment review |
| dirExists := statErr == nil | ||
|
|
||
| if !deployed && len(images) == 0 && !dirExists { | ||
| fmt.Println("Nothing to remove — no deployed stack, no versola images, no local state.") |
There was a problem hiding this comment.
internal/cmd/uninstall.go:83 — When dockerUp is false, images is never checked, so this message can incorrectly assert “no versola images” and return early even if Versola images exist but can’t be queried while Docker is down. Consider distinguishing “couldn’t check images because Docker isn’t running” from “no images found” to avoid misleading output.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| // only reading state, never changing it. | ||
| func offerDockerHelp() { | ||
| if _, err := exec.LookPath("docker"); err == nil { | ||
| fmt.Println("\nDocker is installed but the daemon isn't responding — start Docker Desktop (or Colima, if that's what you use) and try again.") |
There was a problem hiding this comment.
internal/cmd/doctor.go:77 — checks.DockerDaemon() can fail for reasons other than “daemon not running” (e.g., permission denied to /var/run/docker.sock on Linux), so this advice may be misleading in a common setup. Consider tailoring the hint based on the underlying error so users don’t get sent to “start Docker” when it’s actually an access/permissions issue.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
augment review |
| // "permission denied" in docker's own output, which is what this checks | ||
| // for before falling back to the generic "it's not running" hint. | ||
| func daemonUnreachableHint(dockerPath string) string { | ||
| out, _ := exec.Command(dockerPath, "version", "--format", "{{.Server.Version}}").CombinedOutput() |
There was a problem hiding this comment.
internal/cmd/doctor.go:110 — daemonUnreachableHint() runs docker version without any timeout/cancellation (unlike checks.DockerDaemon(), which is time-bounded). If the docker client blocks while trying to contact an unhealthy/remote daemon, versola doctor could hang here even though the initial check already failed fast.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
|
|
||
| func runUninstall(cmd *cobra.Command, args []string) error { | ||
| composePath, deployed, err := state.ComposeFile() |
There was a problem hiding this comment.
internal/cmd/uninstall.go:37 — state.ComposeFile() currently treats any os.Stat error as “compose.yml doesn’t exist”, so deployed can be false even on permission/IO errors. In that case uninstall may skip docker compose down even though a deployment exists.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| return fmt.Errorf("docker compose down failed: %w", err) | ||
| } | ||
| } else if deployed { | ||
| fmt.Println("Docker isn't running — skipping docker compose down, clearing recorded state only.") |
There was a problem hiding this comment.
internal/cmd/uninstall.go:124 — This message assumes checks.DockerDaemon().OK == false means “Docker isn't running”, but that check can also fail for other common reasons (e.g., permission denied to the daemon socket). The output here may send users toward “start Docker” when the real issue is access/configuration.
Severity: low
Other Locations
internal/cmd/uninstall.go:86internal/cmd/uninstall.go:100internal/cmd/uninstall.go:103
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
|
|
||
| if !dockerDaemon.OK { | ||
| offerDockerHelp() |
There was a problem hiding this comment.
internal/cmd/doctor.go:49 — offerDockerHelp() may open a browser tab as part of doctor, which is a side effect that can surprise scripted usage. The command description above says it “only reads state” and “never … changes anything”, which may be a bit misleading now.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
augment review |
| default: | ||
| return fmt.Errorf("don't know how to open a browser on %s", runtime.GOOS) | ||
| } | ||
| return c.Start() |
There was a problem hiding this comment.
internal/browser/browser.go:36 — exec.Cmd.Start() is used without a corresponding Wait() or Process.Release(), which can leave an unreaped child process on Unix if the CLI stays alive after launching the browser. Consider ensuring the process resources are released while still keeping the call non-blocking.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| if dirExists { | ||
| fmt.Printf("Removing %s...\n", versolaDir) | ||
| if err := os.RemoveAll(versolaDir); err != nil { |
There was a problem hiding this comment.
internal/cmd/uninstall.go:137-142 — When dockerUp is false but a deployment is recorded, uninstall still deletes ~/.versola (including compose.yml), which can make it harder to later stop/remove a stack that was actually running but unreachable due to permissions/context. Consider whether preserving the compose state (or warning more strongly) is safer in that scenario.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
augment review |
| if stopStack { | ||
| fmt.Println(" - the running Versola stack and its Postgres data volume") | ||
| } else if deployed { | ||
| fmt.Println(" - recorded deployment state (Docker isn't reachable, so nothing is actually up to stop)") |
There was a problem hiding this comment.
internal/cmd/uninstall.go:111 — The preview line "recorded deployment state (Docker isn't reachable, so nothing is actually up to stop)" looks misleading when keepDirForLaterCleanup is true, since the directory/state is explicitly kept later. It may also be inaccurate if Docker is unreachable due to permissions/context while the stack could still be running.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
augment review |
| // runDocker call site (compose up/down, uninstall's rmi, etc.) that has no | ||
| // need for this. | ||
| func pullAndRunTools(dir, image string) error { | ||
| var stderrBuf bytes.Buffer |
There was a problem hiding this comment.
internal/cmd/bootstrap.go:170-172 — stderrBuf buffers all Docker stderr (including pull progress output) in memory; on a first run this can get fairly large/unbounded and may bloat the CLI’s memory usage unexpectedly.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| // error `docker images` could return. | ||
| func versolaImages() ([]string, error) { | ||
| out, err := exec.Command("docker", "images", "--format", "{{.Repository}}:{{.Tag}}", | ||
| "--filter", "reference=ghcr.io/versolauth/versola-*").Output() |
There was a problem hiding this comment.
internal/cmd/uninstall.go:178-181 — If docker images fails here, the user-visible error will typically be only exit status 1 (wrapped), so Docker’s stderr explaining the failure may be hidden, making uninstall failures harder to diagnose.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
augment review |
| Short: "Remove everything versola deployed locally", | ||
| Long: `uninstall stops the locally deployed Versola stack (including its | ||
| Postgres data volume), removes the Docker images versola pulled, and | ||
| clears ~/.versola. |
There was a problem hiding this comment.
internal/cmd/uninstall.go:21-23 says uninstall "clears ~/.versola", but runUninstall can intentionally keep the directory when keepDirForLaterCleanup is true (recorded deployment + Docker unreachable) to preserve a future cleanup handle. Consider aligning the user-facing help/docs with that conditional behavior so users aren’t misled about what will actually be removed.
Severity: medium
Other Locations
README.md:169
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
augment review |
1 participant
What
stack is ready (--no-browser to opt out).
never published, instead of surfacing Docker's raw "manifest unknown".
(Docker Desktop/Colima choice on macOS, Docker Desktop on Windows,
install docs on Linux) — never installs anything itself, just opens
a browser tab.
hard-fails on
docker compose down— it skips that step and stillclears ~/.versola.
release.yml which only runs on tag push.
Why
Requested for the next versola-cli release (see team lead's notes on
port collisions, uninstall, and browser convenience). The bootstrap
error message and CI workflow are follow-up hardening on top of that.
Testing
Manually tested on Windows: doctor (Docker down and up), bootstrap
(browser opens, unknown-version error), uninstall (Docker down, Docker
up, second-run no-op). ci.yml not yet exercised on a real PR run.