Add normalized LETS_OS and LETS_ARCH environment vars#297
Merged
Conversation
Reviewer's GuideThis PR introduces normalized LETS_OS and LETS_ARCH environment variables exposed at command runtime, implements platform normalization helpers with tests, wires them into the executor, and documents the new behavior in the environment guide and changelog. Class diagram for Executor env setup and platform normalization helpersclassDiagram
class Executor {
+setupEnv(osCmd exec.Cmd, command config.Command, shell string) error
}
class PlatformHelpers {
+normalizeOS(os string) string
+normalizeArch(arch string) string
}
Executor ..> PlatformHelpers : uses
Flow diagram for setting normalized LETS_OS and LETS_ARCH in command environmentflowchart TD
A[Start setupEnv] --> B[Receive osCmd, command, shell]
B --> C[Set LETS_COMMAND_NAME, LETS_COMMAND_DIR, LETS_COMMAND_WORK_DIR]
C --> D[Set LETS_CONFIG, LETS_CONFIG_DIR]
D --> E[Read runtime.GOOS]
E --> F[Call normalizeOS]
F --> G[Set LETS_OS in osCmd.Env]
G --> H[Read runtime.GOARCH]
H --> I[Call normalizeArch]
I --> J[Set LETS_ARCH in osCmd.Env]
J --> K[Set LETS_SHELL and other env vars]
K --> L[Return configured osCmd]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
print-os-archcommand intests/default_env/lets.yamlonly echoesLETS_OS, butdefault_env.batsasserts bothLETS_OSandLETS_ARCH, so the test will currently fail; update the command to also echoLETS_ARCHin the expected second line. - In
platform_test.go, the table-driven tests use amapwitht.Runandt.Parallel, which can lead to non-deterministic ordering and depends on Go’s range variable semantics; consider switching to a slice of structs for the test cases to make the test behavior explicit and stable across Go versions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `print-os-arch` command in `tests/default_env/lets.yaml` only echoes `LETS_OS`, but `default_env.bats` asserts both `LETS_OS` and `LETS_ARCH`, so the test will currently fail; update the command to also echo `LETS_ARCH` in the expected second line.
- In `platform_test.go`, the table-driven tests use a `map` with `t.Run` and `t.Parallel`, which can lead to non-deterministic ordering and depends on Go’s range variable semantics; consider switching to a slice of structs for the test cases to make the test behavior explicit and stable across Go versions.
## Individual Comments
### Comment 1
<location path="internal/executor/platform_test.go" line_range="8-17" />
<code_context>
+func TestNormalizeOS(t *testing.T) {
+ t.Parallel()
+
+ tests := map[string]string{
+ "linux": "linux",
+ "darwin": "darwin",
+ "macOS": "darwin",
+ "OSX": "darwin",
+ "windows": "windows",
+ "win": "windows",
+ }
+
+ for input, expected := range tests {
+ t.Run(input, func(t *testing.T) {
+ t.Parallel()
</code_context>
<issue_to_address>
**suggestion (testing):** Subtests use loop variables directly with t.Parallel, which can be fragile across Go versions
In Go <1.22, capturing loop variables directly in subtest closures can cause flakiness because the closure may observe updated loop values. To avoid this and make the tests robust, rebind the loop variables inside the loop before calling `t.Run`:
```go
for input, expected := range tests {
input, expected := input, expected
t.Run(input, func(t *testing.T) {
t.Parallel()
got := normalizeOS(input)
if got != expected {
t.Fatalf("normalizeOS(%q) = %q, want %q", input, got, expected)
}
})
}
```
The same pattern should be applied to the `TestNormalizeArch` loop below.
Suggested implementation:
```golang
for input, expected := range tests {
input, expected := input, expected
t.Run(input, func(t *testing.T) {
t.Parallel()
got := normalizeOS(input)
if got != expected {
t.Fatalf("normalizeOS(%q) = %q, want %q", input, got, expected)
}
})
}
```
` section.
Here are the edits:
<file_operations>
<file_operation operation="edit" file_path="internal/executor/platform_test.go">
<<<<<<< SEARCH
for input, expected := range tests {
t.Run(input, func(t *testing.T) {
t.Parallel()
got := normalizeOS(input)
if got != expected {
t.Fatalf("normalizeOS(%q) = %q, want %q", input, got, expected)
}
})
}
=======
for input, expected := range tests {
input, expected := input, expected
t.Run(input, func(t *testing.T) {
t.Parallel()
got := normalizeOS(input)
if got != expected {
t.Fatalf("normalizeOS(%q) = %q, want %q", input, got, expected)
}
})
}
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
Apply the same pattern in `TestNormalizeArch` once its body is implemented. Specifically, for the loop that ranges over its test cases:
1. Inside `for arch, expected := range tests { ... }`, add `arch, expected := arch, expected` as the first statement in the loop body.
2. Ensure `t.Run` uses the rebinding (e.g., `t.Run(arch, func(t *testing.T) { ... })`) so each subtest closure captures its own copy of the loop variables.
</issue_to_address>
### Comment 2
<location path="docs/docs/env.md" line_range="21" />
<code_context>
* `LETS_COMMAND_WORK_DIR` - absolute path to `work_dir` specified in command.
* `LETS_CONFIG` - absolute path to lets config file.
* `LETS_CONFIG_DIR` - absolute path to lets config file firectory.
+* `LETS_OS` - normalized current operating system name in Go format, for example `linux`, `darwin`, `windows`
+* `LETS_ARCH` - normalized current architecture name in Go format, for example `amd64`, `arm64`, `386`
</code_context>
<issue_to_address>
**issue (typo):** Fix typo in `LETS_CONFIG_DIR` description ('firectory' -> 'directory').
```suggestion
* `LETS_CONFIG_DIR` - absolute path to lets config file directory.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+8
to
+17
| tests := map[string]string{ | ||
| "linux": "linux", | ||
| "darwin": "darwin", | ||
| "macOS": "darwin", | ||
| "OSX": "darwin", | ||
| "windows": "windows", | ||
| "win": "windows", | ||
| } | ||
|
|
||
| for input, expected := range tests { |
There was a problem hiding this comment.
suggestion (testing): Subtests use loop variables directly with t.Parallel, which can be fragile across Go versions
In Go <1.22, capturing loop variables directly in subtest closures can cause flakiness because the closure may observe updated loop values. To avoid this and make the tests robust, rebind the loop variables inside the loop before calling t.Run:
for input, expected := range tests {
input, expected := input, expected
t.Run(input, func(t *testing.T) {
t.Parallel()
got := normalizeOS(input)
if got != expected {
t.Fatalf("normalizeOS(%q) = %q, want %q", input, got, expected)
}
})
}The same pattern should be applied to the TestNormalizeArch loop below.
Suggested implementation:
for input, expected := range tests {
input, expected := input, expected
t.Run(input, func(t *testing.T) {
t.Parallel()
got := normalizeOS(input)
if got != expected {
t.Fatalf("normalizeOS(%q) = %q, want %q", input, got, expected)
}
})
}` section.
Here are the edits:
<file_operations>
<file_operation operation="edit" file_path="internal/executor/platform_test.go">
<<<<<<< SEARCH
for input, expected := range tests {
t.Run(input, func(t *testing.T) {
t.Parallel()
got := normalizeOS(input)
if got != expected {
t.Fatalf("normalizeOS(%q) = %q, want %q", input, got, expected)
}
})
}
=======
for input, expected := range tests {
input, expected := input, expected
t.Run(input, func(t *testing.T) {
t.Parallel()
got := normalizeOS(input)
if got != expected {
t.Fatalf("normalizeOS(%q) = %q, want %q", input, got, expected)
}
})
}
REPLACE
</file_operation>
</file_operations>
<additional_changes>
Apply the same pattern in TestNormalizeArch once its body is implemented. Specifically, for the loop that ranges over its test cases:
- Inside
for arch, expected := range tests { ... }, addarch, expected := arch, expectedas the first statement in the loop body. - Ensure
t.Runuses the rebinding (e.g.,t.Run(arch, func(t *testing.T) { ... })) so each subtest closure captures its own copy of the loop variables.
| @@ -19,6 +19,8 @@ title: Environment | |||
| * `LETS_COMMAND_WORK_DIR` - absolute path to `work_dir` specified in command. | |||
| * `LETS_CONFIG` - absolute path to lets config file. | |||
| * `LETS_CONFIG_DIR` - absolute path to lets config file firectory. | |||
There was a problem hiding this comment.
issue (typo): Fix typo in LETS_CONFIG_DIR description ('firectory' -> 'directory').
Suggested change
| * `LETS_CONFIG_DIR` - absolute path to lets config file firectory. | |
| * `LETS_CONFIG_DIR` - absolute path to lets config file directory. |
Contributor
Greptile SummaryThis PR adds two new built-in environment variables — Key observations:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Executor.setupEnv()"] --> B["runtime.GOOS"]
A --> C["runtime.GOARCH"]
B --> D["normalizeOS(goos)"]
C --> E["normalizeArch(goarch)"]
D -->|"'linux'→'linux'\n'darwin'→'darwin'\n'windows'→'windows'"| F["LETS_OS = result"]
E -->|"'amd64'→'amd64'\n'arm64'→'arm64'\n'386'→'386'"| G["LETS_ARCH = result"]
F --> H["defaultEnv map"]
G --> H
H --> I["osCmd.Env (appended to os.Environ())"]
style D fill:#ffe0b2,stroke:#ef6c00
style E fill:#ffe0b2,stroke:#ef6c00
note1["⚠️ runtime.GOOS/GOARCH already\nreturn canonical Go-format strings.\nNormalization switch cases are dead code."]
D -.-> note1
E -.-> note1
Last reviewed commit: c882aed |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+5
to
+27
| func normalizeOS(os string) string { | ||
| switch strings.ToLower(os) { | ||
| case "macos", "osx": | ||
| return "darwin" | ||
| case "win": | ||
| return "windows" | ||
| default: | ||
| return strings.ToLower(os) | ||
| } | ||
| } | ||
|
|
||
| func normalizeArch(arch string) string { | ||
| switch strings.ToLower(arch) { | ||
| case "x86_64", "x64": | ||
| return "amd64" | ||
| case "x86", "i386", "i686": | ||
| return "386" | ||
| case "aarch64": | ||
| return "arm64" | ||
| default: | ||
| return strings.ToLower(arch) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Normalization cases are dead code for the actual call site
Both normalizeOS and normalizeArch are only ever called with runtime.GOOS and runtime.GOARCH (see executor.go lines 214-215), which are compile-time constants set by the Go toolchain. These constants are already in the canonical Go format — lowercase, e.g. "linux", "darwin", "windows", "amd64", "arm64", "386".
This means the alternative-name switch cases ("macos", "osx", "win", "x86_64", "x64", "aarch64", etc.) can never be reached from production code. The function will always fall through to the default branch, which just returns strings.ToLower(input) — effectively an identity function for already-lowercase runtime values.
The unit tests in platform_test.go also test these unreachable paths (e.g., "macOS" → "darwin", "aarch64" → "arm64"), giving a false sense of coverage. These normalization functions would make sense if they were normalizing user-supplied input (e.g., a LETS_OS override from an env file), but not for Go runtime constants.
Consider either:
- Simply using
runtime.GOOS/runtime.GOARCHdirectly without the wrapper functions, or - Documenting that these functions are intended for future use cases where user-provided strings need normalizing
kindermax
force-pushed
the
codex/add-letsos-and-letsarch-env
branch
from
c882aed to
0e21735
Compare
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
1 participant
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.
Summary
LETS_OSandLETS_ARCHvariables during command runtime and document them in the environment guide and changelogTesting
Summary by Sourcery
Expose normalized OS and architecture information as environment variables available to commands and document their usage.
New Features:
Enhancements:
Documentation:
Tests: