Skip to content

Conversation

@jcapphelix
Copy link
Contributor

@jcapphelix jcapphelix commented Oct 6, 2025

Description

This PR refactors the code to replace the deprecated DEFAULT_FILE_STORAGE and STATICFILES_STORAGE settings with the more flexible and recommended STORAGES = {} pattern, a change that was recently done in various repos in community.

For more details Refer to Jira links

2U Private Jira Link

@jcapphelix jcapphelix marked this pull request as ready for review October 6, 2025 11:10
Copilot AI review requested due to automatic review settings October 6, 2025 11:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates Django configuration from deprecated DEFAULT_FILE_STORAGE and STATICFILES_STORAGE settings to the modern STORAGES dictionary pattern required for Django 5.2 compatibility.

  • Replaced deprecated storage settings with the new STORAGES configuration structure
  • Updated Python configuration files to conditionally set storage backends via STORAGES
  • Updated YAML configuration files to define storage backends using the new format

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
py_configuration_files/credentials.py Migrated from deprecated storage settings to conditional STORAGES configuration
configuration_files/lms.yml Replaced deprecated DEFAULT_FILE_STORAGE with STORAGES dictionary definition
configuration_files/discovery.yml Updated both DEFAULT_FILE_STORAGE and STATICFILES_STORAGE to use STORAGES format
configuration_files/cms.yml Replaced deprecated DEFAULT_FILE_STORAGE with STORAGES dictionary definition

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 47 to 53
if defaultfile_storage:
STORAGES["default"]["BACKEND"] = defaultfile_storage

staticfiles_storage = os.environ.get("STATICFILES_STORAGE")

if staticfiles_storage:
STORAGES["staticfiles"]["BACKEND"] = staticfiles_storage
Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

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

The code attempts to access nested dictionary keys without ensuring the parent dictionaries exist. This will raise a KeyError if STORAGES is not already defined or if the 'default'/'staticfiles' keys don't exist. Consider using STORAGES.setdefault('default', {})['BACKEND'] = defaultfile_storage or ensure STORAGES is properly initialized first.

Suggested change
if defaultfile_storage:
STORAGES["default"]["BACKEND"] = defaultfile_storage
staticfiles_storage = os.environ.get("STATICFILES_STORAGE")
if staticfiles_storage:
STORAGES["staticfiles"]["BACKEND"] = staticfiles_storage
# Ensure STORAGES is defined
if "STORAGES" not in globals():
STORAGES = {}
if defaultfile_storage:
STORAGES.setdefault("default", {})["BACKEND"] = defaultfile_storage
staticfiles_storage = os.environ.get("STATICFILES_STORAGE")
if staticfiles_storage:
STORAGES.setdefault("staticfiles", {})["BACKEND"] = staticfiles_storage

Copilot uses AI. Check for mistakes.
Comment on lines 48 to 53
STORAGES["default"]["BACKEND"] = defaultfile_storage

staticfiles_storage = os.environ.get("STATICFILES_STORAGE")

if staticfiles_storage:
STORAGES["staticfiles"]["BACKEND"] = staticfiles_storage
Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

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

The code attempts to access nested dictionary keys without ensuring the parent dictionaries exist. This will raise a KeyError if STORAGES is not already defined or if the 'default'/'staticfiles' keys don't exist. Consider using STORAGES.setdefault('default', {})['BACKEND'] = defaultfile_storage or ensure STORAGES is properly initialized first.

Suggested change
STORAGES["default"]["BACKEND"] = defaultfile_storage
staticfiles_storage = os.environ.get("STATICFILES_STORAGE")
if staticfiles_storage:
STORAGES["staticfiles"]["BACKEND"] = staticfiles_storage
STORAGES.setdefault("default", {})["BACKEND"] = defaultfile_storage
staticfiles_storage = os.environ.get("STATICFILES_STORAGE")
if staticfiles_storage:
STORAGES.setdefault("staticfiles", {})["BACKEND"] = staticfiles_storage

Copilot uses AI. Check for mistakes.
Copy link
Member

@deborahgu deborahgu left a comment

Choose a reason for hiding this comment

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

the credentials side of this looks good to me. I put in a suggestion that I would like you to use (being consistent about the presence of underscores between environment variable name and corresponding variable name), but I'm not marking as request changes because it's a nitpick.

@jcapphelix jcapphelix merged commit a4475b7 into master Oct 7, 2025
19 checks passed
@jcapphelix jcapphelix deleted the chintanjoshi-apphelix-2u/fix-storages branch October 7, 2025 07:13
@edx edx deleted a comment from Copilot AI Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants