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

Use collectstatic on media/, without collecting user files #4502

Merged
merged 3 commits into from Aug 14, 2018

Conversation

agjohnson
Copy link
Contributor

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

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
Copy link
Contributor

@davidfischer davidfischer left a comment

Choose a reason for hiding this comment

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

This is close but I think we just need one change.

os.path.join(SITE_ROOT, 'media'),
]
STATICFILES_FINDERS = [
'readthedocs.core.static.SelectiveFileSystemFinder',
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know you could do that. Seems useful!

STATICFILES_DIRS = [os.path.join(SITE_ROOT, 'readthedocs', 'static')]
STATICFILES_DIRS = [
os.path.join(SITE_ROOT, 'readthedocs', 'static'),
os.path.join(SITE_ROOT, 'media'),
Copy link
Contributor

Choose a reason for hiding this comment

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

This still seems a bit weird to me to collect files from a path (/media) and put them in a subdirectory (/media/static) but I guess it's ok with the SelectiveFileSystemFinder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah agreed. I only worry that there are some deep parts requiring the media path, as this pattern goes back to early django I believe. If we run into any weirdness here, I think we can probably justify taking on work to separate static.

"""

def list(self, ignore_patterns):
ignore_patterns.extend(['epub', 'pdf', 'htmlzip', 'json', 'man'])
Copy link
Contributor

Choose a reason for hiding this comment

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

We need static here (and hopefully there's no side effects). Otherwise when you run collectstatic multiple times, you end up with media/static/static/static/....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was on the fence whether to add this -- as we also (unfortunately) have a static/ in readthedocs/ that I was curious if we'd skip. I'll give a test here.

Copy link
Contributor

Choose a reason for hiding this comment

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

And it looks like you can't do static/static...

Copy link
Contributor

Choose a reason for hiding this comment

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

The other answer is we use --clear in dev at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, you're right, multiple runs makes a nest of static/ paths. I still see the files collected that I was worried about, so I think we're safe.

@ericholscher ericholscher merged commit 5f993a0 into master Aug 14, 2018
@stsewd stsewd deleted the agj/test-add-media-collection branch October 1, 2018 04:04
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.

None yet

3 participants