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

return 200 instead of 204 on stream endpoint #266

Merged
merged 2 commits into from
Dec 13, 2023
Merged

Conversation

bruno-garcia
Copy link
Member

@bruno-garcia bruno-garcia commented Dec 13, 2023

With this change, I can merge getsentry/sentry-dotnet#2961 as-is since it relies on all of Sentry's HTTP logic (including retry-after, etc).

Otherwise I'll need to customize it to deal with 200 or 204, which is fine but since I imagine this might hit other SDKs adding support, changing the status code here might be the simplest.

@bruno-garcia bruno-garcia requested a review from HazAT December 13, 2023 02:12
Copy link

vercel bot commented Dec 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
spotlightjs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 13, 2023 1:10pm

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (77f5cb0) 32.44% compared to head (deb1bd7) 32.44%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #266   +/-   ##
=======================================
  Coverage   32.44%   32.44%           
=======================================
  Files          45       45           
  Lines        2090     2090           
  Branches       71       71           
=======================================
  Hits          678      678           
  Misses       1412     1412           

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

@lforst lforst requested a review from Lms24 December 13, 2023 10:30
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

I think this is a fair change - ultimately we make things easier for SDKs if we pretend to be Relay 😅

Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@HazAT HazAT merged commit 0de3490 into main Dec 13, 2023
2 of 3 checks passed
@HazAT HazAT deleted the feat/stream-returns-200 branch December 13, 2023 13:09
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.

3 participants