Skip to content
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

Handle build info more accurately #588

Merged
merged 1 commit into from
Jun 29, 2022
Merged

Conversation

06kellyjac
Copy link
Contributor

runtime.Version() will always give an accurate build host version

https://cs.opensource.google/go/go/+/refs/tags/go1.18.3:src/runtime/extern.go;l=251-266

For the host's GOOS and host's GOARCH they're not available so they must still be passed in via ldflags

matches the go version cli output

https://github.com/golang/go/blob/master/src/cmd/go/internal/version/version.go#L76

make fastly now uses goreleaser. I removed the cleanup comment and left the ldflags for the "dev" builds
Maybe the whole LDFLAGS entry can be removed and the dev builds just dont have version info?

Copy link
Collaborator

@Integralist Integralist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @06kellyjac for opening this PR, this is very welcomed.

I have a few comments if you could address them, please.

Makefile Show resolved Hide resolved
pkg/revision/revision.go Outdated Show resolved Hide resolved
@06kellyjac 06kellyjac force-pushed the buildinfo branch 2 times, most recently from 8bf5dfc to e4ba9ce Compare June 29, 2022 13:34
Makefile Show resolved Hide resolved
`runtime.Version()` will always give an accurate build host version

For the host's GOOS and host's GOARCH they're not available so they must
still be passed in via ldflags

The Makefile has been modified to use goreleaser for the fastly item
Copy link
Collaborator

@Integralist Integralist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again, this is a great clean-up effort.

@Integralist Integralist merged commit 8ef2fd9 into fastly:main Jun 29, 2022
@06kellyjac
Copy link
Contributor Author

No problem.

It'd be nice if runtime or buildinfo also exposed the host os and arch and we wouldn't need any of these env vars (the info is there but only within an internal library :/) but this will do for now

@06kellyjac 06kellyjac deleted the buildinfo branch June 29, 2022 15:35
@06kellyjac
Copy link
Contributor Author

06kellyjac commented Jun 29, 2022

I just realized I didn't touch the Dockerfiles but it looks like they grab the release so that doesn't need touching

I do need to fix the snapshot tests though by the looks of it

@Integralist Integralist added the enhancement New feature or request label Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants