Remove version command and simplify version output#107
Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
gtsiolis
force-pushed
the
george/des-165
branch
from
36f8f3c to
7b4c80b
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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)
📝 Coding Plan
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 (2)
cmd/version.go (1)
13-15: Remove the now-dead build metadata plumbing.
versionLine()now only consumesversion.Version(), sointernal/version.Commit(),internal/version.BuildDate(), and the matching ldflags in.goreleaser.yamlare dead. Cleaning those up in the same change would keep the version model truly single-source and avoid shipping unused metadata in release builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/version.go` around lines 13 - 15, versionLine() now only uses version.Version(), so remove the unused build metadata plumbing: delete internal/version.Commit() and internal/version.BuildDate() implementations and any callers, remove the corresponding ldflags from .goreleaser.yaml, and consolidate the source of truth to version.Version() (leave version.Version() intact); update any docs/comments referencing Commit/BuildDate to avoid dead code and ensure releases no longer include those unused fields.cmd/root.go (1)
48-56: Call out thelstk versionremoval in release notes or migration docs.Dropping the subcommand is a CLI breaking change for automation that still invokes
lstk version. If the hard removal is intentional, I'd make sure the release notes explicitly point users tolstk --version/lstk -v.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 48 - 56, The CLI removed the legacy "lstk version" subcommand (see root.AddCommand where subcommands like newStartCmd/newStopCmd are registered), which is a breaking change for users and automation; update the release notes and migration documentation to explicitly call out removal of "lstk version", mention the supported alternatives "lstk --version" and "lstk -v", and provide a short migration note or example command for CI scripts to replace the old subcommand with the new flag-based usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/non_interactive_test.go`:
- Around line 47-52: The tests TestNonInteractiveFlagBindsToCfg (and the similar
test at 58-62) currently ignore the error from root.Execute() and then assert on
cfg.NonInteractive; change them to check the returned error first (e.g.,
require.NoError(t, err) or if err != nil { t.Fatalf(...)}), then assert
cfg.NonInteractive so a failing Execute() (such as broken --version handling)
will fail the test instead of producing a false positive; locate root :=
NewRootCmd(...) and the subsequent root.Execute() call and update the test to
capture and assert on the error before inspecting cfg.NonInteractive.
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 48-56: The CLI removed the legacy "lstk version" subcommand (see
root.AddCommand where subcommands like newStartCmd/newStopCmd are registered),
which is a breaking change for users and automation; update the release notes
and migration documentation to explicitly call out removal of "lstk version",
mention the supported alternatives "lstk --version" and "lstk -v", and provide a
short migration note or example command for CI scripts to replace the old
subcommand with the new flag-based usage.
In `@cmd/version.go`:
- Around line 13-15: versionLine() now only uses version.Version(), so remove
the unused build metadata plumbing: delete internal/version.Commit() and
internal/version.BuildDate() implementations and any callers, remove the
corresponding ldflags from .goreleaser.yaml, and consolidate the source of truth
to version.Version() (leave version.Version() intact); update any docs/comments
referencing Commit/BuildDate to avoid dead code and ensure releases no longer
include those unused fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 79c82fcd-4c6a-48e0-ac5c-2b1f4a3b9aa5
📒 Files selected for processing (6)
cmd/help_test.gocmd/non_interactive_test.gocmd/root.gocmd/version.gocmd/version_test.gotest/integration/update_test.go
|
The build metadata is needed for goreleaser so it has to stay there |
gtsiolis
force-pushed
the
george/des-165
branch
from
7b4c80b to
f50c7a0
Compare
|
Thanks for taking a look and the insight, @silv-io! Ok then, could we keep the linker-populated build metadata in the binary for Goreleaser and only removed it from the public version output. So |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/root.go`:
- Around line 44-46: The version flag only has the long form; add the -v
shorthand after calling root.InitDefaultVersionFlag() by setting the shorthand
on the existing flag (e.g., use root.Flags().Lookup("version").Shorthand = "v"
and ensure its Usage remains "Show version") so both -v and --version behave
identically; this touches the block using root.InitDefaultVersionFlag(),
root.Flags().Lookup("version").Usage and
root.SetVersionTemplate(versionTemplate()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 74dc0690-06ee-44bc-9d91-626f9728e33c
📒 Files selected for processing (8)
README.mdcmd/help_test.gocmd/non_interactive_test.gocmd/root.gocmd/version.gocmd/version_test.gointernal/version/version.gotest/integration/update_test.go
💤 Files with no reviewable changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- test/integration/update_test.go
- cmd/help_test.go
- cmd/version.go
- cmd/non_interactive_test.go
gtsiolis
force-pushed
the
george/des-165
branch
from
be6ed22 to
da3f8bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/integration/login_test.go`:
- Line 11: The test currently skips when os.Getenv("LOCALSTACK_AUTH_TOKEN") is
empty; since this integration test uses a mocked API server, remove or relax
that env-gate in the login_test.go test (the block checking
LOCALSTACK_AUTH_TOKEN and calling t.Skip) so the test can run with a
synthetic/dummy token instead—either default the token to a known test value
when empty or remove the t.Skip call and ensure the mocked client uses the
synthetic token; update any references to LOCALSTACK_AUTH_TOKEN in the test
setup to accept the fallback token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 160e3b3c-12c5-4378-a597-e8c6cf07deff
📒 Files selected for processing (10)
README.mdcmd/help_test.gocmd/non_interactive_test.gocmd/root.gocmd/version.gocmd/version_test.gointernal/version/version.gotest/integration/logging_test.gotest/integration/login_test.gotest/integration/update_test.go
💤 Files with no reviewable changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/help_test.go
- cmd/version_test.go
- cmd/non_interactive_test.go
- test/integration/update_test.go
- internal/version/version.go
gtsiolis
force-pushed
the
george/des-165
branch
from
1f7b050 to
dab153a
Compare
|
Pushed a "clean-up" commit 3146cf9. @silv-io can you have a look?
|
silv-io
left a comment
silv-io
left a comment
There was a problem hiding this comment.
LGTM!
Thanks for fixing it up.
IMO a version command will be useful for also showing info about the images we currently have downloaded already. But we can add that back in the future.
Maybe this can also just go into the doctor command I was thinking about.
|
Thanks, @silv-io @carole-lavillonniere! 🏀 |
3 participants
This PR simplifies version reporting in
lstkand aligns it with common CLI conventions.versionsubcommand-vand--versionas the supported version entry pointslstk 0.4.1for production andlstk devfor developmentThe previous setup exposed three different version entry points and printed extra build metadata by default. That added surface area without much user value. This change narrows the interface to the standard flag-based pattern most CLIs use, which is simpler for both humans and automation:
This will also remove the version.Commit and version.BuildDate. I know
uvincludes commit and build metadata in a compact format, but forlstkthe current output structure adds noise and makes the binary feel more like a development build than a polished release.For reference:
lstk --versionuv --version