include build time and revision in version information #5973

graphaelli · 2018-01-03T01:14:02Z

I couldn't find a clever way to hook up tests for this, here's one way to verify the changes:

$ cd $GOPATH/src/github.com/elastic/beats/auditbeat
$ make -B
./auditbego build -i -ldflags "-X github.com/elastic/beats/libbeat/version.buildTime=2018-01-03T01:07:04 -X github.com/elastic/beats/libbeat/version.commit=65a82579ddc7fa34a82cf554e6ef4adcbf24bdfa"
at version
$ ./auditbeat version
auditbeat version 7.0.0-alpha1 (amd64), libbeat 7.0.0-alpha1 [65a82579ddc7fa34a82cf554e6ef4adcbf24bdfa built 2018-01-03T01:07:04]

I got caught up for awhile getting this to work for apps that vendor libbeat, like apm-server. This worked there, note the github.com/elastic/apm-server/vendor/ prefix:

$ cd $GOPATH/src/github.com/elastic/apm-serve
$ go build -i -ldflags "-X github.com/elastic/apm-server/vendor/github.com/elastic/beats/libbeat/version.buildTime=2018-01-03T01:00:47 -X github.com/elastic/apm-server/vendor/github.com/elastic/beats/libbeat/version.commit=c066b5885efb57decafbee9fbbb532b4e2041529"
$ ./apm-server version
apm-server version 7.0.0-alpha1 (amd64), libbeat 7.0.0-alpha1 [c066b5885efb57decafbee9fbbb532b4e2041529 built 2018-01-03T01:00:47]

houndci-bot · 2018-01-03T01:14:09Z

libbeat/version/version.go

+	return buildTime
+}
+
+func Commit() string {


exported function Commit should have comment or be unexported

houndci-bot · 2018-01-03T01:14:09Z

libbeat/version/version.go

+	commit    = "unknown"
+)
+
+func BuildTime() string {


exported function BuildTime should have comment or be unexported

houndci-bot · 2018-01-03T01:38:36Z

libbeat/version/version.go

+	return buildTime
+}
+
+// BuildTime exposes the compile-time commit information


comment on exported function Commit should be of the form "Commit ..."

andrewkroh · 2018-01-03T15:52:33Z

libbeat/version/version.go

