Conversation
Reviewer's GuideRefactors the project into an internal-style layout by moving the main entrypoint to cmd/lets, updating version variable wiring and ldflags, and aligning build/test/release tooling to the new structure while cleaning up some run configs. 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 3 issues, and left some high level feedback:
- The new exported
Versionsymbol is now set via ldflags in several places, but some build configs (e.g., parts of.goreleaser.yml) still referencemain.version; it would be good to standardize all ldflags tomain.Versionto avoid version mismatches across artifacts. - In the updated
buildcommand inlets.yaml, when thego buildfails you print "Failed to build" but don't propagate the non-zero exit status; consider exiting with the build's status (e.g.,exit $success) so callers can detect failures correctly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new exported `Version` symbol is now set via ldflags in several places, but some build configs (e.g., parts of `.goreleaser.yml`) still reference `main.version`; it would be good to standardize all ldflags to `main.Version` to avoid version mismatches across artifacts.
- In the updated `build` command in `lets.yaml`, when the `go build` fails you print "Failed to build" but don't propagate the non-zero exit status; consider exiting with the build's status (e.g., `exit $success`) so callers can detect failures correctly.
## Individual Comments
### Comment 1
<location path=".goreleaser.yml" line_range="26" />
<code_context>
ldflags:
- -s -w -X main.version={{.Version}}
- id: darwin-arm64
- main: .
</code_context>
<issue_to_address>
**issue (bug_risk):** The ldflags symbol still targets `main.version` while the variable has been renamed to `main.Version`, which will break this build.
In this config, `main` now builds from `./cmd/lets` and the variable in `cmd/lets/main.go` is `Version`, so `-X main.version` refers to a non‑existent symbol and will cause the linker step to fail. Update this entry to match the others, e.g. `-X main.Version={{.Version}}`.
</issue_to_address>
### Comment 2
<location path="lets.yaml" line_range="124-133" />
<code_context>
- go build -ldflags="-X main.version=${VERSION:1}-dev" -o ${BIN} *.go && \
- echo " - binary './${BIN}' (version ${VERSION}) successfully build"
+
+ go build \
+ -ldflags="-X main.Version=${VERSION:1}-dev" \
+ -o ${BIN} cmd/lets/main.go
+
+ success=$?
+ if [[ $success -eq 0 ]]; then
+ version=$(./${BIN} --version)
+ echo " - binary './${BIN}' ($version) successfully build"
+ else
+ echo "Failed to build"
+ fi
</code_context>
<issue_to_address>
**issue (bug_risk):** The build command failure is swallowed, causing the script to succeed even when the binary was not built.
Because you only echo on failure and don’t exit non‑zero, callers (CI, wrapper scripts) will see this as a successful run even when `go build` fails. Please propagate the build status (e.g. `exit $success` in the `else` branch) or use `if go build ...; then ... else exit 1; fi` so failures are correctly reported.
</issue_to_address>
### Comment 3
<location path="docker-compose.yml" line_range="34" />
<code_context>
- -c
- |
- go build -o /usr/bin/lets *.go
+ go build -o /usr/bin/lets cmd/lets/main.go
if [[ -n "${LETSOPT_TEST}" ]]; then
bats tests/"${LETSOPT_TEST}" ${LETSOPT_OPTS}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Building a single source file may skip other files in the `cmd/lets` package; consider building the package path instead.
Building `cmd/lets/main.go` directly will ignore any other files in the `cmd/lets` package (e.g., future platform-specific or split-out logic). To match how the binary is normally built and avoid missing code later, prefer `go build -o /usr/bin/lets ./cmd/lets` instead.
```suggestion
go build -o /usr/bin/lets ./cmd/lets
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -25,7 +25,7 @@ builds: | |||
| ldflags: | |||
| - -s -w -X main.version={{.Version}} | |||
There was a problem hiding this comment.
issue (bug_risk): The ldflags symbol still targets main.version while the variable has been renamed to main.Version, which will break this build.
In this config, main now builds from ./cmd/lets and the variable in cmd/lets/main.go is Version, so -X main.version refers to a non‑existent symbol and will cause the linker step to fail. Update this entry to match the others, e.g. -X main.Version={{.Version}}.
| go build \ | ||
| -ldflags="-X main.Version=${VERSION:1}-dev" \ | ||
| -o ${BIN} cmd/lets/main.go | ||
|
|
||
| success=$? | ||
| if [[ $success -eq 0 ]]; then | ||
| version=$(./${BIN} --version) | ||
| echo " - binary './${BIN}' ($version) successfully build" | ||
| else | ||
| echo "Failed to build" |
There was a problem hiding this comment.
issue (bug_risk): The build command failure is swallowed, causing the script to succeed even when the binary was not built.
Because you only echo on failure and don’t exit non‑zero, callers (CI, wrapper scripts) will see this as a successful run even when go build fails. Please propagate the build status (e.g. exit $success in the else branch) or use if go build ...; then ... else exit 1; fi so failures are correctly reported.
| - -c | ||
| - | | ||
| go build -o /usr/bin/lets *.go | ||
| go build -o /usr/bin/lets cmd/lets/main.go |
There was a problem hiding this comment.
suggestion (bug_risk): Building a single source file may skip other files in the cmd/lets package; consider building the package path instead.
Building cmd/lets/main.go directly will ignore any other files in the cmd/lets package (e.g., future platform-specific or split-out logic). To match how the binary is normally built and avoid missing code later, prefer go build -o /usr/bin/lets ./cmd/lets instead.
| go build -o /usr/bin/lets cmd/lets/main.go | |
| go build -o /usr/bin/lets ./cmd/lets |
Greptile SummaryThis PR migrates the project to a standard Go internal layout by moving The structural change is clean, but the rename was only partially propagated in Additionally, the new Key changes:
Issues found:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[go source\ncmd/lets/main.go] -->|go build -ldflags\n-X main.Version=...| B[lets binary]
subgraph goreleaser [goreleaser builds]
G1["darwin-amd64\n❌ -X main.version (lowercase)"]
G2["darwin-arm64\n✅ -X main.Version (uppercase)"]
G3["linux-amd64\n❌ -X main.version (lowercase)"]
end
subgraph local [local builds]
L1["lets build\ncmd/lets/main.go\n✅ -X main.Version"]
L2["lets build-and-install\ncmd/lets/main.go\n✅ -X main.Version"]
L3["docker-compose test-bats\ncmd/lets/main.go\n(no version flag)"]
end
A --> goreleaser
A --> local
G1 -->|version = 0.0.0-dev| R1[darwin-amd64 release ❌]
G2 -->|version = tag| R2[darwin-arm64 release ✅]
G3 -->|version = 0.0.0-dev| R3[linux-amd64 release ❌]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -25,7 +25,7 @@ builds: | |||
| ldflags: | |||
| - -s -w -X main.version={{.Version}} | |||
There was a problem hiding this comment.
Stale main.version linker flag — version won't be injected for darwin-amd64
The variable was renamed from version to Version (uppercase) in cmd/lets/main.go, but the darwin-amd64 build still references the old lowercase name. At link time, -X main.version={{.Version}} will silently do nothing because no such symbol exists, so all darwin-amd64 releases will report 0.0.0-dev as the version.
The same issue also appears on line 54 for the linux-amd64 build — only darwin-arm64 (line 41) was correctly updated to -X main.Version={{.Version}}.
| - -s -w -X main.version={{.Version}} | |
| - -s -w -X main.Version={{.Version}} |
| success=$? | ||
| if [[ $success -eq 0 ]]; then | ||
| version=$(./${BIN} --version) | ||
| echo " - binary './${BIN}' ($version) successfully build" | ||
| else | ||
| echo "Failed to build" | ||
| fi |
There was a problem hiding this comment.
Build failure is silently swallowed — command always exits 0
success=$? correctly captures the go build exit code, but the else branch only echos a message and then falls through to the end of the script. The final command executed is echo "Failed to build", which exits with 0, so lets build will appear to succeed even when the Go compilation failed. This will cause callers (CI, scripts) to believe the build produced a valid binary when it did not.
To propagate the failure, exit explicitly with the captured code:
go build \
-ldflags="-X main.Version=${VERSION:1}-dev" \
-o ${BIN} cmd/lets/main.go
success=$?
if [[ $success -eq 0 ]]; then
version=$(./${BIN} --version)
echo " - binary './${BIN}' ($version) successfully build"
else
echo "Failed to build"
exit $success
fi
| - -c | ||
| - | | ||
| go build -o /usr/bin/lets *.go | ||
| go build -o /usr/bin/lets cmd/lets/main.go |
There was a problem hiding this comment.
Prefer package path over file path for go build
Passing a file path (cmd/lets/main.go) to go build limits compilation to that single file. If any additional .go files are ever added to the cmd/lets package they will be silently excluded. The idiomatic and future-proof form is to pass the package directory, consistent with how .goreleaser.yml references the same entry-point.
The same pattern also appears in lets.yaml at lines 112 and 126.
| go build -o /usr/bin/lets cmd/lets/main.go | |
| go build -o /usr/bin/lets ./cmd/lets |
1 participant
Summary by Sourcery
Adopt an internal Go project layout centered on cmd/lets and wire versioning through an exported Version variable across builds, tests, and release tooling.
Enhancements:
Tests: