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

All static media is run through "collectstatic" #4489

Merged
merged 4 commits into from Aug 14, 2018

Conversation

Projects
None yet
3 participants
@davidfischer
Contributor

davidfischer commented Aug 7, 2018

This changes how our static files are collected. Rather than media/ being a directory where static files are served from and collectstatic writes to media/static, this turns the media/ directory into just another entry in STATICFILES_DIRS and all static media is collected into a new directory static/.

This is a somewhat difficult PR to test thoroughly and it is possible that issues we don't anticipate might arise from it. It makes me a bit nervous to be honest. One challenge is that a lot of built documentation still uses /media/ (eg. /media/javascript/jquery/jquery-migrate-1.2.1.min.js)

davidfischer added some commits Aug 7, 2018

@davidfischer davidfischer requested a review from agjohnson Aug 7, 2018

@davidfischer

This comment has been minimized.

Show comment
Hide comment
@davidfischer

davidfischer Aug 7, 2018

Contributor

This line might hide some problems as well.

Contributor

davidfischer commented Aug 7, 2018

This line might hide some problems as well.

@@ -1,12 +0,0 @@
<!DOCTYPE HTML>

This comment has been minimized.

@davidfischer

davidfischer Aug 8, 2018

Contributor

This file does not appear to be used anywhere. I took a look at the commit that added it 6 years ago (38ca662) and most of that code is gone.

@davidfischer

davidfischer Aug 8, 2018

Contributor

This file does not appear to be used anywhere. I took a look at the commit that added it 6 years ago (38ca662) and most of that code is gone.

@humitos

I'm still trying to understand how our static/media works. So, maybe my questions are irrelevant.

One challenge is that a lot of built documentation still uses /media/

How these docs will behave? Will they fail to load the resources?

Also, I want to raise a question here to anticipate a possible issue: how this will affect the corporate site? It looks that we already have a question there:

https://github.com/rtfd/readthedocs-corporate/blob/0f7590f4674028a539673b331194cd2d0ed7cdc6/readthedocsinc/settings/base.py#L68-L77

Show outdated Hide outdated media/javascript/underscore.js Outdated
@davidfischer

This comment has been minimized.

Show comment
Hide comment
@davidfischer

davidfischer Aug 8, 2018

Contributor

How these docs will behave? Will they fail to load the resources?

As long as we continue to serve the media directory, no. Longer term, we should probably redirect or something else.

Contributor

davidfischer commented Aug 8, 2018

How these docs will behave? Will they fail to load the resources?

As long as we continue to serve the media directory, no. Longer term, we should probably redirect or something else.

@davidfischer

This comment has been minimized.

Show comment
Hide comment
@davidfischer

davidfischer Aug 8, 2018

Contributor

I went ahead and tagged this "work in progress". I don't think this PR will get merged as-is.

Contributor

davidfischer commented Aug 8, 2018

I went ahead and tagged this "work in progress". I don't think this PR will get merged as-is.

@davidfischer

This comment has been minimized.

Show comment
Hide comment
@davidfischer

davidfischer Aug 9, 2018

Contributor

As long as we continue to serve the media directory, no. Longer term, we should probably redirect or something else.

I updated this PR to redirect for those resources (eg. /media/javascript/jquery/jquery-migrate-1.2.1.min.js -> /static/javascript/jquery/jquery-migrate-1.2.1.min.js). In production, this should be a redirect at the web server level, not Django. Once this is rolled out, we should update various bits of code (in https://github.com/rtfd/readthedocs-sphinx-ext for example) to use the new URL.

Contributor

davidfischer commented Aug 9, 2018

As long as we continue to serve the media directory, no. Longer term, we should probably redirect or something else.

I updated this PR to redirect for those resources (eg. /media/javascript/jquery/jquery-migrate-1.2.1.min.js -> /static/javascript/jquery/jquery-migrate-1.2.1.min.js). In production, this should be a redirect at the web server level, not Django. Once this is rolled out, we should update various bits of code (in https://github.com/rtfd/readthedocs-sphinx-ext for example) to use the new URL.

@davidfischer

This comment has been minimized.

Show comment
Hide comment
@davidfischer

davidfischer Aug 9, 2018

Contributor

Another option is to move all the files under media/ to readthedocs/static and leave media/ exclusively for build artifacts. Then media/ could be removed from STATICFILES_DIRS. We would still need to redirect /media -> /static for non-build artifact files though.

Contributor

davidfischer commented Aug 9, 2018

Another option is to move all the files under media/ to readthedocs/static and leave media/ exclusively for build artifacts. Then media/ could be removed from STATICFILES_DIRS. We would still need to redirect /media -> /static for non-build artifact files though.

agjohnson added a commit that referenced this pull request Aug 9, 2018

Use collectstatic on `media/`, without collecting user files
This doesn't change the STATICFILES_ROOT yet, in an attempt to avoid making any
breaking changes. This is a port of #4489 that we'll merge before azure
migration
@davidfischer

This comment has been minimized.

Show comment
Hide comment
@davidfischer

davidfischer Aug 10, 2018

Contributor

I think the plan with this PR is to merge #4502 first (before the Azure move) and then merge this relatively soon after.

Contributor

davidfischer commented Aug 10, 2018

I think the plan with this PR is to merge #4502 first (before the Azure move) and then merge this relatively soon after.

@ericholscher

This comment has been minimized.

Show comment
Hide comment
@ericholscher

ericholscher Aug 14, 2018

Member

I've merged master in after #4502, and now will merge this in.

Member

ericholscher commented Aug 14, 2018

I've merged master in after #4502, and now will merge this in.

@ericholscher ericholscher merged commit ce3e4f9 into master Aug 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the davidfischer/all-static-collectstatic branch Aug 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment