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

Allow to use a CDN with S3 #2600

Merged
merged 2 commits into from
Jun 21, 2020
Merged

Allow to use a CDN with S3 #2600

merged 2 commits into from
Jun 21, 2020

Conversation

tanoabeleyra
Copy link
Contributor

Description

This PR allows using a CDN (e.g. AWS CloudFront) to serve the files stored in AWS S3. This is optional and backward-compatible, it will only be applied when specifying the environment variable DJANGO_AWS_S3_CUSTOM_DOMAIN.

Rationale

It's really common to use S3 with CloudFront, this setting makes it easier to achieve that.

@Andrew-Chen-Wang
Copy link
Contributor

You should add in that new environment variable to the .envs/.production/.django, writing a note there, adding some docs to the docs/settings.rst, and give a little more elaboration in the settings/production.py or the env file too.

@tanoabeleyra
Copy link
Contributor Author

Hi @Andrew-Chen-Wang, thanks for the feedback. Considering that this variable is optional, should it go in .envs/.production/.django? For instance, DJANGO_AWS_S3_REGION_NAME is optional and it isn't in the envs file.

The idea is that if the user doesn't give any value to DJANGO_AWS_S3_CUSTOM_DOMAIN it should behave like it's currently doing in master.

@Andrew-Chen-Wang
Copy link
Contributor

Ah I see. The "optional" justification makes sense.

@tanoabeleyra
Copy link
Contributor Author

Is there anything else needed for this to be merged? (cc: @browniebroke)

@browniebroke
Copy link
Member

Thanks for this!

Is there anything else needed for this to be merged?

Nothing apart from me finding some spare time :)

@tanoabeleyra
Copy link
Contributor Author

Hi @browniebroke. Sure, I understand, there's no rush actually, I just wanted to be sure that the changes looked ok to you.

Thanks for approving it!

@browniebroke browniebroke merged commit e095ee9 into cookiecutter:master Jun 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants