Fill in version information #962
Conversation
andyasp
force-pushed
the
aasp/version-information
branch
from
e70a7c6
to
329ba6d
Compare
Makefile
Outdated
| @@ -17,6 +17,9 @@ GOOS ?= $(shell go env GOOS) | |||
| GOARCH ?= $(shell go env GOARCH) | |||
|
|
|||
| HOSTNAME := $(shell hostname) | |||
| BUILD_USER := $(shell id -un)@$(HOSTNAME) | |||
| # Date in ISO 8601 | |||
| BUILD_DATE := $(shell date -u +"%Y-%m-%dT%H:%M:%SZ") | |||
There was a problem hiding this comment.
This will for sure prevent our builds from being reproducible. Not too sure if they are right now.
I suggest to use the commit date of the latest commit hash
There was a problem hiding this comment.
That's a good point. I'm not sure about the current state of reproducible builds either but I'll make the change to the commit date.
There was a problem hiding this comment.
Swapped to git log -1 --format=%cI
There was a problem hiding this comment.
"build date" and "commit date" are two different things, and I would expect to see time being updated on rebuilds.
I just tried to build this locally, and got:
./cmd/mimir/mimir -version
Mimir, version 1.10.0-rc.0 (branch: pull/962, revision: 550a5c367)
build user: [email protected]
build date: 2022-01-31T11:09:21-05:00
go version: go1.17.5
platform: darwin/arm64
Even though it's 2nd of February today. It may be better to skip build date, rather than include wrong information in the binary.
There was a problem hiding this comment.
(I think this may also be super-confusing for users, who check out some old revision, and build it.)
There was a problem hiding this comment.
Furthermore build user is already "random", when building in the container:
Mimir, version 1.10.0-rc.0 (branch: pull/962, revision: 550a5c367)
build user: root@5e8907569246
build date: 2022-01-31T11:09:21-05:00
go version: go1.17.3
platform: darwin/arm64
Maybe the solution is to simply not print build user and build date, and keep them empty.
There was a problem hiding this comment.
I was thinking about this yesterday as well after committing that change and I agree with you. Including any user/hostname would be a hurdle towards reproducible builds as well and "build date" is confusing with a commit date.
BuildUser and BuildDate are now filled. Swapped the Makefile to also set the values directly rather than through main, although this was not a necessary change.
andyasp
force-pushed
the
aasp/version-information
branch
from
550a5c3
to
6da4b47
Compare
| @@ -46,9 +39,6 @@ var configHash *prometheus.GaugeVec = prometheus.NewGaugeVec( | |||
| ) | |||
|
|
|||
| func init() { | |||
| version.Version = Version | |||
| version.Branch = Branch | |||
| version.Revision = Revision | |||
| prometheus.MustRegister(version.NewCollector("cortex")) | |||
There was a problem hiding this comment.
Shall we change cortex to mimir here? It would change cortex_build_info to mimir_build_info, and this needs to be documented in CHANGELOG. I don't see this metric used anywhere in our jsonnet or internal tooling. (Can be separate PR too)
There was a problem hiding this comment.
I think that makes a lot of sense given that this is no longer cortex build information. Probably best to do it in another PR though since I'm unaware of what the plans are for metric renaming.
There was a problem hiding this comment.
In general we plan to keep metrics as-is. However in this specific case, I think it makes sense to rename. But we can discuss that separately.
What this PR does:
I wasn't sure if this was the best way to go about fixing this, but I gave it a try. I added reviewers who I've seen discussing this issue.
Fills in the BuildUser and BuildDate fields when printing
--version.dateisn't consistent between GNU/BSD utils, so a complicated format string was used instead of GNU's-Is(would usinggdatebe better?). There's no particular reason to use ISO-8601.In addition, I swapped the Makefile to also set the values directly rather than through main. That wasn't a necessary change, it just seemed cleaner, but it would negatively impact #951
Which issue(s) this PR fixes:
Fixes #860 when using the Makefile
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]