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

Fix Blobstore failure response #101

Merged
merged 3 commits into from
Dec 9, 2024
Merged

Conversation

MrCreosote
Copy link
Member

Fixed a bug where if the Blobstore returned an non-json response, logging the response would fail due to attempting to concatenate string and binary types. This would shadow the real error returned from the Blobstore.

Also changed a few Shock references to Blobstore. Baby steps.

Fixed a bug where if the Blobstore returned an non-json response,
logging the response would fail due to attempting to concatenate string
and binary types. This would shadow the real error returned from the
Blobstore.

Also changed a few Shock references to Blobstore. Baby steps.
Comment on lines -311 to +312
# this means shock is down or not responding.
log("Couldn't parse response error content from Shock: " +
response.content)
# this means the Blobstore is down or not responding.
log("Couldn't parse response error content from the Blobstore: " + response.text)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the bugfix

Standard SDK docker image too old issue
Copy link

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.

Project coverage is 71.53%. Comparing base (0d85556) to head (7368ba2).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
lib/DataFileUtil/DataFileUtilImpl.py 88.23% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #101   +/-   ##
=======================================
  Coverage   71.53%   71.53%           
=======================================
  Files           5        5           
  Lines        1212     1212           
=======================================
  Hits          867      867           
  Misses        345      345           

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


jobs:

sdk_tests:
runs-on: ubuntu-latest
runs-on: ubuntu-24.04
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that GitHub Actions is transitioning ubuntu-latest from 22.04 to 24.04. However, to me the current ubuntu-latest is 24.04, does it make sense to continue using ubuntu-latest, or could there be potential issues to consider?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied the test.yml file from another sdk repo that was already updated. I don't have strong feelings about using 22 or 24, but I do think we should pin the version. That being said, I really doubt that pinning 22 vs 24 will make a difference.

If you want to use 22 I'm fine with that, or we can keep it as is.

Copy link
Member Author

@MrCreosote MrCreosote Dec 9, 2024

Choose a reason for hiding this comment

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

Oh, you already approved. Ok, I'll merge and I can change it to 22 if you think that's important

@MrCreosote MrCreosote merged commit 8645c46 into master Dec 9, 2024
3 checks passed
@MrCreosote MrCreosote deleted the dev-shock_response_text branch December 9, 2024 21:25
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.

2 participants