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

feat: Read release info from build info #704

Merged
merged 3 commits into from Sep 1, 2023

Conversation

DAcodedBEAT
Copy link
Contributor

Builds from #652

For information about this change, please read the referenced PR above. This PR is building off of the previous one and adds tests for coverage.

@cleptric cleptric enabled auto-merge (squash) August 23, 2023 18:08
util_test.go Outdated
Comment on lines 34 to 36
cleanup := envSetter(map[string]string{
"SENTRY_RELEASE": releaseVersion,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I liked the ability to set multiple environment variables in a single call but it's not strictly necessary. If you think #704 (comment) is a non-issue, I can shift to using T.Setenv instead :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a non-issue for me. One thing that would probably be an issue is flakiness on CI as doing set/un-set environment variable within a test scope won't enable the test suite to run in parallel. See the notes on https://pkg.go.dev/testing#T.Setenv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the end both implementations are similar enough, I just moved this to T.Setenv

util_test.go Outdated Show resolved Hide resolved
mattrobenolt and others added 3 commits August 30, 2023 14:40
Since go1.18, vcs.revision gets compiled into the binary and can be
read.

Conveniently, debug.ReadBuildInfo has been around since go1.12, so
there's no need to conditionally compile this unless sentry-go needs to
support < go1.12 for some reason.

So in builds newer than 1.18, this key just simply won't exist and
effectively will do nothing.

This is much more convenient to detect in anything go1.18 since it's
much more common to have this compiled in.
auto-merge was automatically disabled August 30, 2023 18:43

Head branch was pushed to by a user without write access

@DAcodedBEAT
Copy link
Contributor Author

@cleptric / @aldy505 Can I get a re-review?

@cleptric cleptric self-requested a review August 31, 2023 19:46
@tonyo tonyo self-assigned this Sep 1, 2023
@tonyo tonyo changed the title Read release info from build info (now with tests!) feat: Read release info from build info Sep 1, 2023
@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Patch coverage is 58.33% of modified lines.

Files Changed Coverage
util.go 58.33%

📢 Thoughts on this report? Let us know!.

@cleptric cleptric merged commit 54d884a into getsentry:master Sep 1, 2023
16 of 17 checks passed
@DAcodedBEAT DAcodedBEAT deleted the releasebuildinfo branch September 1, 2023 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants