Remove version command and simplify version output#107
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR removes 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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![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
| func TestNonInteractiveFlagBindsToCfg(t *testing.T) { | ||
| cfg := &env.Env{} | ||
| root := NewRootCmd(cfg, telemetry.New("", true)) | ||
| root.SetArgs([]string{"--non-interactive", "version"}) | ||
| root.SetArgs([]string{"--non-interactive", "--version"}) | ||
| _ = root.Execute() | ||
|
|
There was a problem hiding this comment.
Assert that Execute() succeeds in these flag-binding tests.
Both tests can still flip cfg.NonInteractive during flag parsing even if --version is broken, so they may pass while the user-facing path fails. Please check the returned error before asserting on cfg.
Suggested tightening
func TestNonInteractiveFlagBindsToCfg(t *testing.T) {
cfg := &env.Env{}
root := NewRootCmd(cfg, telemetry.New("", true))
root.SetArgs([]string{"--non-interactive", "--version"})
- _ = root.Execute()
+ if err := root.Execute(); err != nil {
+ t.Fatalf("expected Execute to succeed, got %v", err)
+ }
if !cfg.NonInteractive {
t.Fatal("expected cfg.NonInteractive to be true after --non-interactive flag")
}
}
@@
func TestNonInteractiveFlagDefaultIsOff(t *testing.T) {
cfg := &env.Env{}
root := NewRootCmd(cfg, telemetry.New("", true))
root.SetArgs([]string{"--version"})
- _ = root.Execute()
+ if err := root.Execute(); err != nil {
+ t.Fatalf("expected Execute to succeed, got %v", err)
+ }
if cfg.NonInteractive {
t.Fatal("expected cfg.NonInteractive to be false when flag is not set")
}
}Also applies to: 58-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/non_interactive_test.go` around lines 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.
|
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
| root.InitDefaultVersionFlag() | ||
| root.Flags().Lookup("version").Usage = "Show version" | ||
| root.SetVersionTemplate(versionTemplate()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether this repository explicitly wires a -v shorthand for the version flag.
set -euo pipefail
echo "== uses of InitDefaultVersionFlag =="
rg -nP --type=go '\bInitDefaultVersionFlag\s*\('
echo
echo "== explicit version shorthand registrations =="
rg -nP --type=go 'Bool(?:Var)?P\([^)]*"version"\s*,\s*"v"'
rg -nP --type=go 'Lookup\("version"\)\.Shorthand|Shorthand\s*=\s*"v"'Repository: localstack/lstk
Length of output: 189
🏁 Script executed:
fd -type f -name "*test*.go" cmd/ | head -20Repository: localstack/lstk
Length of output: 229
🏁 Script executed:
rg -nP --type=go "TestVersionFlagsPrintSameOutput" --context=5Repository: localstack/lstk
Length of output: 583
🏁 Script executed:
# Also check the git diff or PR changes to understand context
git log --oneline -10Repository: localstack/lstk
Length of output: 110
🏁 Script executed:
# Check if there are any recent changes to cmd/root.go
git log --oneline cmd/root.go | head -5Repository: localstack/lstk
Length of output: 110
🏁 Script executed:
cat -n cmd/version_test.goRepository: localstack/lstk
Length of output: 1034
🏁 Script executed:
# Also check for any calls to root command with -v flag
rg -nP --type=go '"\-v"' cmd/ | head -20Repository: localstack/lstk
Length of output: 124
🏁 Script executed:
cat -n cmd/root.go | head -80Repository: localstack/lstk
Length of output: 2695
🏁 Script executed:
# Search for any other version-related wiring in the codebase
rg -nP --type=go "version" cmd/root.go | head -20Repository: localstack/lstk
Length of output: 303
Wire the -v shorthand for the version flag.
The test at cmd/version_test.go:16 expects both -v and --version flags to produce identical output (lines 17, 22, 30-32), but the current code only registers --version. Cobra's InitDefaultVersionFlag() does not include a shorthand; the -v flag must be explicitly registered.
Suggested fix
- root.InitDefaultVersionFlag()
- root.Flags().Lookup("version").Usage = "Show version"
+ root.Flags().BoolP("version", "v", false, "Show version")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/root.go` around lines 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()).
2 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