Read Go build info from the binary

[lucacome]

Uses build info from runtime/debug, so it doesn't need to be injected at build time.

This is possible since go 1.18

[pleshakov]

(sorry, missed this in the first review)

is there are a chance any of those settings are not present in info.Settings ? in that case, are we're ok with reporting empty strings? perhaps "unknown" would work better?

do we want a unit test to test this logic?

wonder what @kate-osborn thinks

[lucacome]

ah, good catch! I've updated it

[kate-osborn]

I think we should add unit tests wherever possible.

[lucacome]

As I understand it that information is not available when running the tests. We could manually build the binary and then check that the info is present, but maybe it doesn't belong in a unit test?

[pleshakov]

one approach is like below:

type buildInfoFunc func() (info *debug.BuildInfo, ok bool)

var buildInfo buildInfoFunc = debug.ReadBuildInfo

func getBuildInfo() (commitHash string, commitTime string, dirtyBuild string) {
	commitHash = "unknown"
	commitTime = "unknown"
	dirtyBuild = "unknown"

	info, ok := buildInfo()
	if !ok {
		return
	}
	for _, kv := range info.Settings {
		switch kv.Key {
		case "vcs.revision":
			commitHash = kv.Value
		case "vcs.time":
			commitTime = kv.Value
		case "vcs.modified":
			dirtyBuild = kv.Value
		}
	}

	return
}

this way it will be possible to write and use a stub of buildInfo to use in the unit test

[lucacome]

I was thinking about this, and if we can't test the actual values, there's no logic (that we implemented) to test in this function as there's just a loop reading values from strings. Right?

[pleshakov]

I see a few cases here:

• normal case: buildInfo() returns OK with values populated in info.Settings
• edge case: buildInfo() returns not OK
• edge case: buildInfo() returns OK without values got populated info.Settings

in all those cases, we can ensure that the getBuildInfo() returns the expected values. There is some amount of logic, since we have if and a loop.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues found.

Scanned Manifest Files

[sjberman]

Is this still needed? @lucacome

[lucacome]

yes @sjberman it would be nice to remove some extra variables. The only thing missing are the tests.

FWIW we've been using the same in multiple other projects and it works 👀

[sjberman]

@lucacome Ok cool, if it can be rebased and feedback addressed (on your time), I say we get this in since it's been sitting here for awhile.

[github-actions]

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[github-actions]

This PR was closed because it has been stalled for 14 days with no activity.