-
-
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?
Conversation
Co-authored-by: humitos <[email protected]>
Co-authored-by: humitos <[email protected]>
humitos
left a comment
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 good to me. I tested it locally and it worked. I will check -ops repositories and corporate now.
I didn't find any code that requires changes there. |
| storage = storages[alias] | ||
| return type(storage) |
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
import_string(storages.backends[alias]['BACKEND'])
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_string but since the class is already imported, calling type over the instance makes more sense to me than re-importing.
| serve.build_media_storage = get_storage_class( | ||
| settings.RTD_BUILD_MEDIA_STORAGE | ||
| )() | ||
| serve.build_media_storage = storages["build-media"] |
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.
Checking the comment, this instance is also probably cached and will make some tests fail.
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.
It seems that all tests are passing.
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.
Should we remove this line completely? Seems Django is smart enough and it's not required anymore?
| serve.build_media_storage = storages["build-media"] |
| class ConfiguredBuildMediaStorage(LazyObject): | ||
| def _setup(self): | ||
| self._wrapped = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)() | ||
| self._wrapped = storages["build-media"] |
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.
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.
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.
Hrm, I'm not sure. The documentation doesn't say too much about lazyness.
I asked AI about this and it says that storage[alias] is not lazy by default and we should do what we are currently doing if we want it to be lazy.
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.
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
Co-authored-by: Santos Gallegos <[email protected]>
Migrates custom storage backends (
build-media,build-commands,build-tools) to Django'sSTORAGESsetting, enabling use of the standarddjango.core.files.storage.storagesAPI.Changes
STORAGESproperty inbase.py,docker_compose.py, andtest.pyreadthedocs/storage/__init__.pyto usestorages["alias"]instead ofget_storage_class(settings.RTD_*)readthedocs/projects/tasks/storage.pyto retrieve storage classes viastoragesAPIproxito/tests/base.pyandrtd_tests/tests/test_imported_file.pyto new patternget_storage_class()functionUsage
The lazy-loaded instances (
build_media_storage,build_commands_storage, etc.) continue to work unchanged.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
search/usr/bin/python python -m pytest readthedocs/rtd_tests/tests/test_imported_file.py -v --tb=short(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
STORAGESsetting to the new pattern #12221💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.