fix: Windows toolchain installation issues#2012
Conversation
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughConditional loading for atmos.d/.atmos.d, centralized Windows .exe normalization across installer/asset/extract flows, platform-aware PATH hinting, many Windows-focused tests/fixtures, and updated CLI docs and snapshots. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/docs/cli/commands/env.mdx (1)
41-70: Quoteevalto avoid word-splitting in shell examples.
Unquotedeval $(...)can break when values contain spaces or special characters.🔧 Suggested update
- eval $(atmos env) + eval "$(atmos env)" @@ - eval $(atmos env --profile ci) + eval "$(atmos env --profile ci)" @@ - ATMOS_PROFILE=ci eval $(atmos env) + ATMOS_PROFILE=ci eval "$(atmos env)"
🧹 Nitpick comments (4)
tests/snapshots/TestCLICommands_atmos_describe_configuration.stderr.golden (1)
8-24: Observation: Multiple directory loads per command.The snapshot shows
atmos.dbeing discovered/loaded 10 times during a single command execution. This is existing behavior now made visible by the new logging. Not a concern for this PR, but could be a future optimization opportunity if config loading performance becomes a consideration.toolchain/installer/download.go (1)
189-213: Consider improving error message clarity.The error on line 212 references
assetURLandfallbackURLbut doesn't mention the.exevariants that were actually attempted. This could confuse debugging.🔧 Suggested improvement
- return "", fmt.Errorf("%w: tried %s and %s: %w", ErrHTTPRequest, assetURL, fallbackURL, ErrHTTP404) + return "", fmt.Errorf("%w: tried %s.exe and %s.exe: %w", ErrHTTPRequest, assetURL, fallbackURL, ErrHTTP404)tests/toolchain_custom_commands_test.go (2)
328-336: Consider usingt.Skipor a dedicated TODO test marker.The soft failure pattern here means CI passes even when the feature doesn't work. If toolchain PATH injection isn't yet implemented, consider:
- Using
t.Skip("Pending: toolchain PATH injection not implemented")- Or marking this as an expected failure with a tracking issue
The current approach risks the incomplete behavior being forgotten.
♻️ Alternative approach
// The command should succeed if toolchain integration works. if err != nil { - t.Logf("Command failed: %v", err) - t.Logf("This may indicate that toolchain tools are not in PATH for custom commands") - // Don't fail - document current behavior. - // Once the toolchain PATH injection is implemented, change to require.NoError. + // TODO: Remove skip once toolchain PATH injection is implemented. + t.Skipf("Pending implementation: toolchain tools not in PATH for custom commands (error: %v)", err) } else { assert.Contains(t, strings.ToLower(string(output)), strings.ToLower(tc.expectedOutput), "Output should contain %q", tc.expectedOutput)
346-349: Fragile path calculation.The
"../../../.."assumes the fixture is always exactly 4 levels deep from repo root. If the fixture directory structure changes, this breaks silently.A more robust approach would find the repo root by locating
go.mod:♻️ More robust alternative
// findRepoRoot walks up from cwd to find go.mod. func findRepoRoot(t *testing.T) string { t.Helper() dir, err := os.Getwd() require.NoError(t, err) for { if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { return dir } parent := filepath.Dir(dir) if parent == dir { t.Fatal("could not find repo root (go.mod)") } dir = parent } }
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2012 +/- ##
==========================================
- Coverage 75.38% 75.36% -0.02%
==========================================
Files 792 792
Lines 73356 73396 +40
==========================================
+ Hits 55299 55318 +19
- Misses 14553 14566 +13
- Partials 3504 3512 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@coderabbitai full review please |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@pkg/config/load.go`:
- Around line 1042-1063: The code currently treats any os.Stat error as "not
found" for atmosDPath and dotAtmosDPath, hiding permission/IO errors; update
both checks around os.Stat(atmosDPath) and os.Stat(dotAtmosDPath) to distinguish
os.IsNotExist(err) (log.Trace "No ... directory found" and continue) from other
errors (log.Error or log.Warn with the error details and the path), and only
call stat.IsDir() when err == nil; keep the existing call to
loadAtmosConfigsFromDirectory(searchPattern, dst, "atmos.d"/".atmos.d")
unchanged.
In `@tests/fixtures/scenarios/toolchain-custom-commands/atmos.yaml`:
- Around line 45-47: The fixtures use a hard-coded Unix path in logs.file (value
"/dev/stderr") which will fail on Windows; update the fixture by either removing
the logs.file key entirely or setting logs.file to an empty string so atmos
falls back to platform-appropriate defaults (i.e., change the logs.file entry in
the atmo.yaml fixture).
In `@toolchain/installer/asset.go`:
- Around line 71-73: The block comment in asset.go that starts "On Windows, add
.exe to raw binary asset names that don't have an archive extension." and ends
with the URL "See: https://aquaproj.github.io/docs/reference/windows-support/"
is missing a terminal period; update that trailing sentence to end with a period
so the comment finishes with proper punctuation (e.g., add a period after the
URL) -- locate this comment near the Windows/asset logic in the asset.go file
(the comment above the raw binary asset handling) and amend it accordingly.
- Around line 84-92: Update hasArchiveExtension to recognize additional archive
suffixes so Windows assets aren't misdetected and `.exe` isn't appended;
specifically, extend the strings.HasSuffix checks in hasArchiveExtension
(asset.go) to include multi-part and other common archive extensions such as
".tar.xz", ".tar.bz2", ".tar.zst" as well as single-part extensions like ".7z",
".xz", ".bz2", ".rar", ".deb", and ".rpm", making sure to check longer
multi-part suffixes before shorter ones (e.g., ".tar.xz" before ".xz") against
the lower variable.
🧹 Nitpick comments (2)
docs/fixes/windows-atmos-d-and-toolchain-issues.md (1)
61-65: Optional: Add language specifier to fenced code block.These code blocks would benefit from a language specifier for syntax highlighting consistency. Based on learnings, this can be addressed in a separate documentation cleanup commit.
📝 Suggested fix
-``` +```text Asset template: jq-{{.OS}}-{{.Arch}} On Windows: https://github.com/jqlang/jq/releases/download/jq-1.7.1/jq-windows-amd64.exe ✅ On Linux: https://github.com/jqlang/jq/releases/download/jq-1.7.1/jq-linux-amd64 ✅tests/toolchain_custom_commands_test.go (1)
232-236: Use the getBinaryPath helper for consistency instead of hardcoding the path structure.The test path at line 233 and the getBinaryPath helper at line 366 actually construct identical paths. Both produce
toolsDir/bin/jqlang/jq/1.7.1/jq.exe. Consider using getBinaryPath to reduce duplication and future-proof against path structure changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@toolchain/installer/asset.go`:
- Around line 71-76: The code unconditionally calls EnsureWindowsExeExtension
for any asset that lacks an archive extension, causing names like
"installer.msi" to become "installer.msi.exe"; change the conditional in the
asset name handling so EnsureWindowsExeExtension is only called when the asset
has no file extension at all (use filepath.Ext or an equivalent file-extension
check) — update the logic around hasArchiveExtension and
EnsureWindowsExeExtension to check that filepath.Ext(assetName) == "" before
appending .exe to assetName.
🧹 Nitpick comments (2)
pkg/config/load_error_paths_test.go (2)
129-139: Prefert.Setenvfor test-scoped env isolation.
You can replace the manual unset with an emptyt.Setenv, which still yields an emptyos.Getenvresult while preserving automatic cleanup.Based on learnings, prefer `t.Setenv` for env setup/teardown in tests.🛠️ Suggested tweak
- os.Unsetenv("ATMOS_CLI_CONFIG_PATH") + t.Setenv("ATMOS_CLI_CONFIG_PATH", "")
92-112: Avoid swallowing errors in the Getwd error-path test.
Right now the test can pass without exercising the error path. Consider asserting onerrwhen directory removal succeeds, ort.Skipwhen it can’t be reproduced.As per coding guidelines, tests should assert behavior rather than silently discarding errors.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/load_error_paths_test.go (1)
393-397: Tautological test - error silenced without assertion.The
_ = errpattern means this test always passes regardless of behavior. Either assert on the error (e.g., check for max depth error) or document why no assertion is needed.Per coding guidelines: "Avoid tautological tests."
Suggested fix
// This should eventually hit max import depth err = mergeConfig(v, tempDir, "atmos.yaml", true) - // May error on max depth or may complete - testing the path exists - _ = err + // Circular imports should trigger max depth error. + assert.Error(t, err, "Expected error from circular import")
🧹 Nitpick comments (1)
pkg/config/load_error_paths_test.go (1)
489-572: Good scenario coverage.Tests cover the key permutations well. Optional enhancement: consider asserting that viper actually contains the loaded values (e.g.,
v.GetString("settings.test_value")) to verify the config was processed, not just that no panic occurred.Example assertion
// After loadAtmosDFromDirectory call: assert.Equal(t, "from_atmos_d", v.GetString("settings.test_value"))
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 (1)
pkg/config/load_error_paths_test.go (1)
393-397: Test discards result without assertion.Line 396 uses
_ = errwhich makes this a no-op. The test should either assert that an error occurs (circular import should hit max depth) or assert success. As-is, it doesn't verify any behavior.Suggested fix
// This should eventually hit max import depth err = mergeConfig(v, tempDir, "atmos.yaml", true) - // May error on max depth or may complete - testing the path exists - _ = err + // Circular imports should trigger max depth error + assert.Error(t, err, "circular import should hit max depth limit")
🤖 Fix all issues with AI agents
In `@toolchain/installer/extract.go`:
- Around line 238-253: The current os.Stat check for src treats any error on the
.exe probe as "not found" and returns ErrToolNotFound, which hides other errors;
modify the logic in the block that computes srcWithExe so that after calling
os.Stat(srcWithExe) you distinguish os.IsNotExist(exeErr) versus other errors —
if not exist then return ErrToolNotFound as before, but if exeErr is non-nil and
not IsNotExist, return or wrap exeErr as ErrFileOperation (preserving the
original error); ensure you still set src = srcWithExe and call
EnsureWindowsExeExtension(dst) only when the .exe Stat succeeds.
🧹 Nitpick comments (2)
pkg/config/load_error_paths_test.go (2)
226-226: Hardcoded path with forward slashes.Per coding guidelines, prefer
filepath.Join()even for non-existent paths. This keeps tests consistent cross-platform.Suggested fix
- _, err := readConfigFileContent("/nonexistent/path/atmos.yaml") + _, err := readConfigFileContent(filepath.Join(string(os.PathSeparator), "nonexistent", "path", "atmos.yaml"))
445-445: Same hardcoded path pattern.Consider using
filepath.Join()for consistency.Suggested fix
- err := mergeConfigFile("/nonexistent/path/config.yaml", v) + err := mergeConfigFile(filepath.Join(string(os.PathSeparator), "nonexistent", "path", "config.yaml"), v)
|
These changes were released in v1.204.1-rc.5. |
Add .exe handling to buildHTTPAssetURL() for HTTP type tools (like kubectl from dl.k8s.io) to match existing behavior in buildGitHubReleaseURL(). This fixes HTTP 404 errors on Windows when downloading raw binaries. Added comprehensive test fixture in tests/fixtures/scenarios/windows-test/ with .tool-versions, .atmos.d custom commands, and Terraform components for Windows testing. Related: #2002, #2012 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…2027) * WIP: Windows toolchain HTTP type .exe fix and test fixture Add .exe handling to buildHTTPAssetURL() for HTTP type tools (like kubectl from dl.k8s.io) to match existing behavior in buildGitHubReleaseURL(). This fixes HTTP 404 errors on Windows when downloading raw binaries. Added comprehensive test fixture in tests/fixtures/scenarios/windows-test/ with .tool-versions, .atmos.d custom commands, and Terraform components for Windows testing. Related: #2002, #2012 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * updates * fix toolchain installation, improve errors, improve registries detection, add test fixtures and tests * address comments, add tests, fix platform detection in Aqua registry * address comments, add tests, fix platform detection in Aqua registry * updates * address comments, add tests, fix platform detection in Aqua registry * update docs * address comments --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
2 participants
what
.exeextension.exefor raw binaries on Windows (following Aqua behavior).exebinaries inside archivesInvoke-Expressionsyntax instead of Unixevalon Windows.atmos.ddirectory detection to differentiate permission errors from not-found errors.tar.xz,.tar.bz2,.7z, and other formatswhy
Users reported multiple issues when using
atmos toolchain installon Windows:.exeextension - Installing tools like terraform resulted in binaries without.exeextension, causingterraform --versionto hang indefinitely.exe- Tools like jq that have standalone Windows binaries (e.g.,jq-windows-amd64.exe) failed to download because the URL was constructed without.exe.tar.gz,.zip) failed because the extractor looked forwindows-amd64/helminstead ofwindows-amd64/helm.exeeval $(...)syntax instead of PowerShellInvoke-ExpressionsyntaxArchitecture
Centralized Windows Extension Handling
Following Aqua's Windows support approach, Windows executables need the
.exeextension to be found byos/exec.LookPath. We use a single centralized function:Download URL Handling by Tool Type
.exeHandlinggithub_release.exeon Windows for raw binaries (assets without archive extensions like.tar.gz,.zip)http.exein the URL if neededThis matches Aqua's behavior where
.exeis added to the download URL upfront during URL construction, not as a fallback after a 404 error.Archive Extension Detection
Extended to recognize additional formats to avoid incorrectly appending
.exeto archive URLs:Archive Extraction
When extracting from archives, the
.exefallback only runs on Windows (not on Unix) to avoid masking wrong-asset errors.Fixes Applied
toolchain/installer/installer.goEnsureWindowsExeExtension()centralized functiontoolchain/installer/asset.go.exeto GitHub release URLs for raw binaries on Windows; extended archive detectiontoolchain/installer/extract.go.exefallback only on Windowstoolchain/install_helpers.gopkg/config/load.go.atmos.ddirectoriesTest Results (Windows)
All integration tests pass on Windows:
references
docs/fixes/windows-atmos-d-and-toolchain-issues.mdtests/fixtures/scenarios/toolchain-custom-commandstests/toolchain_custom_commands_test.goSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.