-
Notifications
You must be signed in to change notification settings - Fork 1.6k
✨ Improve version command output: add runtime fallbacks and unit tests. #4898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
be710fe
a402ee3
c4a0d3e
e5082b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -17,58 +17,91 @@ limitations under the License. | |||||||||
package cmd | ||||||||||
|
||||||||||
import ( | ||||||||||
"encoding/json" | ||||||||||
"fmt" | ||||||||||
"runtime" | ||||||||||
"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 | ||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we keep the previous comments? |
||||||||||
gitCommit = "$Format:%H$" | ||||||||||
buildDate = "1970-01-01T00:00:00Z" | ||||||||||
) | ||||||||||
|
||||||||||
// version contains all the information related to the CLI version | ||||||||||
type version struct { | ||||||||||
// VersionInfo holds all CLI version-related information | ||||||||||
type VersionInfo struct { | ||||||||||
Comment on lines
+38
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Can we avoid externalising this one? 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. |
||||||||||
KubeBuilderVersion string `json:"kubeBuilderVersion"` | ||||||||||
KubernetesVendor string `json:"kubernetesVendor"` | ||||||||||
GitCommit string `json:"gitCommit"` | ||||||||||
BuildDate string `json:"buildDate"` | ||||||||||
GoOs string `json:"goOs"` | ||||||||||
GoOS string `json:"goOs"` | ||||||||||
GoArch string `json:"goArch"` | ||||||||||
} | ||||||||||
|
||||||||||
// versionString returns the Full CLI version | ||||||||||
func versionString() string { | ||||||||||
// resolveBuildInfo ensures dynamic fields are populated | ||||||||||
func resolveBuildInfo() { | ||||||||||
if kubeBuilderVersion == unknown { | ||||||||||
if info, ok := debug.ReadBuildInfo(); ok && info.Main.Version != "" { | ||||||||||
kubeBuilderVersion = info.Main.Version | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
return fmt.Sprintf("Version: %#v", version{ | ||||||||||
kubeBuilderVersion, | ||||||||||
kubernetesVendorVersion, | ||||||||||
gitCommit, | ||||||||||
buildDate, | ||||||||||
goos, | ||||||||||
goarch, | ||||||||||
}) | ||||||||||
if goos == unknown { | ||||||||||
goos = runtime.GOOS | ||||||||||
} | ||||||||||
if goarch == unknown { | ||||||||||
goarch = runtime.GOARCH | ||||||||||
} | ||||||||||
Comment on lines
+55
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks so much for the suggestion! 🙌 However, I think we should not fallback to 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.
—even for macOS binaries like kubebuilder version We already inject the correct values using Could we please revert it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, see : https://github.com/kubernetes-sigs/kubebuilder/pull/4516/files |
||||||||||
if gitCommit == "$Format:%H$" || gitCommit == "" { | ||||||||||
gitCommit = unknown | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
// getKubebuilderVersion returns only the CLI version string | ||||||||||
func getKubebuilderVersion() string { | ||||||||||
if kubeBuilderVersion == unknown { | ||||||||||
if info, ok := debug.ReadBuildInfo(); ok && info.Main.Version != "" { | ||||||||||
kubeBuilderVersion = info.Main.Version | ||||||||||
} | ||||||||||
// getVersionInfo returns populated VersionInfo | ||||||||||
func getVersionInfo() VersionInfo { | ||||||||||
resolveBuildInfo() | ||||||||||
return VersionInfo{ | ||||||||||
KubeBuilderVersion: kubeBuilderVersion, | ||||||||||
KubernetesVendor: kubernetesVendorVersion, | ||||||||||
GitCommit: gitCommit, | ||||||||||
BuildDate: buildDate, | ||||||||||
GoOS: goos, | ||||||||||
GoArch: goarch, | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
// 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, | ||||||||||
) | ||||||||||
} | ||||||||||
Comment on lines
+79
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @camilamacedo86 @Thedarkmatter10 could we move the output formatting to 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that make sense we could do something like
But I would accept it in a follow up PR as well |
||||||||||
|
||||||||||
// getKubeBuilderVersion returns just the CLI version | ||||||||||
func getKubeBuilderVersion() string { | ||||||||||
resolveBuildInfo() | ||||||||||
return kubeBuilderVersion | ||||||||||
} | ||||||||||
|
||||||||||
// versionJSON returns version as JSON string | ||||||||||
func versionJSON() string { | ||||||||||
v := getVersionInfo() | ||||||||||
b, _ := json.MarshalIndent(v, "", " ") | ||||||||||
camilamacedo86 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Positive FeedbackNegative Feedback |
||||||||||
return string(b) | ||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
/* | ||
Copyright 2017 The Kubernetes Authors. | ||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
http://www.apache.org/licenses/LICENSE-2.0 | ||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package cmd | ||
|
||
import ( | ||
"encoding/json" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestVersionStringIncludesExpectedFields(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Amazing 🚀 |
||
output := versionString() | ||
|
||
assert.Contains(t, output, "KubeBuilder Version:") | ||
assert.Contains(t, output, "Kubernetes Vendor:") | ||
assert.Contains(t, output, "Git Commit:") | ||
assert.Contains(t, output, "Build Date:") | ||
assert.Contains(t, output, "Go OS/Arch:") | ||
} | ||
|
||
func TestVersionJSONFormatAndKeys(t *testing.T) { | ||
jsonStr := versionJSON() | ||
|
||
var result map[string]string | ||
err := json.Unmarshal([]byte(jsonStr), &result) | ||
assert.NoError(t, err) | ||
|
||
assert.Contains(t, result, "kubeBuilderVersion") | ||
assert.Contains(t, result, "kubernetesVendor") | ||
assert.Contains(t, result, "gitCommit") | ||
assert.Contains(t, result, "buildDate") | ||
assert.Contains(t, result, "goOs") | ||
assert.Contains(t, result, "goArch") | ||
} | ||
|
||
func TestGetVersionInfoFieldsArePopulated(t *testing.T) { | ||
v := getVersionInfo() | ||
|
||
assert.NotEmpty(t, v.KubeBuilderVersion) | ||
assert.NotEmpty(t, v.KubernetesVendor) | ||
assert.NotEmpty(t, v.GitCommit) | ||
assert.NotEmpty(t, v.BuildDate) | ||
assert.NotEmpty(t, v.GoOS) | ||
assert.NotEmpty(t, v.GoArch) | ||
|
||
assert.NotEqual(t, "unknown", v.KubeBuilderVersion, "KubeBuilderVersion should not be 'unknown'") | ||
assert.NotEqual(t, "unknown", v.GoOS, "GoOS should not be 'unknown'") | ||
assert.NotEqual(t, "unknown", v.GoArch, "GoArch should not be 'unknown'") | ||
assert.NotEqual(t, "$Format:%H$", v.GitCommit, "GitCommit should not be default placeholder") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we keep the previous comments?