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

Migrate to the unified STORAGES setting added in Django 4.2 #4477

Merged

Conversation

browniebroke
Copy link
Member

@browniebroke browniebroke commented Jul 21, 2023

Description

There was a PR open for it in #4457 but the author closed it before it could be merged. Re-opening it with commits attributions.

Checklist:

  • I've made sure that tests are updated accordingly (especially if adding or updating a template option)
  • I've updated the documentation or confirm that my change doesn't require any updates

Rationale

Fix #4443

# Conflicts:
#	{{cookiecutter.project_slug}}/config/settings/production.py
Copy link
Contributor

@jkaeske jkaeske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks fine to me.
I made one comment to specify the object storage options directly in the settings file as recommended in the django-storages docs.
Is there something missing or could we get this merged?

},
{%- elif cookiecutter.cloud_provider == 'AWS' %}
"default": {
"BACKEND": "{{cookiecutter.project_slug}}.utils.storages.MediaS3Storage",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid relying on the instantiated storage classes, should we provide the settings here directly like so:

"default": {
    "BACKEND": "storages.backends.s3.S3Storage",
    "OPTIONS": {
        "location": "static",
        "default_acl": "public-read",
    },
},
"staticfiles": {
    "BACKEND": "storages.backends.s3.S3Storage",
    "OPTIONS": {
        "location": "media",
        "file_overwrite": False,
    },
},

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes great suggestion

@browniebroke
Copy link
Member Author

Is there something missing or could we get this merged?

Last time I tried to deploy this change I hit some permission issues with AWS S3, but I couldn't figure out if it was caused by this PR or soemthing that changed in AWS.

If you could try out a combination or 2 you care about, and feedback your results, that would be very helpful. You can run it against my branch with: cookiecutter https://github.com/browniebroke/cookiecutter-django --checkout migrate/unified-storages-setting

@jkaeske
Copy link
Contributor

jkaeske commented Jan 22, 2024

@browniebroke was this perhaps the error that you have seen?
botocore.exceptions.ClientError: An error occurred (AccessControlListNotSupported) when calling the PutObject operation: The bucket does not allow ACLs

There can also be an issue if the bucket is in the new eu-central-2 Zurich region, since it is opt-in

But other than that, if ACLs are enabled for the bucket, there are no permission issues in AWS during my test

@jkaeske
Copy link
Contributor

jkaeske commented Jan 24, 2024

I am happy to help change the settings file and delete storages.py if you like.
It should be straightforward from here to get this merged.

@browniebroke browniebroke merged commit 1899b48 into cookiecutter:master Jan 24, 2024
13 checks passed
@browniebroke browniebroke deleted the migrate/unified-storages-setting branch January 24, 2024 15:32
@browniebroke
Copy link
Member Author

I am happy to help change the settings file and delete storages.py if you like. It should be straightforward from here to get this merged.

It's now merged so feel free to create a new PR to do these changes 👍🏻 Thanks for your help in getting this reviewed.

@foarsitter
Copy link
Collaborator

Thanks @browniebroke!

@jkaeske
Copy link
Contributor

jkaeske commented Jan 24, 2024

I am happy to help change the settings file and delete storages.py if you like. It should be straightforward from here to get this merged.

It's now merged so feel free to create a new PR to do these changes 👍🏻 Thanks for your help in getting this reviewed.

Thanks to you @browniebroke !
I will create a new PR for the other changes

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.

Migrate the media & static settings to the new unified STORAGES
3 participants