Skip to content

18896 Replace STORAGE_BACKEND with STORAGES and support Script running from S3 #18680

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

Merged
merged 21 commits into from
Mar 17, 2025

Conversation

arthanson
Copy link
Collaborator

@arthanson arthanson commented Feb 19, 2025

Fixes: #18896 (and #18423)

This Allows scripts to be run from S3 as well as config changes to allow for setting it. The changes are in two basic parts - the Script Form is changed to upload files using Django-storages (either FileSystem or S3) and the Script module loader is changed to pull from Django-storages.

  • Django-storages is now a requirement
  • STORAGE_BACKEND is now deprecated
  • STORAGES is now set directly as a config option
  • write_to_disk was moved from DataFile to ManagedFile as it is only ever used there or in classes derived from it.
  • The changes in BaseScript find_source is a replacement for the inspect module to actually find and load the modules.
  • extras.storage.ScriptFileSystemStorage is required for backwards compatibility with SCRIPTS_ROOT setting as otherwise with normal FileSystemStorage it will cause a path permission error going outside media root.

Note: the Script convenience methods load_yaml and load_json are not compatible when using S3 as they go direct to the local file system and are based on the path of the Script so wouldn't be backwards compatible. They are convenience functions so the script author could write them directly.

RELEASE_NOTES should probably have a section about replacing STORAGE_BACKEND and STORAGE_CONFIG with STORAGES for those updating from a previous version.

@jeremystretch jeremystretch added this to the v4.3 milestone Feb 21, 2025
@arthanson arthanson marked this pull request as ready for review February 27, 2025 15:46
@arthanson arthanson changed the title DRAFT: 18423 django storages 18423 Replace STORAGE_BACKEND with STORAGES and support Script running from S3 Feb 27, 2025
Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

Looking good! Found a few things I think we can clean up.

@jeremystretch
Copy link
Member

@arthanson could you submit a retroactive FR for this work please? The scope of #18423 was just the configuration parameter change. The changes here have a far broader impact (such as introducing django-storages as a new mandatory dependency) that need to be captured.

@arthanson arthanson requested a review from jeremystretch March 6, 2025 21:32
@arthanson arthanson requested a review from jeremystretch March 7, 2025 21:53
@arthanson arthanson requested a review from jeremystretch March 10, 2025 15:46
jeremystretch
jeremystretch previously approved these changes Mar 10, 2025
Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

Thanks @arthanson!

@jeremystretch
Copy link
Member

This still needs a corresponding FR which captures the full extent of changes made in the PR. #18423 is not sufficient for this in its current form.

@arthanson
Copy link
Collaborator Author

arthanson commented Mar 12, 2025

This still needs a corresponding FR which captures the full extent of changes made in the PR. #18423 is not sufficient for this in its current form.

Expanded the issue with more details. Please let me know if there are any details missing or how it should be expanded if needed.

@arthanson arthanson requested a review from jeremystretch March 12, 2025 16:45
@jeremystretch
Copy link
Member

@arthanson please revert your edits to #18423 and open a new feature request to capture the intent of this PR, and re-target this PR to the new FR. I opened #18423 solely to propose a change to the configuration parameters in use. I'm not comfortable having it appear as though I requested this change; my involvement in this work has been limited to reviewing the proposed changes at a technical level.

@arthanson arthanson changed the title 18423 Replace STORAGE_BACKEND with STORAGES and support Script running from S3 18896 Replace STORAGE_BACKEND with STORAGES and support Script running from S3 Mar 13, 2025
@arthanson
Copy link
Collaborator Author

Created issue #18896

@arthanson arthanson merged commit 1b4e00a into feature Mar 17, 2025
6 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2025
@jeremystretch jeremystretch removed this from the v4.3 milestone May 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants