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 S3Boto3Storage backend and test cases #415

Merged
merged 7 commits into from
May 3, 2024

Conversation

krystofbe
Copy link
Contributor

Fix for S3Boto3StorageHealthCheck Failure in Version 3.18.0

Problem Description

After upgrading from django-health-check version 3.17.0 to 3.18.0, several users have reported failures with the S3Boto3StorageHealthCheck. This issue appears to stem from self.storage_alias being None when it is used within the S3Boto3StorageHealthCheck, leading to an AttributeError when the code attempts to call the save method on a NoneType 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.

Comment on lines 25 to 29
try:
storage = self.get_storage()
storage.delete(file_name)
except Exception as e:
raise ServiceUnavailable("File was not deleted") from e

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.

Copy link
Contributor Author

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!

Copy link
Contributor Author

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!

Copy link

@50-Course 50-Course left a 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 🚀

health_check/contrib/s3boto3_storage/backends.py Outdated Show resolved Hide resolved
@victoraugusto6
Copy link

Hello, everyone! 😊 Do you have estimate of when this fix will be available, please?

@victoraugusto6
Copy link

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!

@krystofbe
Copy link
Contributor Author

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!
@frankwiles

@frankwiles
Copy link
Member

Looks like there is a linting error, if you can correct that I'll get this merged. Thanks!

@krystofbe
Copy link
Contributor Author

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.

@krystofbe
Copy link
Contributor Author

@frankwiles i think i have it now. actions are now successul on my fork.

@krystofbe
Copy link
Contributor Author

@frankwiles could you please have a look? thanks alot!

@victoraugusto6
Copy link

victoraugusto6 commented Mar 21, 2024

Hello guys, some news for this PR? @frankwiles @krystofbe

@krystofbe
Copy link
Contributor Author

@victoraugusto6 my work is done, but this needs to be merged by a member of revsys.

@krystofbe
Copy link
Contributor Author

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 django-health-check package.

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?

@frankwiles frankwiles merged commit 251b156 into revsys:master May 3, 2024
@frankwiles
Copy link
Member

Sorry for the delay in getting to this and thanks for fixing it!

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.

4 participants