-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
S3 200 errors implementation #3276
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #3276 +/- ##
===========================================
+ Coverage 93.17% 93.20% +0.02%
===========================================
Files 66 66
Lines 14339 14358 +19
===========================================
+ Hits 13361 13382 +21
+ Misses 978 976 -2 ☔ View full report in Codecov by Sentry. |
f5a7bb6
to
6e31ea8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some high level comments of what may make this a bit simpler. We should be able to drop the empty body requirement after our discussion.
tests/functional/configured_endpoint_urls/test_configured_endpoint_url.py
Outdated
Show resolved
Hide resolved
b08fd43
to
e3f275a
Compare
Co-authored-by: Nate Prewitt <[email protected]>
Co-authored-by: Nate Prewitt <[email protected]>
Co-authored-by: Nate Prewitt <[email protected]>
Co-authored-by: Nate Prewitt <[email protected]>
e3f275a
to
bd5d4ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Have a few minor notes on comments and unit tests. We want to try to be succinct with comments/logging as long messages are often ignored.
Co-authored-by: Nate Prewitt <[email protected]>
Co-authored-by: Nate Prewitt <[email protected]>
Co-authored-by: Nate Prewitt <[email protected]>
Co-authored-by: Nate Prewitt <[email protected]>
Co-authored-by: Nate Prewitt <[email protected]>
Co-authored-by: Nate Prewitt <[email protected]>
Co-authored-by: Nate Prewitt <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! One last thing to fix and then I think we're set.
Co-authored-by: Nate Prewitt <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things look good, assuming tests pass I think this is ready to ship. Great job, @alexgromero!
Closing/reopening to rerun tests against fix in #3282. |
This reverts commit d64e956.
The S3 service may occasionally respond with an HTTP 200 OK status code while including errors in the response body. This PR addresses this handling to ensure accurate error reporting.