Calculate settings when storage is instantiated; not imported #524

jdufresne · 2018-07-06T04:52:16Z

Makes the storage classes usable with Django's override_settings. For example, projects can now change the S3 location & bucket when running their tests. Affects the following backends:

azure_storage
gcloud
gs
s3boto
s3boto3

The remaining storage backends do not need to be altered as they do no calculate settings at import time.

The class BaseStorage was created to use a common pattern to initialize settings for various backends. Users of these backends can expect them to work consistently. This class is fully backwards compatible. Settings can now be set:

In settings.py
Using override_settings
As a class variable
As an argument to init

Closes #498

Completes all necessary backends and adds tests.

codecov-io · 2018-07-06T04:55:08Z

Codecov Report

❗ No coverage uploaded for pull request base (master@87c42d1). Click here to learn what that means.
The diff coverage is 81.08%.

@@            Coverage Diff            @@
##             master     #524   +/-   ##
=========================================
  Coverage          ?   75.72%           
=========================================
  Files             ?       12           
  Lines             ?     1524           
  Branches          ?        0           
=========================================
  Hits              ?     1154           
  Misses            ?      370           
  Partials          ?        0

Impacted Files	Coverage Δ
storages/backends/azure_storage.py	`0% <0%> (ø)`
storages/backends/gcloud.py	`94.61% <100%> (ø)`
storages/backends/gs.py	`61.01% <100%> (ø)`
storages/backends/s3boto.py	`87.54% <100%> (ø)`
storages/backends/s3boto3.py	`86.68% <100%> (ø)`
storages/base.py	`91.66% <91.66%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87c42d1...78d40a0. Read the comment docs.

sww314 · 2018-07-06T20:11:21Z

@jdufresne This looks good, but it will not merge cleanly after I merged on of your other changesets. Can you update?

jdufresne · 2018-07-09T02:02:28Z

Thanks @sww314 I have rebased on to the latest master.

jdufresne · 2018-07-11T21:53:31Z

Rebased to resolve merge conflicts.

sww314

These changes look good to me, but I would like another person to review as well.

sww314 · 2018-07-12T01:48:35Z

@jschneier These changes look good to me. Can you review as well?

jschneier · 2018-08-11T22:55:10Z

Looking now. This will close what I consider by far the biggest issue in the original design of the library.

jdufresne · 2018-08-12T13:05:49Z

I have rebased to resolve all merge conflicts. Ready for additional review. Thanks!

sww314 · 2018-09-11T16:17:57Z

@jschneier @jdufresne Now that 1.7 is out. Maybe we can get this merged for version 2?

jdufresne · 2018-09-17T03:15:41Z

I've rebased this to the latest master. Feedback welcome.

yozlet · 2018-10-24T16:55:11Z

@jschneier Bumping this PR - it would be very useful for those of us having to initialise credentials from other sources (e.g. reading Google Cloud creds from an env var rather than a file).

sushifan · 2018-11-15T17:50:38Z

@jschneier, any indication as to when this might make it into an official release? Would be extremely useful as we often need to switch between multiple sets of storage settings on the fly. Thanks!

jschneier · 2018-11-15T17:52:06Z

Hi, Thanks for the interest. I’ve been playing work catch up after coming back from vacation this week. I’m going to dedicate one day to django-storages this weekend and this is one of my top priorities.

…

On Nov 15, 2018, at 12:50 PM, sushifan ***@***.***> wrote: @jschneier <https://github.com/jschneier>, any indication as to when this might make it into an official release? Would be extremely useful as we often need to switch between multiple sets of storage settings on the fly. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#524 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACJB2IJJp4IdwaAVF1JjjAHO1mF6B79Pks5uvalwgaJpZM4VE4Sa>.

yozlet · 2019-05-20T17:57:25Z

@sww314 Are you able to merge this? (Note that Travis last tested this PR several months ago, so it would be good to check they still pass.)

jdufresne · 2019-05-21T13:41:45Z

Note that Travis last tested this PR several months ago, so it would be good to check they still pass.

Rebased. Travis is still green.

jdufresne · 2020-03-05T12:00:01Z

Hi, is there still interest in this PR? I'll happily rebase if it will be reviewed for inclusion. Just let me know.

sushifan · 2020-03-05T16:37:44Z

Hi, is there still interest in this PR? I'll happily rebase if it will be reviewed for inclusion. Just let me know.

I've been using the following workaround this entire time, but the original idea of this PR still seems like a great, essential feature... My workaround is defining multiple storage backends and using them as appropriate:

from storages.backends.s3boto3 import S3Boto3Storage


class PrivateS3OverwriteStorage(S3Boto3Storage):
    """Only specify params that are different from those in Django settings."""
    def __init__(self):
        super().__init__(file_overwrite=True)


class PublicS3OverwriteStorage(S3Boto3Storage):
    """Only specify params that are different from those in Django settings."""
    def __init__(self):
        super().__init__(default_acl='public-read', file_overwrite=True, querystring_auth=False)

jschneier · 2020-03-05T18:27:33Z

Yes, still interested. This greatly improves testability.

…

On Thursday, March 5, 2020, sushifan ***@***.***> wrote: Hi, is there still interest in this PR? I'll happily rebase if it will be reviewed for inclusion. Just let me know. I've been using the following workaround this entire time, but the original idea of this PR still seems like a great, essential feature... My workaround is defining multiple storage backends and using them as appropriate: from storages.backends.s3boto3 import S3Boto3Storage class PrivateS3OverwriteStorage(S3Boto3Storage): """Only specify params that are different from those in Django settings.""" def __init__(self): super().__init__(file_overwrite=True) class PublicS3OverwriteStorage(S3Boto3Storage): """Only specify params that are different from those in Django settings.""" def __init__(self): super().__init__(default_acl='public-read', file_overwrite=True, querystring_auth=False) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#524?email_source=notifications&email_token=AAREDWAG5MPOL7Q5WCQQQ4DRF7INTA5CNFSM4FITQSNKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEN57A6A#issuecomment-595325048>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAREDWHJ65SUS7GINAGIGN3RF7INTANCNFSM4FITQSNA> .

jdufresne · 2020-03-07T15:13:54Z

Great! I have rebased.

jschneier · 2020-03-09T05:09:50Z

Can you please change the method name to get_default_settings? Good to go after that. Thanks for your patience as usual.

Makes the storage classes usable with Django's override_settings. For example, projects can now change the S3 location & bucket when running their tests. Affects the following backends: - azure_storage - gcloud - s3boto3 The remaining storage backends do not need to be altered as they do no calculate settings at import time. The class BaseStorage was created to use a common pattern to initialize settings for various backends. Users of these backends can expect them to work consistently. This class is fully backwards compatible. Settings can now be set: - In settings.py - Using override_settings - As a class variable - As an argument to __init__ Closes #498 Completes all necessary backends and adds tests.

jdufresne · 2020-03-09T13:36:17Z

👍 done.

terencehonles · 2020-03-18T17:13:14Z

@jdufresne @jschneier I was just updating an unrelated PR, but I was wondering should BaseStorage provide a default for get_default_settings which returns an empty {}? Right now this would break for any custom storages, but if the base storage was customized to not raise ImproperlyConfigured but a deprecated warning it would be a soft fail until the next major release.

Since it will only take me a couple moments to write and I'm already updating an existing PR I'll just create a PR for this, and we can go from there.

jdufresne · 2020-03-18T17:16:36Z

Right now this would break for any custom storages,

Custom storages aren't yet inheriting from BaseStorage as it is brand new.

But I'd also be fine with an implementation that return {}.

terencehonles · 2020-03-18T17:22:30Z

@jdufresne I was just going to update my comment with that, sorry, disregard my previous message.

I'm not sure an empty {} is warranted because it's likely that a storage is providing some options, and it should use the get_default_settings for that? Or do you suspect people would create a new storage based off the BaseStorage that would have no settings? I guess for maybe a private source/single use storage that might be the case.

Similar to jschneier/django-storages#524 - makes testing easier

…rted This allows the "storages.backends.s3boto3" module to be cleanly imported before Django settings are configured. This further supports the changes from jschneier#524. Removing "S3Boto3StorageFile.buffer_size" as a class variable does not affect the API surface of S3Boto3StorageFile, as "buffer_size" was always accessible as an instance variable.

…rted This allows the "storages.backends.s3boto3" module to be cleanly imported before Django settings are configured. This further supports the changes from jschneier#524. Removing "S3Boto3StorageFile.buffer_size" as a class variable does not affect the API surface of "S3Boto3StorageFile", as "buffer_size" was always accessible as an instance variable.

…rted (#930) This allows the "storages.backends.s3boto3" module to be cleanly imported before Django settings are configured. This further supports the changes from #524. Removing "S3Boto3StorageFile.buffer_size" as a class variable does not affect the API surface of "S3Boto3StorageFile", as "buffer_size" was always accessible as an instance variable.

…rted (jschneier#930) This allows the "storages.backends.s3boto3" module to be cleanly imported before Django settings are configured. This further supports the changes from jschneier#524. Removing "S3Boto3StorageFile.buffer_size" as a class variable does not affect the API surface of "S3Boto3StorageFile", as "buffer_size" was always accessible as an instance variable.

jdufresne mentioned this pull request Jul 6, 2018

S3 storage backend is not @override_settings friendly #498

Closed

sww314 reviewed Jul 12, 2018

View reviewed changes

This was referenced Aug 11, 2018

Deprecate old arguments acl & bucket to S3Boto3Storage #516

Merged

AWS credentials only work in base settings module #67

Closed

Consider a GenericStorage or SettingStorage #444

Closed

jdufresne mentioned this pull request Mar 7, 2020

Allow SFTPStorage to be configured via class variables #852

Merged

jschneier merged commit 955ee63 into jschneier:master Mar 10, 2020

jdufresne deleted the override branch March 18, 2020 17:24

terencehonles mentioned this pull request Mar 18, 2020

add default implementation of get_default_settings to BaseStorage #864

Merged

tomkins added a commit to developersociety/devsoc-contentfiles that referenced this pull request Apr 29, 2020

Update settings when storage class is instantiated

a001893

Similar to jschneier/django-storages#524 - makes testing easier

brianhelba mentioned this pull request Sep 8, 2020

Calculate settings for S3Boto3StorageFile when instantiated, not imported #930

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calculate settings when storage is instantiated; not imported #524

Calculate settings when storage is instantiated; not imported #524

jdufresne commented Jul 6, 2018

codecov-io commented Jul 6, 2018 •

edited

Loading

sww314 commented Jul 6, 2018

jdufresne commented Jul 9, 2018

jdufresne commented Jul 11, 2018

sww314 left a comment

sww314 commented Jul 12, 2018

jschneier commented Aug 11, 2018

jdufresne commented Aug 12, 2018

sww314 commented Sep 11, 2018

jdufresne commented Sep 17, 2018

yozlet commented Oct 24, 2018

sushifan commented Nov 15, 2018

jschneier commented Nov 15, 2018 via email

yozlet commented May 20, 2019

jdufresne commented May 21, 2019

jdufresne commented Mar 5, 2020

sushifan commented Mar 5, 2020

jschneier commented Mar 5, 2020 via email

jdufresne commented Mar 7, 2020

jschneier commented Mar 9, 2020

jdufresne commented Mar 9, 2020

terencehonles commented Mar 18, 2020

jdufresne commented Mar 18, 2020

terencehonles commented Mar 18, 2020

Calculate settings when storage is instantiated; not imported #524

Calculate settings when storage is instantiated; not imported #524

Conversation

jdufresne commented Jul 6, 2018

codecov-io commented Jul 6, 2018 • edited Loading

Codecov Report

sww314 commented Jul 6, 2018

jdufresne commented Jul 9, 2018

jdufresne commented Jul 11, 2018

sww314 left a comment

Choose a reason for hiding this comment

sww314 commented Jul 12, 2018

jschneier commented Aug 11, 2018

jdufresne commented Aug 12, 2018

sww314 commented Sep 11, 2018

jdufresne commented Sep 17, 2018

yozlet commented Oct 24, 2018

sushifan commented Nov 15, 2018

jschneier commented Nov 15, 2018 via email

yozlet commented May 20, 2019

jdufresne commented May 21, 2019

jdufresne commented Mar 5, 2020

sushifan commented Mar 5, 2020

jschneier commented Mar 5, 2020 via email

jdufresne commented Mar 7, 2020

jschneier commented Mar 9, 2020

jdufresne commented Mar 9, 2020

terencehonles commented Mar 18, 2020

jdufresne commented Mar 18, 2020

terencehonles commented Mar 18, 2020

codecov-io commented Jul 6, 2018 •

edited

Loading