-
Notifications
You must be signed in to change notification settings - Fork 15
main: add osbuild version to version command #332
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: main
Are you sure you want to change the base?
Conversation
This commit refactors the readVersionInfo function to improve code readability and maintainability.
Because I have modified my PATH to include a custom osbuild binary, it is useful to see which osbuild version is being used by image-builder. After this change, the output looks like this: $ go run ./cmd/image-builder --version image-builder: version: unknown commit: unknown dependencies: images: v0.197.0 osbuild: osbuild 162
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.
I like it! One small questin inline and I wonder if we could add some smoke test to test_container_version_smoke for this? (just a single line)
} | ||
|
||
func readVersionFromBinary() *versionDescription { | ||
var osbuildCmd = "osbuild" |
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.
Silly question, why is this a var? Given that its not mocked or anything we could as well just hardcode it in line 56?
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.
Except for my inline comment, this LGTM.
cmd := exec.Command(osbuildCmd, "--version") | ||
var out bytes.Buffer | ||
cmd.Stdout = &out | ||
if err := cmd.Run(); err != nil { | ||
vd.ImageBuilder.Dependencies.OSBuild = fmt.Sprintf("error: %s", err) | ||
} | ||
vd.ImageBuilder.Dependencies.OSBuild = strings.TrimSpace(out.String()) |
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.
Please use https://github.com/osbuild/images/blob/f52789ed6611ddf9b52ba0f06f7f70d5dd86c939/pkg/osbuild/osbuild-exec.go#L131-L146 instead of implementing it again here.
A very tiny refactoring to improve readability of the function.