-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Upgrade STORAGES setting to use Django's standard pattern #12625
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
base: main
Are you sure you want to change the base?
Changes from all commits
ada1ef5
1fcb668
90de92a
a848f3f
bceb15d
247f637
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -2,9 +2,8 @@ | |||
|
|
||||
| import django_dynamic_fixture as fixture | ||||
| import pytest | ||||
| from django.conf import settings | ||||
| from django.contrib.auth.models import User | ||||
| from readthedocs.storage import get_storage_class | ||||
| from django.core.files.storage import storages | ||||
| from django.test import TestCase | ||||
|
|
||||
| from readthedocs.builds.constants import LATEST | ||||
|
|
@@ -19,9 +18,7 @@ def setUp(self): | |||
| # Re-initialize storage | ||||
| # Various tests override either this setting or various aspects of the storage engine | ||||
| # By resetting it every test case, we avoid this caching (which is a huge benefit in prod) | ||||
| serve.build_media_storage = get_storage_class( | ||||
| settings.RTD_BUILD_MEDIA_STORAGE | ||||
| )() | ||||
| serve.build_media_storage = storages["build-media"] | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checking the comment, this instance is also probably cached and will make some tests fail.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that all tests are passing.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we remove this line completely? Seems Django is smart enough and it's not required anymore?
Suggested change
|
||||
|
|
||||
| self.eric = fixture.get(User, username="eric") | ||||
| self.eric.set_password("eric") | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,38 +6,28 @@ | |
| so doing those upfront improves performance. | ||
| """ | ||
|
|
||
| from django.conf import settings | ||
| from django.core.files.storage import storages | ||
| from django.utils.functional import LazyObject | ||
| from django.utils.module_loading import import_string | ||
|
|
||
|
|
||
| # Borrowed from Django 4.2 since it was deprecrated and removed in 5.2 | ||
| # NOTE: we can use settings.STORAGES for our own storages as well if we want to use the standards. | ||
| # | ||
| # https://docs.djangoproject.com/en/5.0/ref/settings/#std-setting-STORAGES) | ||
| # https://github.com/django/django/blob/4.2/django/core/files/storage/__init__.py#L31 | ||
| def get_storage_class(import_path=None): | ||
| return import_string(import_path or settings.DEFAULT_FILE_STORAGE) | ||
|
|
||
|
|
||
| class ConfiguredBuildMediaStorage(LazyObject): | ||
| def _setup(self): | ||
| self._wrapped = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)() | ||
| self._wrapped = storages["build-media"] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these were made in order to lazy instantiate the storage object. storages[alias] does that by default, probably? We should check, we don't need these wrappers otherwise.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hrm, I'm not sure. The documentation doesn't say too much about lazyness. I asked AI about this and it says that
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked the Django's code and it seems it's not lazy: https://github.com/django/django/blob/dba622ebc1f6a6700b75303a65f8a334bd46bd8e/django/core/files/storage/handler.py#L11 |
||
|
|
||
|
|
||
| class ConfiguredBuildCommandsStorage(LazyObject): | ||
| def _setup(self): | ||
| self._wrapped = get_storage_class(settings.RTD_BUILD_COMMANDS_STORAGE)() | ||
| self._wrapped = storages["build-commands"] | ||
|
|
||
|
|
||
| class ConfiguredBuildToolsStorage(LazyObject): | ||
| def _setup(self): | ||
| self._wrapped = get_storage_class(settings.RTD_BUILD_TOOLS_STORAGE)() | ||
| self._wrapped = storages["build-tools"] | ||
|
|
||
|
|
||
| class ConfiguredStaticStorage(LazyObject): | ||
| def _setup(self): | ||
| self._wrapped = get_storage_class(settings.RTD_STATICFILES_STORAGE)() | ||
| self._wrapped = storages["staticfiles"] | ||
|
|
||
|
|
||
| build_media_storage = ConfiguredBuildMediaStorage() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this can be done with
since there is no need for an instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we could use
import_stringbut since the class is already imported, callingtypeover the instance makes more sense to me than re-importing.