+)
+
+// BuildTime exposes the compile-time build time information
+func BuildTime() string {


How about returning a time.Time instead of string? And if the value is not set appropriately then return a zero value (time.Time{}). This will save callers that need the time in a certain format from having to learn the format and parse it themselves.

I like that but concerned about how to handle parse failure, in case of user error. At the risk of overkill, what about a versionTime type with func String() string and func Parse() (time.Time, error) methods?

I was thinking to only have version.BuildTime() time.Time. If the value was not set or was set incorrectly then return time.Time{}. For me this would be adequate (I could check for validity if my use case required it with time.IsZero()). I'd clearly state in the godocs that the zero value could be returned if the version.buildTime variable was not set or set incorrectly.

But if you want to be more explicit in stating that the build time value is bogus then I'd use version.BuildTime() (time.Time, error).

updated as requested, now:

# valid time $ go build -i -ldflags "-X github.com/elastic/beats/libbeat/version.buildTime=2018-01-03T17:30:56Z -X github.com/elastic/beats/libbeat/version.commit=1234" $ ./libbeat version mockbeat version 9.9.9 (amd64), libbeat 7.0.0-alpha1 [1234 built 2018-01-03 17:30:56 +0000 UTC] # invalid time $ go build -i -ldflags "-X github.com/elastic/beats/libbeat/version.buildTime=XXX -X github.com/elastic/beats/libbeat/version.commit=1234" $ ./libbeat version mockbeat version 9.9.9 (amd64), libbeat 7.0.0-alpha1 [1234 built unknown]

andrewkroh · 2018-01-03T16:05:50Z

libbeat/scripts/Makefile

@@ -36,7 +36,8 @@ COVERAGE_TOOL?=${BEAT_GOPATH}/bin/gotestcover
 COVERAGE_TOOL_REPO?=github.com/elastic/beats/vendor/github.com/pierrre/gotestcover
 TESTIFY_TOOL_REPO?=github.com/elastic/beats/vendor/github.com/stretchr/testify
 LIBCOMPOSE_TOOL_REPO?=github.com/docker/libcompose
-GOBUILD_FLAGS?=-i
+NOW=$(shell date -u '+%Y-%m-%dT%H:%M:%S')


This looks to be very close to RFC3339. Can you add the Zulu timezone identifier (e.g. date -u +"%Y-%m-%dT%H:%M:%SZ"). This will make the value parse nicely like time.Parse(time.RFC3339, "2018-01-03T16:03:28Z").

houndci-bot · 2018-01-03T17:42:40Z

libbeat/version/version.go

+func BuildTime() time.Time {
+	if t, err := time.Parse(time.RFC3339, buildTime); err != nil {
+		return time.Time{}
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

houndci-bot · 2018-01-03T17:42:40Z

libbeat/version/version.go

+func BuildTime() time.Time {
+	if t, err := time.Parse(time.RFC3339, buildTime); err != nil {
+		return time.Time{}
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

andrewkroh

Overall LGTM in its current state. Left a few nit comments. Thanks for making this change upstream.

andrewkroh · 2018-01-03T21:20:09Z

libbeat/version/version.go

+	commit    = "unknown"
+)
+
+// BuildTime exposes the compile-time build time information


You should document the conditions under which a zero value can be returned.

andrewkroh · 2018-01-03T21:21:21Z

libbeat/version/version.go

+	return t
+}
+
+// Commit exposes the compile-time commit information


I think this should mention "hash" or "commit ID".

andrewkroh · 2018-01-03T21:37:27Z

libbeat/cmd/version.go

@@ -22,8 +22,15 @@ func genVersionCmd(name, beatVersion string) *cobra.Command {
 					return fmt.Errorf("error initializing beat: %s", err)
 				}

-				fmt.Printf("%s version %s (%s), libbeat %s\n",
-					beat.Info.Beat, beat.Info.Version, runtime.GOARCH, version.GetDefaultVersion())
+				var buildTime string


Consider initializing buildTime := "unknown". I think it could make this a bit more concise.

andrewkroh · 2018-01-03T21:46:52Z

Regarding testing, we should probably add a check to beats-tester to verify that the .build_hash.txt file matches the beat version output.

andrewkroh

LGTM, but I think it needs a rebase to pull in the fix for the NOTICE.txt year.

This reverts commit a3970ee. Revert "Include build time and revision in version information (elastic#5973)" This reverts commit 54d6b4c.

This reverts commit a3970ee. Revert "Include build time and revision in version information (#5973)" This reverts commit 54d6b4c. Temporarily remove version info that is not provided by the current release process. before: ``` $ ./libbeat version mockbeat version 9.9.9 (amd64), libbeat 6.2.0 [unknown built unknown] ``` after: ``` $ ./libbeat version mockbeat version 9.9.9 (amd64), libbeat 6.2.0 ``` for #6051

houndci-bot reviewed Jan 3, 2018

View reviewed changes

graphaelli mentioned this pull request Jan 3, 2018

include build time and revision in version information elastic/apm-server#396

Merged

houndci-bot reviewed Jan 3, 2018

View reviewed changes

graphaelli force-pushed the bake-version branch from 13c484e to 36212e1 Compare January 3, 2018 01:40

ruflin added libbeat review labels Jan 3, 2018

ruflin requested a review from tsg January 3, 2018 11:20

andrewkroh reviewed Jan 3, 2018

View reviewed changes

houndci-bot reviewed Jan 3, 2018

View reviewed changes

graphaelli force-pushed the bake-version branch from a3a148f to 3acba2a Compare January 3, 2018 17:47

andrewkroh reviewed Jan 3, 2018

View reviewed changes

andrewkroh approved these changes Jan 3, 2018

View reviewed changes

graphaelli force-pushed the bake-version branch from 2d22382 to a1613b3 Compare January 3, 2018 22:21

include build time and revision in version information

ff7fa5a

graphaelli force-pushed the bake-version branch from a1613b3 to ff7fa5a Compare January 4, 2018 03:21

andrewkroh merged commit 54d6b4c into elastic:master Jan 4, 2018

graphaelli mentioned this pull request Jan 11, 2018

version info not in releases #6051

Closed

This was referenced Feb 9, 2018

Expose libbeat version in generated beats #6345

Merged

set compile time version options in package builds #6360

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

include build time and revision in version information #5973

include build time and revision in version information #5973

graphaelli commented Jan 3, 2018

houndci-bot Jan 3, 2018

houndci-bot Jan 3, 2018

houndci-bot Jan 3, 2018

andrewkroh Jan 3, 2018

graphaelli Jan 3, 2018

andrewkroh Jan 3, 2018 •

edited

Loading

graphaelli Jan 3, 2018

andrewkroh Jan 3, 2018

houndci-bot Jan 3, 2018

houndci-bot Jan 3, 2018

andrewkroh left a comment

andrewkroh Jan 3, 2018

andrewkroh Jan 3, 2018

andrewkroh Jan 3, 2018

andrewkroh commented Jan 3, 2018

andrewkroh left a comment

include build time and revision in version information #5973

include build time and revision in version information #5973

Conversation

graphaelli commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh commented Jan 3, 2018

andrewkroh left a comment

Choose a reason for hiding this comment

andrewkroh Jan 3, 2018 •

edited

Loading