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

Customize default build media storage for the FS #6215

Merged
merged 2 commits into from Sep 27, 2019

Conversation

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Sep 27, 2019

Edit: closely mirror the logic to get the production media path:

if settings.DEFAULT_PRIVACY_LEVEL == 'public' or settings.DEBUG:
path = os.path.join(
settings.MEDIA_ROOT,
type_,
self.slug,
version_slug,
)
else:
path = os.path.join(
settings.PRODUCTION_MEDIA_ARTIFACTS,
type_,
self.slug,
version_slug,
)
if include_file:

- Prioritize PRODUCTION_MEDIA_ARTIFACTS if it exists
- Make it customizable
- Fallback to MEDIA_ROOT
@davidfischer davidfischer requested a review from Sep 27, 2019
Copy link
Member

@ericholscher ericholscher left a comment

This looks like a good solution for .com -- do we actually want to pass a location when on the .org, since we aren't actually serving it out of the MEDIA_ROOT?

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Sep 27, 2019

do we actually want to pass a location when on the .org

In production, this class is not used on .org at all. We use an Azure storage class that mixes in BuildMediaStorageMixin. In development for .org, perhaps a better approach is to actually have settings.PRODUCTION_MEDIA_ARTIFACTS have meaning. That is, we could actually write build artifacts there as opposed to it being a meaningless, non-existent folder which it is now. In that case, we could just always make this class use settings.PRODUCTION_MEDIA_ARTIFACTS.

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Sep 27, 2019

The biggest drawback to what I outlined above as far as I can tell is that people will have to move their built artifacts to a different directory in dev.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Sep 27, 2019

That might make sense. We can always work around having a secondary path, it's going to be much harder to tease apart the build output path, normal media path usage, and improper/old static file media path usage. Locally, we'd probably still run the local build storage on community development, we don't want to be writing to Azure from local dev.

We can come back to this case, but I'd agree that the production artifact path should be used in local development as well.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Sep 27, 2019

I'll be applying this as hotfix to .com later, the change seems to look good

@agjohnson agjohnson merged commit 6031afa into master Sep 27, 2019
3 checks passed
@agjohnson agjohnson deleted the davidfischer/prod-media-artifacts-storage branch Sep 27, 2019
@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Sep 27, 2019

Locally, we'd probably still run the local build storage on community development, we don't want to be writing to Azure from local dev.

Just to be clear, I completely agree. I'm proposing basically having prod_artifacts/media exist in development.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Sep 30, 2019

I'm proposing basically having prod_artifacts/media exist in development.

I think this is probably the right approach. It also works well w/ the move to having all the build media served via the webs as well, instead of directly off the media server.

stsewd added a commit to stsewd/readthedocs.org that referenced this issue Sep 30, 2019
This is to fix tests on .com.
This test broke with the new changes on readthedocs#6215
agjohnson added a commit that referenced this issue Oct 2, 2019
* Customize default build media storage for the FS

- Prioritize PRODUCTION_MEDIA_ARTIFACTS if it exists
- Make it customizable
- Fallback to MEDIA_ROOT

* More closely mirror the production media path logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants