Conversation
Reviewer's GuideRefactors the lets CLI into an internal cmd/lets application structure, updates version injection to use a public main.Version variable, and aligns build, test, Docker, and GoReleaser configurations with the new entrypoint and version symbol while cleaning up IDE run configs. Class diagram for the updated main package entrypointclassDiagram
class main {
+string Version
+main()
}
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 1 issue, and left some high level feedback:
- In
.goreleaser.yml, some build configs (e.g.,darwin-amd64) still use-X main.version={{.Version}}while others usemain.Version; align all ldflags to the newVersionsymbol so releases embed the correct version. - In the
buildcommand inlets.yaml, the failure branch only prints"Failed to build"but does not return a non-zero exit code; consider exiting with the originalgo buildstatus so CI and callers can detect build failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `.goreleaser.yml`, some build configs (e.g., `darwin-amd64`) still use `-X main.version={{.Version}}` while others use `main.Version`; align all ldflags to the new `Version` symbol so releases embed the correct version.
- In the `build` command in `lets.yaml`, the failure branch only prints `"Failed to build"` but does not return a non-zero exit code; consider exiting with the original `go build` status so CI and callers can detect build failures.
## Individual Comments
### Comment 1
<location path="lets.yaml" line_range="114" />
<code_context>
+ go build -ldflags="-X main.Version=${VERSION:1}-dev" -o "${BIN}" cmd/lets/main.go && \
sudo mv ./${BIN} $PATH2LETSDEV/${BIN} && \
- echo " - binary ${BIN} version ${VERSION} successfully installed in ${PATH2LETSDEV}"
+ echo " - binary ${BIN} version $($PATH2LETSDEV/${BIN} --version) successfully installed in ${PATH2LETSDEV}"
build:
</code_context>
<issue_to_address>
**nitpick (bug_risk):** Unquoted paths in the version check could break if `PATH2LETSDEV` contains spaces.
The unquoted `$PATH2LETSDEV/${BIN} --version` invocation will break when the install path has spaces or special characters. Please quote the command, e.g.:
```bash
echo " - binary ${BIN} version "$"${PATH2LETSDEV}/${BIN}" --version" successfully installed in ${PATH2LETSDEV}
```
and apply similar quoting anywhere `${BIN}` or `${PATH2LETSDEV}` are used in commands.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| go build -ldflags="-X main.Version=${VERSION:1}-dev" -o "${BIN}" cmd/lets/main.go && \ | ||
| sudo mv ./${BIN} $PATH2LETSDEV/${BIN} && \ | ||
| echo " - binary ${BIN} version ${VERSION} successfully installed in ${PATH2LETSDEV}" | ||
| echo " - binary ${BIN} version $($PATH2LETSDEV/${BIN} --version) successfully installed in ${PATH2LETSDEV}" |
There was a problem hiding this comment.
nitpick (bug_risk): Unquoted paths in the version check could break if PATH2LETSDEV contains spaces.
The unquoted $PATH2LETSDEV/${BIN} --version invocation will break when the install path has spaces or special characters. Please quote the command, e.g.:
echo " - binary ${BIN} version "$"${PATH2LETSDEV}/${BIN}" --version" successfully installed in ${PATH2LETSDEV}and apply similar quoting anywhere ${BIN} or ${PATH2LETSDEV} are used in commands.
Contributor
Greptile SummaryThis PR migrates the repository to a standard Go internal project structure by moving Key changes:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[goreleaser / go build] -->|"-X main.Version"| B[cmd/lets/main.go\nvar Version = '0.0.0-dev']
B --> C{Platform}
C -->|darwin-arm64\n✅ main.Version| D[Version injected correctly]
C -->|darwin-amd64\n❌ main.version| E[Version stays '0.0.0-dev']
C -->|linux-amd64\n❌ main.version| F[Version stays '0.0.0-dev']
D --> G[Binary reports correct version]
E --> H[Binary reports wrong version]
F --> H
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
.goreleaser.yml
Outdated
| @@ -25,7 +25,7 @@ builds: | |||
| ldflags: | |||
| - -s -w -X main.version={{.Version}} | |||
Contributor
There was a problem hiding this comment.
Incorrect ldflags variable name — version injection will silently fail
main.version (lowercase) is used here, but the variable was renamed to Version (uppercase) in cmd/lets/main.go. Go's -X linker flag performs an exact, case-sensitive match on the fully-qualified symbol name. Because main.version no longer exists, goreleaser will silently ignore this flag and the released darwin-amd64 binary will always report 0.0.0-dev instead of the actual release version.
The same problem exists for linux-amd64 at line 54.
Suggested change
| - -s -w -X main.version={{.Version}} | |
| - -s -w -X main.Version={{.Version}} |
docker-compose.yml
Outdated
| - -c | ||
| - | | ||
| go build -o /usr/bin/lets *.go | ||
| go build -o /usr/bin/lets cmd/lets/main.go |
Contributor
There was a problem hiding this comment.
Building a file path instead of a package path
go build -o /usr/bin/lets cmd/lets/main.go passes a file path to go build. While this works today because main.go is the only file in cmd/lets/, if any additional .go files (e.g. helper functions, _test.go build-tagged files, or init code) are added to that directory later, they will be silently excluded from the build. The idiomatic approach is to pass the package import path:
Suggested change
| go build -o /usr/bin/lets cmd/lets/main.go | |
| go build -o /usr/bin/lets ./cmd/lets |
| echo " - binary ${BIN} version $($PATH2LETSDEV/${BIN} --version) successfully installed in ${PATH2LETSDEV}" | ||
|
|
||
| build: | ||
| description: Build lets from source code |
Contributor
There was a problem hiding this comment.
Building a file path instead of a package path
Same concern as in docker-compose.yml: go build ... cmd/lets/main.go passes a source-file path rather than a package path (./cmd/lets). Any future files added to cmd/lets/ would be silently omitted. Consider using ./cmd/lets for both the install and build commands.
Suggested change
| description: Build lets from source code | |
| go build -ldflags="-X main.Version=${VERSION:1}-dev" -o "${BIN}" ./cmd/lets && \ |
kindermax
force-pushed
the
fix-go-build-command
branch
3 times, most recently
from
f98ebe6 to
f33ec89
Compare
kindermax
force-pushed
the
fix-go-build-command
branch
from
f33ec89 to
372f076
Compare
kindermax
force-pushed
the
fix-go-build-command
branch
from
372f076 to
d1123f1
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 by Sourcery
Switch the CLI to an internal cmd/lets entrypoint and central Version variable, updating build, release, and test tooling accordingly.
Enhancements:
Tests: