Skip to content
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

feat: add support for picking storage config from settings #344

Merged
merged 2 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion edx_sga/sga.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
from django.conf import settings
from django.core.exceptions import PermissionDenied
from django.core.files import File
from django.core.files.storage import default_storage
from django.template import Context, Template
from django.utils.encoding import force_str
from django.utils.timezone import now as django_now
Expand All @@ -44,6 +43,7 @@
from edx_sga.tasks import get_zip_file_name, get_zip_file_path, zip_student_submissions
from edx_sga.utils import (
file_contents_iter,
get_default_storage,
get_file_modified_time_utc,
get_file_storage_path,
get_sha1,
Expand All @@ -53,6 +53,8 @@

log = logging.getLogger(__name__)

default_storage = get_default_storage()


def reify(meth):
"""
Expand Down
5 changes: 3 additions & 2 deletions edx_sga/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,18 @@
import tempfile
import zipfile

from django.core.files.storage import default_storage
from celery import shared_task
from opaque_keys.edx.locator import BlockUsageLocator
from common.djangoapps.student.models import user_by_anonymous_id
from submissions import api as submissions_api

from edx_sga.constants import ITEM_TYPE
from edx_sga.utils import get_file_storage_path, is_finalized_submission
from edx_sga.utils import get_default_storage, get_file_storage_path, is_finalized_submission

log = logging.getLogger(__name__)

default_storage = get_default_storage()


def _get_student_submissions(block_id, course_id, locator):
"""
Expand Down
26 changes: 25 additions & 1 deletion edx_sga/tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
"""
Tests for SGA utility functions
"""
from django.test import override_settings
from django.core.files.storage import default_storage
import pytest
import pytz
from storages.backends.s3boto3 import S3Boto3Storage

from edx_sga.tests.common import is_near_now
from edx_sga.utils import is_finalized_submission, utcnow
from edx_sga.utils import get_default_storage, is_finalized_submission, utcnow


@pytest.mark.parametrize(
Expand All @@ -31,3 +34,24 @@ def test_utcnow():
now = utcnow()
assert is_near_now(now)
assert now.tzinfo.zone == pytz.utc.zone

@override_settings(SGA_STORAGE_SETTINGS={
'STORAGE_CLASS': 'storages.backends.s3boto3.S3Boto3Storage',
'STORAGE_KWARGS':
{'bucket_name': 'test', 'location': 'abc/def'}
})
def test_get_default_storage_with_settings_override():
"""
get_default_storage should return an S3Boto3Storage object
"""
storage = get_default_storage()
assert storage.__class__ == S3Boto3Storage
# make sure kwargs are passed through constructor
assert storage.bucket.name == 'test'

def test_get_default_storage_without_settings_override():
"""
get_default_storage should return default_storage object
"""
storage = get_default_storage()
assert storage == default_storage
26 changes: 25 additions & 1 deletion edx_sga/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,34 @@

import pytz
from django.conf import settings
from django.core.files.storage import default_storage
from django.core.files.storage import default_storage as django_default_storage, get_storage_class
from edx_sga.constants import BLOCK_SIZE


def get_default_storage():

Choose a reason for hiding this comment

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

Can you write a simple test for this? That would go in test_utils.py.

Copy link
Author

Choose a reason for hiding this comment

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

Added

"""
Get config for storage from settings, use Django's default_storage if no such settings are defined
"""
# .. setting_name: SGA_STORAGE_SETTINGS
# .. setting_default: {}
# .. setting_description: Specifies the storage class and keyword arguments to use in the constructor

Choose a reason for hiding this comment

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

I think this might be a better description.

Suggested change
# .. setting_description: Specifies the storage class and keyword arguments to use in the constructor
# .. setting_description: Specifies the storage class and keyword arguments to use in the constructor.
# .. Default Storage would be used if this setting is not specified.

Copy link
Author

Choose a reason for hiding this comment

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

Done

# Default storage will be used if this settings in not specified.
# .. setting_example: {
# STORAGE_CLASS: 'storage',
# STORAGE_KWARGS: {}
# }
sga_storage_settings = getattr(settings, "SGA_STORAGE_SETTINGS", None)
Copy link
Member

Choose a reason for hiding this comment

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

Is this introducing a new configuration setting to edx-platform for this xBlock?

Please follow the guidelines in OEP-17 for adding annotations for settings and flags.

In the annotation, please include an example of the data structure for SGA_STORAGE_SETTINGS.

Copy link
Author

Choose a reason for hiding this comment

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

Yes introducing new configuration, added the annotations


if sga_storage_settings:
return get_storage_class(

Choose a reason for hiding this comment

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

I have two things in mind regarding this:

  1. nit: get_storage_class in Django automatically fallbacks to and returns default storage, e.g. FileStorage if the provided args are None or empty. We can use this and easily remove some condition statements and code lines.

    Let me know, If you think that would be less readable.

  2. nit: This can throw ImportError if the provided settings/class name isn't correct.

    • Now we can either 1) let the error bubble up in the logs or 2) Catch it and raise a properly formatted named exception such as ImproperlyConfigured from django.core.exceptions. I'm fine either way so this change is kind of optional, I just wanted to mention it anyway.

Copy link
Author

Choose a reason for hiding this comment

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

  1. Yes, but we still have to call the constructor with the keyword arguments too. It would still require the conditional statement.

  2. Yes it can throw an error, but there is a single source from which this value will come, that's why skipped throw catch in first place.

sga_storage_settings['STORAGE_CLASS']
)(**sga_storage_settings['STORAGE_KWARGS'])

# If settings not defined, use default_storage from Django
return django_default_storage

default_storage = get_default_storage()

def utcnow():
"""
Get current date and time in UTC
Expand Down
1 change: 1 addition & 0 deletions test_requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ coverage==7.2.7
cryptography==41.0.1
ddt==1.6.0
djangorestframework
django-storages
edx-celeryutils==1.2.2
edx-lint==5.2.4
edx-opaque-keys
Expand Down