-
-
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 3 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 |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
|
|
||
| import structlog | ||
| from django.conf import settings | ||
| from django.utils.module_loading import import_string | ||
| from django.core.files.storage import storages | ||
|
|
||
| from readthedocs.doc_builder.exceptions import BuildAppError | ||
| from readthedocs.projects.models import Feature | ||
|
|
@@ -53,13 +53,24 @@ def _get_storage_class(storage_type: StorageType): | |
| raise ValueError("Invalid storage type") | ||
|
|
||
|
|
||
| def _get_storage_backend(alias: str): | ||
| """ | ||
| Get the storage backend class for a given alias from STORAGES setting. | ||
|
|
||
| This returns the class, not an instance, so it can be instantiated | ||
| with custom kwargs (e.g., per-build credentials). | ||
| """ | ||
| storage = storages[alias] | ||
| return type(storage) | ||
|
Comment on lines
+63
to
+64
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. Looks like this can be done with since there is no need for an instance
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. Yeah, we could use |
||
|
|
||
|
|
||
| def _get_build_media_storage_class(): | ||
| """ | ||
| Get a storage class to use for syncing build artifacts. | ||
|
|
||
| This is done in a separate method to make it easier to mock in tests. | ||
| """ | ||
| return import_string(settings.RTD_BUILD_MEDIA_STORAGE) | ||
| return _get_storage_backend("build-media") | ||
humitos marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| def _get_build_tools_storage_class(): | ||
|
|
@@ -68,7 +79,7 @@ def _get_build_tools_storage_class(): | |
|
|
||
| This is done in a separate method to make it easier to mock in tests. | ||
| """ | ||
| return import_string(settings.RTD_BUILD_TOOLS_STORAGE) | ||
| return _get_storage_backend("build-tools") | ||
humitos marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| def _get_s3_scoped_credentials(*, project, build_id, api_client, storage_type: StorageType): | ||
|
|
||
| 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,51 @@ | |
| so doing those upfront improves performance. | ||
| """ | ||
|
|
||
| from django.conf import settings | ||
| import warnings | ||
|
|
||
| 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) | ||
| """ | ||
| Get a storage class from an import path. | ||
| .. deprecated:: | ||
| This function is deprecated. Use Django's ``storages`` API instead: | ||
| ``from django.core.files.storage import storages`` | ||
| and access storage by alias: ``storages["build-media"]`` | ||
| """ | ||
| warnings.warn( | ||
| "get_storage_class is deprecated. Use Django's storages API instead: " | ||
| "from django.core.files.storage import storages; storages['alias']", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
humitos marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| from django.conf import settings as django_settings | ||
|
|
||
| return import_string(import_path or django_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() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.