main: add osbuild version to version command #332
Conversation
lzap
requested review from
mvo5,
achilleas-k and
thozza
and removed request for
a team
cmd/image-builder/version.go
Outdated
| } | ||
|
|
||
| func readVersionFromBinary() *versionDescription { | ||
| var osbuildCmd = "osbuild" |
There was a problem hiding this comment.
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.
Oh I was planning to make it a constant. But I am going to delete this as @thozza had a better idea.
lzap
force-pushed
the
version-osbuild1
branch
2 times, most recently
from
9f9870a to
36362cb
Compare
|
Updated, the output is a bit inconsistent now: Images having the |
cmd/image-builder/version.go
Outdated
| commit := "unknown" | ||
| images := "unknown" | ||
| vd.ImageBuilder.Commit = "unknown" | ||
| vd.ImageBuilder.Version = "unknown" |
There was a problem hiding this comment.
It seems like you're never actually setting this value to the global version later on anymore (previously on line 48); this seems to mean that the version will always be unknown and can't be set by command-line argument anymore (as done by rpm/makefile).
cmd/image-builder/version.go
Outdated
| vd.ImageBuilder.Dependencies.Images = images | ||
| osbuildVersion, err := osbuild.OSBuildVersion() | ||
| if err != nil { | ||
| vd.ImageBuilder.Dependencies.OSBuild = fmt.Sprintf("unknown (%v)", err) |
There was a problem hiding this comment.
Why do we want the error in here? It's never useful to the user (though useful to the developer, I guess) but will make ingesting this programmatically slightly harder.
There was a problem hiding this comment.
Parsing is a good point, I guess I will just print a warning to stderr.
cmd/image-builder/version.go
Outdated
| if err != nil { | ||
| vd.ImageBuilder.Dependencies.OSBuild = fmt.Sprintf("unknown (%v)", err) | ||
| } | ||
| vd.ImageBuilder.Dependencies.OSBuild = fmt.Sprintf("v%s", osbuildVersion) |
There was a problem hiding this comment.
I mean I guess we can prefix with v but that's not what the actual osbuild version is: https://github.com/osbuild/osbuild/releases don't really care enough to block on it but it seems a bit unnecessary.
There was a problem hiding this comment.
IMHO, we should report the version as reported by the tool
This commit refactors the readVersionInfo function to improve code readability and maintainability.
|
Thanks for the review, updated all three places: |
There was a problem hiding this comment.
Tests are failing and since prettyVersion is always called in main the unit tests are always spamming the output for osbuild and it messes with the mock call args for the fakeOsbuildCmd since it's now the second call, not the first. You should update the mock command to include support for --version, but:
This is actually a bit of a thing, it would be really nice if we could not call osbuild on start up every time; it adds a bunch of overhead. Can we get around that?
I'd also appreciate if the test_container_version_smoke test was updated with the new key.
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: v158
|
Thanks again for the analysis, unfortunately, cobra does not support lazy initialization and out of the several workarounds this one seems the nicest to me. The main command shows help as the default action (as it does nothing), so I deleted the version string (which was causing the problem) and check for it in the Added the test case, I missed this one when I was searching for the test. |
A very tiny refactoring to improve readability of the function.