theriverman / django-minio-backend

Minio Backend for Django
https://pypi.org/project/django-minio-backend/
MIT License
111 stars 22 forks source link

Getting ConnectionError in tox environment from call to check_bucket_existence when using static files storage #36

Open alexolivas opened 2 years ago

alexolivas commented 2 years ago

Note: I'm only running into this issue in my CI/CD pipeline, because I am using tox to create an isolated python 3.8 environment to run all my django/python unit tests.

In my settings.py I set STATIC_FILES_STORAGE to use MinioBackendStatic to serve all my static files from my minio server, because of this I get a ConnectionError when the tox environment starts up, this library makes a call to check the buckets exist at django_minio_backend/apps.py#L32.

        # Validate static storage and default storage configurations
        staticfiles_storage: str = get_setting('STATICFILES_STORAGE')
        if staticfiles_storage.endswith(MinioBackendStatic.__name__):
            mbs = MinioBackendStatic()
            mbs.check_bucket_existence()

Screenshot of stacktrace on startup

Screen Shot 2022-09-20 at 10 24 50 AM

Because this is an isolated python environment specifically used for testing, it can't really reach out to ensure any buckets exist. By looking at the source code, it doesn't seem like there's a way to prevent the call to mbs.check_bucket_existence() unless I don't set STATICFILES_STORAGE = "django_minio_backend.models.MinioBackendStatic" in my settings.py file whenever I'm running this tox environment, which is not an elegant solution IMO.

I'm basically looking for any guidance on how to get around this a bit more elegantly. I'm also open to opening up a pull request to introduce an additional (or reuse MINIO_CONSISTENCY_CHECK_ON_START) configuration setting in that line of code to not check for bucket existence e.g.

There are 2 places where this needs to be done, in apps.py:

        consistency_check_on_start = get_setting('MINIO_CONSISTENCY_CHECK_ON_START', False)

        # other code goes here...

        # Validate static storage and default storage configurations
        staticfiles_storage: str = get_setting('STATICFILES_STORAGE')
        if staticfiles_storage.endswith(MinioBackendStatic.__name__):
            mbs = MinioBackendStatic()
            # This is my proposed change - this entire line could also be removed entirely
            # since the init method of MinioBackendStatic already checks for bucket existence
            if consistency_check_on_start:
                    mbs.check_bucket_existence()

and in models.py

@deconstructible
class MinioBackendStatic(MinioBackend):
    """
    MinIO-compatible Django custom storage system for Django static files.
    The used bucket can be configured in settings.py through `MINIO_STATIC_FILES_BUCKET`
    :arg *args: Should not be used for static files. It's here for compatibility only
    :arg **kwargs: Should not be used for static files. It's here for compatibility only
    """
    def __init__(self, *args, **kwargs):
        super().__init__(self.MINIO_STATIC_FILES_BUCKET, *args, **kwargs)

        consistency_check_on_start = get_setting('MINIO_CONSISTENCY_CHECK_ON_START', False)
        if consistency_check_on_start: # This is my proposed change
                self.check_bucket_existence()  # make sure the `MINIO_STATIC_FILES_BUCKET` exists
        self.set_bucket_to_public()  # the static files bucket must be publicly available
theriverman commented 2 years ago

I see your problem and I think your proposal to reuse MINIO_CONSISTENCY_CHECK_ON_START is a good call. I would like to avoid introducing yet another configuration parameter.

You're right about the proposed changes in those two places, but in the case of MinioBackendStatic, the last line calling self.set_bucket_to_public() should be indented into the if branch as well. Otherwise it may raise an Exception while trying to manipulate a non-existing bucket. At least that's what I suspect to happen.

Please feel free to open a PR, and I'll check it out ASAP.

alexolivas commented 1 year ago

FYI I've not forgotten about this, I will open up a PR as soon as I free up a bit from work. I calculate sometime around mid next week.