Conversation
28eac74 to
4364dba
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances the version command to provide richer debug-friendly build/runtime metadata, and updates build/test plumbing to support it.
Changes:
- Add build metadata variables (
BuildDate,BuildCommit) and embed them viago build -ldflagsinbuild.sh. - Expand
versioncommand output (JSON + styled text) to include commit/date/Go version/platform. - Update tests for the new version output, and adjust
AMTBaseCmd.AfterApplyretry behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/constants.go | Introduces build metadata globals (BuildDate, BuildCommit) used by the version command. |
| internal/commands/version.go | Extends version output with build/runtime info and adds styled text rendering. |
| internal/commands/version_test.go | Updates assertions to cover new JSON fields and styled output content; disables lipgloss colors in tests. |
| internal/commands/base.go | Changes GetControlMode retry loop behavior (affects SkipWSMANSetup flows). |
| build.sh | Adds build date/commit capture and injects them into binaries via -X ldflags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func init() { | ||
| // Disable colors in tests so assertions match plain text. | ||
| lipgloss.SetColorProfile(termenv.Ascii) | ||
| } | ||
|
|
||
| func TestVersionCmd_Run_PlainText(t *testing.T) { |
There was a problem hiding this comment.
This package-level init() mutates global lipgloss state for the entire commands test package, which can create hidden coupling with other tests (and makes it harder to re-enable colors for specific assertions). Prefer using TestMain to set/restore global state, or set the color profile within the specific tests that capture output and restore it via t.Cleanup.
| func init() { | |
| // Disable colors in tests so assertions match plain text. | |
| lipgloss.SetColorProfile(termenv.Ascii) | |
| } | |
| func TestVersionCmd_Run_PlainText(t *testing.T) { | |
| func setASCIIColorProfile(t *testing.T) { | |
| t.Helper() | |
| previousProfile := lipgloss.ColorProfile() | |
| lipgloss.SetColorProfile(termenv.Ascii) | |
| t.Cleanup(func() { | |
| lipgloss.SetColorProfile(previousProfile) | |
| }) | |
| } | |
| func TestVersionCmd_Run_PlainText(t *testing.T) { | |
| setASCIIColorProfile(t) |
| for attempt := 1; attempt <= maxAttempts; attempt++ { | ||
| controlMode, err = amtCommand.GetControlMode() | ||
| if err == nil { | ||
| break | ||
| } |
There was a problem hiding this comment.
This retry loop now always uses maxAttempts even when SkipWSMANSetup is true. Previously (per the removed effectiveMaxAttempts logic) SkipWSMANSetup avoided the backoff/retry delay, letting commands like amtinfo degrade quickly when HECI is unavailable. If the intent is still to be fast for SkipWSMANSetup, consider restoring the reduced-attempt behavior to avoid up to ~12s delay before falling back.
| effectiveMaxAttempts := maxAttempts | ||
|
|
||
| for attempt := 1; attempt <= effectiveMaxAttempts; attempt++ { | ||
| for attempt := 1; attempt <= maxAttempts; attempt++ { |
There was a problem hiding this comment.
This change modifies AMTBaseCmd.AfterApply retry behavior (unrelated to the PR’s stated goal of enhancing the version command). If this is intentional, it would help to move it into a separate PR or document the rationale here; otherwise it risks introducing behavior changes that reviewers may miss when focusing on the version output work.
3eb171b to
39d83fc
Compare
No description provided.