-
Notifications
You must be signed in to change notification settings - Fork 69
Properly handle non-S3 storage backends #124
Changes from all commits
855b12a
96d5b7e
7b55b97
b2c4803
9998365
9244b6b
9dddbcb
7dd3ca5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,3 +6,4 @@ | |
*.egg-info | ||
/static/ | ||
/static_root/ | ||
.idea |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,13 +2,20 @@ | |
from multiprocessing.dummy import Pool | ||
import warnings | ||
|
||
from django.conf import settings as django_settings | ||
from django.contrib.staticfiles.management.commands import collectstatic | ||
from django.utils.encoding import smart_str | ||
|
||
from collectfast.etag import should_copy_file | ||
from collectfast.boto import reset_connection | ||
from collectfast import settings | ||
|
||
# Backends with which Collectfast should work properly. | ||
VALID_BACKENDS = [ | ||
'storages.backends.s3boto3.S3Boto3Storage', | ||
'storages.backends.s3boto.S3BotoStorage', | ||
] | ||
|
||
|
||
class Command(collectstatic.Command): | ||
def add_arguments(self, parser): | ||
|
@@ -32,13 +39,20 @@ def __init__(self, *args, **kwargs): | |
self.tasks = [] | ||
self.etags = {} | ||
self.collectfast_enabled = settings.enabled | ||
if self.storage.preload_metadata is not True: | ||
self.storage.preload_metadata = True | ||
warnings.warn( | ||
"Collectfast does not work properly without " | ||
"`preload_metadata` set to `True` on the storage class. Try " | ||
"setting `AWS_PRELOAD_METADATA` to `True`. Overriding " | ||
"`storage.preload_metadata` and continuing.") | ||
if settings.enabled: | ||
if django_settings.STATICFILES_STORAGE not in VALID_BACKENDS: | ||
raise RuntimeError( | ||
"Collectfast is intended to work with an S3 storage " | ||
"backend only." | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is the desired behaviour. In case a storage backend is configured that isn't S3 we want to handle it gracefully. I've had a plan (in my head) for some time to introduce an abstraction layer for what strategy to use for different backends, but I think that's a much larger scope than what's needed here. I believe all we want to do is skip the md5/etag comparison if the backend isn't s3. If it's feasible I think it'd be cool if we could still have threading enabled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your comment @antonagestam. Yes, I understand the desire to handle non-S3 backends gracefully. Just to verify I understood you correctly, you're saying that when if settings.enabled and self.storage.preload_metadata is not True:
... instead of checking the storage type. So, now I'm confused :-) If we want to allow Could you please clarify the right approach, the way you see it? I'd love to solve this this way or the other, without increasing the scope of this bugfix too much. Please let me know your thoughts. Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @antonagestam - are you suggesting something like: self.collectfast_enabled = settings.enabled
if not hasattr(self.storage, 'preload_metadata'): # or S3 checks
self.collectfast_enabled = False
warnings.warn('Your current storage does not have preload_metadata!'
'Disabling collectfast and continuing...'
)
if self.collectfast_enabled and self.storage.preload_metadata is not True:
self.storage.preload_metadata = True
warnings.warn(
# ...
) |
||
if self.storage.preload_metadata is not True: | ||
self.storage.preload_metadata = True | ||
warnings.warn( | ||
"Collectfast does not work properly without " | ||
"`preload_metadata` set to `True` on the storage " | ||
"class. Try setting `AWS_PRELOAD_METADATA` to `True`. " | ||
"Overriding `storage.preload_metadata` and continuing." | ||
) | ||
|
||
def set_options(self, **options): | ||
""" | ||
|
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.
one problem with this is that it would cause a RuntimeError if you're using a subclass of S3BotoStorage/S3Boto3Storage. I see two other options:
(1) import the classes and use
if isinstance(self.storage, S3BotoStorage) or isinstance(self.storage, S3Boto3Storage)
(which could be refactored into a helper:if is_s3_backend(self.storage)
)(2) duck typing:
if not hasattr(self.storage, 'preload_metadata'): raise
--> but I'm wondering if we'd just want to setpreload_metadata
defensively in that case: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.
Yes, that's a good point @pachewise.
Actually I wanted to implement it as in your first suggestion but encountered problems with mocked objects in some existing tests.
In any case, I think both of your suggestions could work, however looks like we have bigger questions now following @antonagestam's comment below. Let's clear the desired behavior first, and then I'll solve the problems in the implementation.