-
Notifications
You must be signed in to change notification settings - Fork 194
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 S3Boto3Storage backend and test cases #415
Conversation
try: | ||
storage = self.get_storage() | ||
storage.delete(file_name) | ||
except Exception as e: | ||
raise ServiceUnavailable("File was not deleted") from e |
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.
Somehow I have a few question around this design.
What do you think of the other design:
if not storage.exists(file_name):
raise ServiceUnavailable("File does not exist")
...since, ServiceUnavailable
is already a well-defined exception.
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.
good catch! i'll change that!
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.
have another look @50-Course if you like!
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.
Going through, this looks much better with the tests now 🚀
Hello, everyone! 😊 Do you have estimate of when this fix will be available, please? |
Hello, guys! I think this PR is very important because this error invalidate the purpose of package. Please, merge this PR if is done. Thanks! |
I can only second that! |
Looks like there is a linting error, if you can correct that I'll get this merged. Thanks! |
I've attempted to resolve the isort error and believe it should be fixed now. However, I'm currently unable to initiate GitHub Actions to confirm if the continuous integration (CI) checks are successful. |
@frankwiles i think i have it now. actions are now successul on my fork. |
@frankwiles could you please have a look? thanks alot! |
Hello guys, some news for this PR? @frankwiles @krystofbe |
@victoraugusto6 my work is done, but this needs to be merged by a member of revsys. |
Just reaching out because I'm a bit bummed about the delay in getting my pull request merged. It's a critical fix for the S3Boto3StorageHealthCheck, and I know it's important for folks using the I've put a lot of work into this, and it feels like we're leaving users in the lurch by not rolling it out. I'm considering forking the project and pushing the fix to PyPI if we can't get it merged here soon. Really don't want to go down that road, but I want to make sure users have what they need. Can we get some movement on this? |
Sorry for the delay in getting to this and thanks for fixing it! |
Fix for
S3Boto3StorageHealthCheck
Failure in Version 3.18.0Problem Description
After upgrading from
django-health-check
version 3.17.0 to 3.18.0, several users have reported failures with theS3Boto3StorageHealthCheck
. This issue appears to stem fromself.storage_alias
beingNone
when it is used within theS3Boto3StorageHealthCheck
, leading to anAttributeError
when the code attempts to call thesave
method on aNoneType
object. This behavior was not observed in version 3.17.0 and has been consistently reproducible in version 3.18.0.Additional Notes
This fix aims to restore the expected functionality of the S3Boto3StorageHealthCheck without altering the behavior of other storage health checks. Feedback and further testing are welcome to ensure comprehensive coverage and resolution of the issue.