Windows: show correct Docker Desktop start command#162
Merged
anisaoshafi merged 2 commits intomainfrom Mar 24, 2026
Merged
Conversation
anisaoshafi
requested review from
carole-lavillonniere and
silv-io
as code owners
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 (1)
test/integration/docker_windows_test.go (1)
12-17: Consider documenting why PSModulePath and DOCKER_HOST are both removed first.The helper removes both
PSModulePathandDOCKER_HOST, then re-addsDOCKER_HOST. The removal ofPSModulePathis the default state (for CMD detection), but this might not be immediately obvious to future readers.📝 Suggested documentation improvement
-// windowsDockerErrorEnv returns an Environ with an invalid DOCKER_HOST and no PSModulePath. +// windowsDockerErrorEnv returns an Environ configured to trigger a Docker connection failure. +// PSModulePath is removed to default to CMD shell detection; callers can add it back for PowerShell tests. +// DOCKER_HOST points to an unreachable address to force the "Docker not available" error path. func windowsDockerErrorEnv() env.Environ {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/docker_windows_test.go` around lines 12 - 17, The helper windowsDockerErrorEnv removes PSModulePath and DOCKER_HOST then re-adds DOCKER_HOST with an invalid value; add a brief inline comment in the windowsDockerErrorEnv function explaining that PSModulePath is removed to simulate a non-PowerShell (CMD) environment detection and that DOCKER_HOST is cleared first to ensure the test only uses the intentionally invalid "tcp://localhost:1" value—this will make the intent clear to future readers without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/runtime/docker.go`:
- Around line 104-110: The windowsDockerStartCommand currently returns shell
commands that rely on name resolution ("Docker Desktop"); update it to use
Docker's documented startup methods: detect PowerShell via
getenv("PSModulePath") (as already done) and return either the "docker desktop
start" command for modern Docker (preferred) or the full executable path
"C:\Program Files\Docker\Docker\Docker Desktop.exe" as a fallback (ensuring
proper quoting/escaping), so modify windowsDockerStartCommand to prefer "docker
desktop start" when available and fall back to the full executable path for both
PowerShell and cmd.exe branches instead of the current Start-Process/'start'
strings.
---
Nitpick comments:
In `@test/integration/docker_windows_test.go`:
- Around line 12-17: The helper windowsDockerErrorEnv removes PSModulePath and
DOCKER_HOST then re-adds DOCKER_HOST with an invalid value; add a brief inline
comment in the windowsDockerErrorEnv function explaining that PSModulePath is
removed to simulate a non-PowerShell (CMD) environment detection and that
DOCKER_HOST is cleared first to ensure the test only uses the intentionally
invalid "tcp://localhost:1" value—this will make the intent clear to future
readers without changing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c7954954-6350-4197-ab2e-efbddf8f048e
📒 Files selected for processing (3)
internal/runtime/docker.gointernal/runtime/docker_test.gotest/integration/docker_windows_test.go
anisaoshafi
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On Windows, lstk now detects if the user is running PowerShell or cmd.exe and suggests the correct command to start Docker Desktop. It used to show the PowerShell command regardless of shell.
We use the
PSModulePathenvironment variable, which PowerShell always sets (cmd.exe does not).Also took advantage to suppress the verbose Windows Docker error message since it isn't actionable.