fix(RHTAP-5642): tssc application should support tssc --version to display the application version #1292
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds build-time version injection via ldflags (GoReleaser and Makefile), introduces a VERSION variable in Makefile, adds an exported Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as tssc-cli (RootCmd)
participant Flags as Flags
participant Const as constants.Version
User->>CLI: tssc-cli [--version]
CLI->>Flags: parse persistent flags
alt --version present
Flags->>Const: read Version
CLI->>User: print app name + Version
CLI->>User: exit 0
else no --version and no subcommand
CLI->>User: show help
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: raisavarghese The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
raisavarghese
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
.goreleaser.yaml (1)
20-21: Prefer GoReleaser’s built-in version template over env coupling.Relying on .Env.VERSION makes builds depend on Makefile/export order. Use GoReleaser’s {{ .Version }} to source the tag/version it computes.
- ldflags: - - -X github.com/redhat-appstudio/tssc-cli/pkg/constants.Version={{ .Env.VERSION }} + ldflags: + - -X github.com/redhat-appstudio/tssc-cli/pkg/constants.Version={{ .Version }}pkg/constants/contants.go (1)
21-22: LGTM; exported var works with -ldflags -X.Minor nit: consider renaming file to constants.go to avoid the “contants” typo for discoverability.
Makefile (1)
69-69: OK to inject via -ldflags; optional size flags.This works. Optionally add -s -w to strip symbols for smaller binaries.
-go build -ldflags "-X github.com/redhat-appstudio/tssc-cli/pkg/constants.Version=$(VERSION)" -o $(BIN) $(CMD) +go build -ldflags "-s -w -X github.com/redhat-appstudio/tssc-cli/pkg/constants.Version=$(VERSION)" -o $(BIN) $(CMD)pkg/flags/flags.go (2)
67-71: Avoid hardcoding app name; print constants.AppName.Keeps output consistent if AppName changes.
-func (f *Flags) ShowVersion() { - fmt.Printf("tssc %s\n", constants.Version) - os.Exit(0) -} +func (f *Flags) ShowVersion() { + fmt.Printf("%s %s\n", constants.AppName, constants.Version) + os.Exit(0) +}
31-31: Flag wiring looks good. Consider short alias if free (optional).If -v isn’t used for verbosity elsewhere, adding it improves UX: --version/-v.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.goreleaser.yaml(1 hunks)Makefile(2 hunks)pkg/cmd/root.go(1 hunks)pkg/constants/contants.go(1 hunks)pkg/flags/flags.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / tssc-cli-on-pull-request
| # Version will be set at build time via git describe | ||
| VERSION ?= $(shell git describe --tags --abbrev=0) | ||
|
|
There was a problem hiding this comment.
VERSION computation fragile on repos without tags or shallow clones.
git describe --abbrev=0 fails if no tag is reachable, yielding an empty version. Provide a resilient fallback (and prefer GITHUB_REF_NAME when present).
-# Version will be set at build time via git describe
-VERSION ?= $(shell git describe --tags --abbrev=0)
+# Version derivation (CI tag > git describe > dev)
+VERSION ?= $(shell \
+ if [ -n "$${GITHUB_REF_NAME}" ]; then echo "$${GITHUB_REF_NAME}"; \
+ else git describe --tags --dirty --always 2>/dev/null || echo dev; fi)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Version will be set at build time via git describe | |
| VERSION ?= $(shell git describe --tags --abbrev=0) | |
| # Version derivation (CI tag > git describe > dev) | |
| VERSION ?= $(shell \ | |
| if [ -n "$${GITHUB_REF_NAME}" ]; then echo "$${GITHUB_REF_NAME}"; \ | |
| else git describe --tags --dirty --always 2>/dev/null || echo dev; fi) |
🤖 Prompt for AI Agents
Makefile around lines 53-55: VERSION computation is fragile when no tags or in
shallow clones; update the assignment to prefer GITHUB_REF_NAME when present,
otherwise try git describe --tags --abbrev=0 but fall back resiliently (attempt
a lightweight git fetch --tags if describe fails, then fall back to git
rev-parse --short HEAD, and finally "unknown" if all else fails). Implement this
as a single shell expression in the Makefile that checks GITHUB_REF_NAME first,
then runs git describe with a guarded fetch and fallbacks using || so VERSION is
never empty.
| // Handle version flag | ||
| r.cmd.RunE = func(cmd *cobra.Command, args []string) error { | ||
| if r.flags.Version { | ||
| r.flags.ShowVersion() | ||
| return nil | ||
| } | ||
| return cmd.Help() | ||
| } |
There was a problem hiding this comment.
--version isn’t truly “global”; root RunE won’t fire when a subcommand is invoked.
With Cobra, root RunE is skipped when a subcommand runs. tssc deploy --version won’t print the version. Handle it in PersistentPreRunE instead.
- // Handle version flag
- r.cmd.RunE = func(cmd *cobra.Command, args []string) error {
- if r.flags.Version {
- r.flags.ShowVersion()
- return nil
- }
- return cmd.Help()
- }
+ // Handle --version globally so it works with subcommands, too.
+ r.cmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
+ if r.flags.Version {
+ r.flags.ShowVersion() // exits 0
+ }
+ return nil
+ }
+ // When no subcommand is provided, show help.
+ r.cmd.RunE = func(cmd *cobra.Command, args []string) error {
+ return cmd.Help()
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle version flag | |
| r.cmd.RunE = func(cmd *cobra.Command, args []string) error { | |
| if r.flags.Version { | |
| r.flags.ShowVersion() | |
| return nil | |
| } | |
| return cmd.Help() | |
| } | |
| // Handle --version globally so it works with subcommands, too. | |
| r.cmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { | |
| if r.flags.Version { | |
| r.flags.ShowVersion() // exits 0 | |
| } | |
| return nil | |
| } | |
| // When no subcommand is provided, show help. | |
| r.cmd.RunE = func(cmd *cobra.Command, args []string) error { | |
| return cmd.Help() | |
| } |
🤖 Prompt for AI Agents
In pkg/cmd/root.go around lines 27 to 34, the version check is in RunE which
doesn't execute for subcommands; move the logic into r.cmd.PersistentPreRunE so
it runs for every invocation (including subcommands), and when r.flags.Version
is true call r.flags.ShowVersion() and then stop further command execution
(either by returning a non-nil error or calling os.Exit(0)) so the subcommand
body does not run.
…splay application version - Added the flag --version as Global flags - declared the variable Version in constant package so that flags.go and root.go can access it directly - Updated Makefile to inject version via ldflags - Version injection configured in goreleaser.yaml to support different build methods
raisavarghese
force-pushed
the
RHTAP-5642
branch
from
bfef379 to
725489a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/constants/contants.go (1)
1-23: CRITICAL: Fix the filename typo.The file is named
contants.gobut should beconstants.go(missing an "s"). This typo:
- Creates inconsistency with the package name
constants- May cause confusion for developers navigating the codebase
- Could lead to mismatches in ldflags injection paths if the build configuration uses the correct spelling
Rename the file:
git mv pkg/constants/contants.go pkg/constants/constants.goAfter renaming, verify that all ldflags injection paths in Makefile and .goreleaser.yaml use the correct path
github.com/redhat-appstudio/tssc-cli/pkg/constants.Version.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.goreleaser.yaml(1 hunks)Makefile(2 hunks)pkg/cmd/root.go(1 hunks)pkg/constants/contants.go(1 hunks)pkg/flags/flags.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- .goreleaser.yaml
- pkg/cmd/root.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/flags/flags.go (1)
pkg/constants/contants.go (2)
Version(22-22)AppName(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / tssc-cli-on-pull-request
🔇 Additional comments (3)
pkg/constants/contants.go (1)
21-22: LGTM! Version variable correctly implements ldflags injection pattern.The Version variable declaration follows the standard Go pattern for build-time version injection via ldflags. The default value "v0.0.0-SNAPSHOT" appropriately indicates development/unversioned builds.
pkg/flags/flags.go (2)
13-13: LGTM! Import correctly references the constants package.
24-24: LGTM! Version flag follows established patterns.The Version field, flag registration, and initialization follow the same patterns used by other boolean flags in this struct (Debug, DryRun).
Also applies to: 31-31, 93-93
| // ShowVersion shows the application version and exits | ||
| func (f *Flags) ShowVersion() { | ||
| fmt.Printf("%s %s\n", constants.AppName, constants.Version) | ||
| os.Exit(0) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor to avoid os.Exit and improve testability.
Calling os.Exit(0) directly has several drawbacks:
- Prevents deferred cleanup functions from running
- Makes the function untestable without subprocess testing
- Violates the principle of separation of concerns (display vs. program termination)
- Library code should not control program exit
The version display logic should be handled in the command execution layer (root.go's RunE) which can return early after printing the version.
Refactor ShowVersion to only print the version without exiting:
-// ShowVersion shows the application version and exits
+// ShowVersion prints the application version
func (f *Flags) ShowVersion() {
fmt.Printf("%s %s\n", constants.AppName, constants.Version)
- os.Exit(0)
}The caller in pkg/cmd/root.go should handle the exit behavior:
if flags.Version {
flags.ShowVersion()
return nil // or cmd.SilenceUsage = true; return nil
}This approach:
- Allows testing ShowVersion without process termination
- Enables proper cleanup via defer statements
- Follows Cobra's standard pattern for early returns
- Makes the code more modular and maintainable
🤖 Prompt for AI Agents
In pkg/flags/flags.go around lines 67 to 71, ShowVersion currently prints the
version and calls os.Exit(0); remove the os.Exit call so ShowVersion only prints
the application name and version (no process termination) to make it testable
and avoid preventing deferred cleanup, then update the command execution layer
(pkg/cmd/root.go) to check flags.Version, call flags.ShowVersion(), and perform
the early return (e.g., set cmd.SilenceUsage = true and return nil) so
termination/flow control is handled by the caller rather than the library
function.
1747a57 to
725489a
Compare
…splay application version - Added the flag --version as Global flags - declared the variable Version in constant package so that flags.go and root.go can access it directly - Updated Makefile to inject version via ldflags - Version injection configured in goreleaser.yaml to support different build methods
raisavarghese
force-pushed
the
RHTAP-5642
branch
from
725489a to
c0f0dac
Compare
|
|
@raisavarghese: The following test has Failed, say /retest to rerun failed tests.
Inspecting Test ArtifactsTo inspect your test artifacts, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/rhtap-team/rhtap-cli:e2e-4.19-fnd5mTest results analysis<not enabled> OCI Artifact Browser URL<not enabled> |
…splay application version
Summary by CodeRabbit
New Features
Chores