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

Serve non-html files from nginx (X-Accel-Redirect) #6404

Merged
merged 14 commits into from Nov 26, 2019

Conversation

@humitos
Copy link
Member

humitos commented Nov 25, 2019

Remove the "if" that checks if we are in community or commercial and always serve the files from storage (S3 on commercial). The URL generated will have an AccessKey and Expires arguments.

EDIT: this PR also returns a response with X-Accel-Redirect to serve the file from NGINX instead of redirecting the user to the container in BlobStorage (community). This allows us to use the Content-Disposition header and return the filename with the version slug on it.

Related to #6326 --we can probably merge both together.

Remove the "if" that checks if we are in community or commercial and
always serve the files from storage (S3 on commercial). The URL
generated will have an AccessKey and Expires arguments.
@humitos humitos requested review from davidfischer and agjohnson Nov 25, 2019
@humitos

This comment has been minimized.

Copy link
Member Author

humitos commented Nov 25, 2019

This change will bring a possible refactor of Project.get_production_media_path (probably remove it completely) and make more usage of Project.get_storage_path instead.

humitos added 4 commits Nov 25, 2019
This allows us to use Content-Disposition header with the proper
filename on the response (including version slug on it).

Besides, on corporate, this will allow us to serve non-html files
without exposing the AccessKey and/or Expires and handle this internally.
@humitos humitos force-pushed the humitos/commercial-serve-non-html-storage branch from a257ce8 to 6638b43 Nov 25, 2019
@humitos humitos force-pushed the humitos/commercial-serve-non-html-storage branch from 6638b43 to 540ad7f Nov 25, 2019
@humitos humitos requested a review from ericholscher Nov 25, 2019
Copy link
Member

ericholscher left a comment

This looks like a good approach to me. It requires some nginx changes on .org & .com, but it will make our UX around project downloads the same across both properties, which is really nice. It will let us serve the files directly from:

And they will have the proper time-test-latest.epub filename, which feels 💯

readthedocs/proxito/views/serve.py Outdated Show resolved Hide resolved
readthedocs/proxito/views/mixins.py Outdated Show resolved Hide resolved
Copy link
Member

ericholscher left a comment

This looks good to me. We need to make sure that .com is ready for it when it gets deployed, but 👍 overall.

@humitos humitos changed the title Serve non-html files from storage in commercial Serve non-html files from nginx (X-Accel-Redirect) Nov 25, 2019
Copy link
Contributor

davidfischer left a comment

I made a small suggestion but this looks good.

readthedocs/proxito/views/mixins.py Outdated Show resolved Hide resolved
Co-Authored-By: David Fischer <david@readthedocs.org>
@humitos humitos merged commit 5f22355 into master Nov 26, 2019
3 checks passed
3 checks passed
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details
@humitos humitos deleted the humitos/commercial-serve-non-html-storage branch Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.