✨ Improve version command output: add runtime fallbacks and unit tests. #4898
Conversation
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @Thedarkmatter10! |
|
Hi @Thedarkmatter10. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Thedarkmatter10 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 |
k8s-ci-robot
added
cncf-cla: yes
Thedarkmatter10
changed the title
Thedarkmatter10
force-pushed
the
version/refactor-add-tests
branch
from
3a825c1 to
0610694
Compare
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Thedarkmatter10
force-pushed
the
version/refactor-add-tests
branch
from
0610694 to
63173f7
Compare
|
See that the testdata job will be fixed with: #4905 By last since it will produce a change for end users it should end up in our release notes We need to use like ✨ and add a text that clarifies what was done such as :
Could you please update those things? |
Thedarkmatter10
changed the title
Thedarkmatter10
force-pushed
the
version/refactor-add-tests
branch
2 times, most recently
from
df6f0c9 to
5649294
Compare
Hi @camilamacedo86 👋 I tried squashing the commits into one as suggested, but I ran into some issues with rebase and merge conflicts that I wasn't able to resolve cleanly. As a result, the PR still shows extra commits. Apologies for the inconvenience caused 🙏 . I understand the importance of keeping the commit history clean and will make sure not to repeat this in future contributions. Please let me know if you'd prefer that I close this PR and open a fresh one with a clean history, or if there's another way you'd like to proceed. Thanks again for your guidance and patience! 😊 |
|
/test pull-kubebuilder-e2e-k8s-1-32-0 |
| // versionString returns a human-friendly string version | ||
| func versionString() string { | ||
| v := getVersionInfo() | ||
| return fmt.Sprintf(`KubeBuilder Version: %s | ||
| Kubernetes Vendor: %s | ||
| Git Commit: %s | ||
| Build Date: %s | ||
| Go OS/Arch: %s/%s`, | ||
| v.KubeBuilderVersion, | ||
| v.KubernetesVendor, | ||
| v.GitCommit, | ||
| v.BuildDate, | ||
| v.GoOS, | ||
| v.GoArch, | ||
| ) | ||
| } |
There was a problem hiding this comment.
@camilamacedo86 @Thedarkmatter10 could we move the output formatting to cli/version.go?
I think that separating the data gathering logic from the presentation logic could give us more flexibility to deal with output formatting, add flags, and leave room for other tools/commands to define how they want to display the version. The same goes for the JSON output. We could turn that into a flag in cli/version.go.
This separation was discussed briefly in a previous PR conversation (before I messed up and closed the PR).
WDYT?
There was a problem hiding this comment.
I think that make sense we could do something like
if jsonOutput {
printAsJSON(GetVersionInfo())
} else {
printPretty(GetVersionInfo())
}
But I would accept it in a follow up PR as well
I think the goal of this one ( unless I misunderstood) would be add only the printPretty part
|
This one is not passing all tests. |
Thedarkmatter10
force-pushed
the
version/refactor-add-tests
branch
from
5649294 to
a402ee3
Compare
Hi @camilamacedo86 , I’ve completed the rebase successfully. Regarding the multiple file changes that are showing up: these seem to be caused by running golangci-lint on my Windows machine, which has likely introduced differences in line endings (CRLF vs LF). This ends up marking a lot of files as changed even though there are no logical code modifications. Since changing all these files would introduce noise in the PR, I’ve held off committing them. Would you be able to re-run golangci-lint on a Unix/Linux environment on your side to avoid the line-ending conflict? Or alternatively, let me know how you'd prefer to handle this I’m happy to follow any suggestion. Thanks! |
|
Thank you for your effort in this one.
Could you run go mod tidy Also, this one should tirgger: https://github.com/kubernetes-sigs/kubebuilder/blob/master/.github/workflows/release-version-ci.yml . The path is wrong, we need to add cmd/ as well. Could you please update it to add the path? By last we need to have all in the same commit, could you please squash? |
k8s-ci-robot
added
the
needs-rebase
CI trigger path and mod file.
Thedarkmatter10
force-pushed
the
version/refactor-add-tests
branch
from
af7db2f to
c4a0d3e
Compare
k8s-ci-robot
removed
the
needs-rebase
|
Hi @camilamacedo86, I hope you're doing well. Thank you |
|
Hi @Thedarkmatter10, Hope you're doing well! Just wanted to kindly check if there’s anything else pending on my side to get this PR merged. Please let me know if any further changes are needed. I saw you’ve resolved all CI issues and rebased—thank you! 🎉 Could you please squash the commits? We aim for 1 commit per PR = 1 problem = 1 solution. This helps keep our git history clean and avoids conflicts when reverting or cherry-picking changes. Once that’s done, I’ll take a final look. Now that all tests are green, it should be quick! |
| "runtime/debug" | ||
| ) | ||
|
|
||
| const unknown = "unknown" | ||
|
|
||
| // var needs to be used instead of const as ldflags is used to fill this | ||
| // information in the release process | ||
| // These are filled via ldflags during build |
There was a problem hiding this comment.
Could we keep the previous comments?
| var ( | ||
| kubeBuilderVersion = unknown | ||
| kubernetesVendorVersion = "1.33.0" | ||
| goos = unknown | ||
| goarch = unknown | ||
| gitCommit = "$Format:%H$" // sha1 from git, output of $(git rev-parse HEAD) | ||
|
|
||
| buildDate = "1970-01-01T00:00:00Z" // build date in ISO8601 format, output of $(date -u +'%Y-%m-%dT%H:%M:%SZ') |
There was a problem hiding this comment.
Could we keep the previous comments?
| // VersionInfo holds all CLI version-related information | ||
| type VersionInfo struct { |
There was a problem hiding this comment.
| // VersionInfo holds all CLI version-related information | |
| type VersionInfo struct { | |
| // version holds all CLI version-related information | |
| type version struct { |
Can we avoid externalising this one?
If we externalise it (e.g., by using v instead of V), it becomes accessible outside of Kubebuilder and effectively becomes part of its public API.
We intentionally do not want to expose this structure for use by other projects. Let's keep it internal to maintain tighter control and avoid unintended usage.
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestVersionStringIncludesExpectedFields(t *testing.T) { |
| if goos == unknown { | ||
| goos = runtime.GOOS | ||
| } | ||
| if goarch == unknown { | ||
| goarch = runtime.GOARCH | ||
| } |
There was a problem hiding this comment.
Thanks so much for the suggestion! 🙌
However, I think we should not fallback to runtime.GOOS / runtime.GOARCH here.
if goos == unknown {
goos = runtime.GOOS
}
if goarch == unknown {
goarch = runtime.GOARCH
}This will always reflect the build-time platform — not the actual target platform of the binary.
For example, in our case, all builds run on Ubuntu (see our GoReleaser config), so this will return:
GOOS = "linux"
GOARCH = "amd64"
—even for macOS binaries like darwin/amd64. This results in incorrect output when users run:
kubebuilder versionWe already inject the correct values using -ldflags at build time, and if something goes wrong with that injection, it's better to leave the values as "unknown" to surface the issue, rather than silently reporting incorrect metadata.
Could we please revert it?
There was a problem hiding this comment.
Also, see : https://github.com/kubernetes-sigs/kubebuilder/pull/4516/files
It can either be used if kubebuilder is installed with go install module
(another behaviour that we need to consider)
There was a problem hiding this comment.
Really appreciate your contribution here 🥇 — and sorry for the delay in reviewing it!
This change definitely makes the output nicer (thank you for that!) 🚀, but we have to be extra cautious because it touches parts that can affect things in more subtle ways — especially around how the CLI is released and used as a Go module or by other libraries.
Even small changes here can impact consumers or break the release process in ways that aren’t easily caught by tests. That’s why we need to be conservative when altering this part of the code.
Thanks again for the effort — it’s super valuable, and we’d love to find a safe path forward if we can!
I've added a few comments — the main concerns are:
-
These methods should remain private. They're internal to Kubebuilder, and exposing them would mean supporting them as part of our public API, which we don’t intend to do.
-
The fallback logic:
if goos == unknown { goos = runtime.GOOS } if goarch == unknown { goarch = runtime.GOARCH }
can introduce misleading output. Since our builds run on Ubuntu
(GoReleaser config), this would always return:linux/amd64
—even for macOS binaries, which can confuse users and scripts relying on
kubebuilder version.
Would you be open to adjusting the implementation to:
- Keep the fallback logic removed — rely on
ldflagsonly - Limit the scope of the PR to just the necessary improvements
- Keep internal methods unexported and avoid API surface changes
We really appreciate the effort and want to support your contribution!
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the version command logic to improve maintainability, testability, and robustness by exposing version helpers, adding runtime fallbacks, and providing comprehensive test coverage.
- Exposed internal version structures and functions for programmatic access
- Added runtime fallbacks for OS/Arch detection and Git commit validation
- Introduced comprehensive unit tests to validate version command behavior
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| go.mod | Adds testify dependency for unit testing |
| cmd/version_test.go | Comprehensive unit tests for version command functionality |
| cmd/version.go | Refactored version logic with exposed VersionInfo struct and improved formatting |
| cmd/cmd.go | Function name correction from getKubebuilderVersion to getKubeBuilderVersion |
| // versionJSON returns version as JSON string | ||
| func versionJSON() string { | ||
| v := getVersionInfo() | ||
| b, _ := json.MarshalIndent(v, "", " ") |
There was a problem hiding this comment.
The error from json.MarshalIndent is being ignored. While this is unlikely to fail for a simple struct, it's better practice to handle the error or document why it's safe to ignore.
Copilot uses AI. Check for mistakes.
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
🔧 Refactor version logic: expose functions, add fallbacks, remove duplication, and add tests
Summary
This PR improves the internal versioning logic by making it more maintainable, testable, and robust.
✨ What’s Changed
Publicly exposed version helpers:
Removed duplication (DRY):
Centralized resolution logic using debug.ReadBuildInfo() into a helper
Eliminated repeated fallback code for resolving build info
Added runtime fallbacks:
Uses runtime.GOOS and runtime.GOARCH when not set via ldflags
Ensures gitCommit is validated and not left as$Format:%H$
Improved output formatting:
versionString() provides clean, human-readable output
versionJSON() supports structured, machine-readable output (useful in CI)
Renamed function Name from
getKubebuilderVersion()togetKubeBuilderVersion().Added unit tests (version_test.go):
Validate output includes expected fields
Confirm versionJSON() is parsable and includes required keys
Ensure GetVersionInfo() returns non-empty, resolved values
🧠 Why It Matters
Ensures test coverage for a previously untested code path
Helps catch missing ldflags values (e.g. empty Git commit) early
Lays the foundation for long-term maintainability of CLI version reporting
📈 Impact
Exposes version info for programmatic and CLI use
Adds meaningful tests that reduce risk of silent version resolution failures
Improves developer and CI experience with cleaner output and stronger guarantees