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: Added ByteAttachmentContent #1384

Merged
merged 10 commits into from
Dec 15, 2021
Merged

feat: Added ByteAttachmentContent #1384

merged 10 commits into from
Dec 15, 2021

Conversation

bitsandfoxes
Copy link
Contributor

@bitsandfoxes bitsandfoxes commented Dec 13, 2021

Adding a byte array as an attachment and then trying to send events can lead to System.ObjectDisposedException: Cannot access a closed Stream.

So instead of relying on one stream we create a new MemoryStream every time the stream gets requested.

The issue persists when trying to attach a stream.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 13, 2021

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 3f01339

@lucas-zimerman
Copy link
Collaborator

Running build.sh or build.ps1 should update the API changes on your local branch.

@bruno-garcia
Copy link
Member

Running build.sh or build.ps1 should update the API changes on your local branch.

I wonder if it's worth adding a GHA that does the diff and commits to the branch

@SimonCropp
Copy link
Contributor

Running build.sh or build.ps1 should update the API changes on your local branch.

i disagree. the point of the api verification is to get people to view the changes and confirm they are correct. if we are going to automate the accepting of a new api, we may as well not have that step at all

@bruno-garcia
Copy link
Member

@bitsandfoxes Today @SimonCropp shared with us that it's possible the verify files were nto written because the tests didn't run for that specific runtime. Do you have .NET Core 2.1 installed? It should run the tests on that target and create the file that @SimonCropp pushed to your branch: 8756887 (#1384)

@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2021

Codecov Report

Merging #1384 (3f01339) into main (bf99e70) will decrease coverage by 1.46%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1384      +/-   ##
==========================================
- Coverage   82.73%   81.27%   -1.47%     
==========================================
  Files         215      216       +1     
  Lines        7165     7172       +7     
  Branches     1410     1410              
==========================================
- Hits         5928     5829      -99     
- Misses        802      917     +115     
+ Partials      435      426       -9     
Impacted Files Coverage Δ
src/Sentry/ByteAttachmentContent.cs 0.00% <0.00%> (ø)
src/Sentry/ScopeExtensions.cs 62.22% <0.00%> (-7.78%) ⬇️
src/Sentry/PlatformAbstractions/FrameworkInfo.cs 0.00% <0.00%> (-100.00%) ⬇️
...ntry/PlatformAbstractions/RegistryKeyExtensions.cs 0.00% <0.00%> (-100.00%) ⬇️
...Sentry/PlatformAbstractions/FrameworkInfo.NetFx.cs 0.00% <0.00%> (-70.43%) ⬇️
...rmAbstractions/NetFxInstallationsEventProcessor.cs 4.54% <0.00%> (-68.19%) ⬇️
...ntry/PlatformAbstractions/FrameworkInstallation.cs 25.00% <0.00%> (-37.50%) ⬇️
...ntry/Integrations/NetFxInstallationsIntegration.cs 28.57% <0.00%> (-28.58%) ⬇️
src/Sentry/PlatformAbstractions/RuntimeInfo.cs 53.44% <0.00%> (-5.18%) ⬇️
src/Sentry/Internal/AppDomainAdapter.cs 66.66% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf99e70...3f01339. Read the comment docs.

@bruno-garcia bruno-garcia enabled auto-merge (squash) December 15, 2021 01:26
@bruno-garcia bruno-garcia merged commit 7513dd3 into main Dec 15, 2021
@bruno-garcia bruno-garcia deleted the fix/bytearray-attachment branch December 15, 2021 01:29
@bitsandfoxes
Copy link
Contributor Author

@bitsandfoxes Today @SimonCropp shared with us that it's possible the verify files were nto written because the tests didn't run for that specific runtime. Do you have .NET Core 2.1 installed? It should run the tests on that target and create the file that @SimonCropp pushed to your branch: 8756887 (#1384)

I did not, but I do now. Thank for fixing my PR @bruno-garcia @SimonCropp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants