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: Attachments support #670

Merged
merged 2 commits into from
Jul 25, 2023
Merged

feat: Attachments support #670

merged 2 commits into from
Jul 25, 2023

Conversation

edigaryev
Copy link
Contributor

@edigaryev edigaryev commented Jul 20, 2023

This is similar to how sentry-rust supports attachments.

Resolves #574.

@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Patch coverage: 72.09% and project coverage change: +0.02% 🎉

Comparison is base (4b3a135) 80.59% compared to head (52caef2) 80.62%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #670      +/-   ##
==========================================
+ Coverage   80.59%   80.62%   +0.02%     
==========================================
  Files          43       44       +1     
  Lines        4407     4459      +52     
==========================================
+ Hits         3552     3595      +43     
- Misses        726      732       +6     
- Partials      129      132       +3     
Files Changed Coverage Δ
interfaces.go 93.30% <ø> (ø)
transport.go 76.64% <55.55%> (-1.56%) ⬇️
scope.go 91.66% <100.00%> (+0.53%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tonyo
Copy link
Contributor

tonyo commented Jul 21, 2023

Nice, thank you @edigaryev!
We'll review in the next few days.

Copy link
Contributor

@tonyo tonyo left a comment

Choose a reason for hiding this comment

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

Looking good, just one ask about test coverage 👍

@@ -94,6 +94,38 @@ func getRequestBodyFromEvent(event *Event) []byte {
return nil
}

func encodeAttachment(enc *json.Encoder, b io.Writer, attachment *Attachment) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test in transport_test.go that tests this function.

Copy link
Contributor Author

@edigaryev edigaryev Jul 25, 2023

Choose a reason for hiding this comment

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

PTAL at 52caef2.

Not sure if testing encodeAttachment() directly makes sense in the presence of tests like TestEnvelopeFromErrorBody, which will already call that function under the hood if we set event.attachments to something.

Copy link
Contributor

@tonyo tonyo Jul 25, 2023

Choose a reason for hiding this comment

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

What you added is actually what I had in mind.
Fair point, but if we treat attachments as a separate "feature" of the SDK, having a separate test for that sounds like a good thing to me.

@edigaryev edigaryev requested a review from tonyo July 25, 2023 11:08
@tonyo tonyo changed the title Attachments support feat: Attachments support Jul 25, 2023
@tonyo tonyo merged commit 4f72145 into getsentry:master Jul 25, 2023
@tonyo
Copy link
Contributor

tonyo commented Jul 25, 2023

Thanks @edigaryev 🎉

@edigaryev edigaryev deleted the attachments branch July 25, 2023 13:50
@tonyo
Copy link
Contributor

tonyo commented Aug 1, 2023

The attachments support is now released as part of https://github.com/getsentry/sentry-go/releases/tag/v0.23.0 👍

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.

Support for attachments in go sdk
2 participants