fix STORAGE_KWARGS #70

CefBoud · 2024-02-01T10:21:55Z

Description

Quince release came and with it a new django-storage version. New versions of django-storage have stricter checks for STORAGE_KWARGS. It is no longer allowed to have options from other storage classes (notably, base_url which is irrelevant to the S3 storage class but is currently always set in Tutor as default).

The bucket option has also been deprecated and renamed to bucket_name.

These changes cause errors when deploying the Quince release with the S3 plugin. This PR aims to address that.

Instructions to reproduce the error

Launch Tutor with Quince version and with s3 plugin.
In the LMS, head to Data Downloads section under the Instrutor tab. Click the Download profile information as a CSV.
The download will fail and LMS logs will show django.core.exceptions.ImproperlyConfigured: Invalid setting 'base_url' for S3Storage .

Helpful

To access GRADES_DOWNLOAD storage settings:

# in LMS shell
from django.conf import settings
getattr(settings, 'GRADES_DOWNLOAD').get('STORAGE_KWARGS')

fghaas

Thanks for the PR! I have a couple of questions; please see the inline comments.

fghaas · 2024-02-01T10:44:49Z

tutors3/patches/openedx-common-settings

@@ -1,8 +1,8 @@
 DEFAULT_FILE_STORAGE = "storages.backends.s3boto3.S3Boto3Storage"
 VIDEO_IMAGE_SETTINGS["STORAGE_KWARGS"]["location"] = VIDEO_IMAGE_SETTINGS["STORAGE_KWARGS"]["location"].lstrip("/")
 VIDEO_TRANSCRIPTS_SETTINGS["STORAGE_KWARGS"]["location"] = VIDEO_TRANSCRIPTS_SETTINGS["STORAGE_KWARGS"]["location"].lstrip("/")
-GRADES_DOWNLOAD["STORAGE_KWARGS"]["location"] = GRADES_DOWNLOAD["STORAGE_KWARGS"]["location"].lstrip("/")
-GRADES_DOWNLOAD["STORAGE_KWARGS"]["bucket"] = "{{ S3_GRADE_BUCKET }}"
+GRADES_DOWNLOAD["STORAGE_KWARGS"] = {"location": GRADES_DOWNLOAD["STORAGE_KWARGS"]["location"].lstrip("/")}


I presume you are overriding the whole dictionary here, rather than just overriding the value of location, in order to remove the problematic base_url key. Is that correct?

If so, I'm curious as to why the tutor-minio plugin does not fail in the same way. Because it appears to be doing the same thing that we do here, namely only override the location key rather than the whole dictionary:

https://github.com/overhangio/tutor-minio/blob/ac817717c92ca006e7752efd1c034a6175f3c948/tutorminio/patches/openedx-common-settings

I presume you are overriding the whole dictionary here, rather than just overriding the value of location, in order to remove the problematic base_url key. Is that correct?

Exactly.

If so, I'm curious way why the tutor-minio plugin does not fail in the same way. Because it appears to be doing the same thing that we do here, namely only override the location key rather than the whole dictionary:

I am not really familiar with the tutor-minior plugin. But please note that this problem occurs in Quince (3 months-ish ago?) and the last commit from that file dates back to 7 months ago. So they might have the same issue. I am not sure.

No problem. @regisb, just checking, does this ring a bell?

I ask because if setting base_url breaks both tutor-minio and tutor-contrib-s3, then it should probably be fixed in both — or else, maybe we want to remove lines such as this from Tutor Core:

https://github.com/overhangio/tutor/blob/2924b92ceab8aca550ec5ecae559a367fb1b713a/tutor/templates/apps/openedx/settings/partials/common_all.py#L122

What do you think?

Interesting. Thanks for the heads up Florian. @FahadKhalid210 could you please have a look and see if you can reproduce the issue described here?

@regisb sure

@regisb The issue has been replicated locally by initiating Tutor with the Quince version and the s3 plugin. Additionally, the problem was verified using Minio, and it was reproduced in that environment as well.

Thanks @FahadKhalid210!

@regisb, at your convenience could you please advise whether the base_url setting is even still necessary in Tutor Core? Because if it is not, then I'd suggest we remove it from there, and remove any reference to it from the s3 and minio plugins.

I can't figure remember the reason I initially introduced a base_url setting in GRADES_DOWNLOAD. The most information I can find is from this commit, five years ago: overhangio/tutor@abfbf1c
Indeed, we should remove this base_url attribute if it's causing errors.
@FahadKhalid210 can you please handle this? We should verify that grades download keeps working both with and wihout minio.

@FahadKhalid210: When you create a PR for such a change in either tutor or tutor-minio, would you mind adding a link to this issue in the discussion thread, so we get a backlink here and can coordinate the fix?

Sure thing. Here it is: overhangio/tutor#999

EDIT: actually, here's the relevant PR overhangio/tutor-minio#35

fghaas · 2024-02-01T10:47:34Z

tutors3/patches/openedx-lms-common-settings

        "location": PROFILE_IMAGE_BACKEND["options"]["location"].lstrip("/"),
        {% if S3_PROFILE_IMAGE_CUSTOM_DOMAIN %}"custom_domain": "{{ S3_PROFILE_IMAGE_CUSTOM_DOMAIN }}",{% endif %}
-        # A non empty base_url is necessary in development as it will be used
-        # to build a static url for the profile images.
-        "base_url": "dummyprofileimagebaseurl",


I am not sure I understand why it's necessary to remove this from here, and then reindroduce it in openedx-lms-development-settings. Can you elaborate please?

Sure. The comment indicates that this is necessary for development and I thought it was cleaner to move this to the dev-specific patch.
This didn't cause an issue for me but I moved it for peace of mind.

Okay, that makes sense. :)

Quince release came and with it a new django-storage version. New versions of django-storage have stricter checks for STORAGE_KWARGS. It is no longer allowed to have options from other storage classes. Notably, base_url which is irrelevant to the S3 storage class. The bucket option has also been deprecated and renamed to bucket_name.

fghaas · 2024-02-20T13:55:15Z

tutors3/patches/openedx-lms-development-settings

@CefBoud If I read overhangio/tutor#999 and overhangio/tutor-minio#35 correctly, then I would assume that we don't need to mess with base_url at all anymore. So I'd suggest we drop it from openedx-lms-common-settings, but we also don't introduce an openedx-lms-development-settings patch. Would you agree?

CefBoud · 2024-02-20T17:15:24Z

Please check #71.

fghaas reviewed Feb 1, 2024

View reviewed changes

CefBoud force-pushed the fix-grade-storage-kwargs branch from 285d227 to 5d4bf9e Compare February 1, 2024 12:39

CefBoud force-pushed the fix-grade-storage-kwargs branch from 5d4bf9e to fe7c758 Compare February 1, 2024 14:58

fghaas mentioned this pull request Feb 4, 2024

Bump tutor requirements to allow v17 #66

Merged

regisb mentioned this pull request Feb 9, 2024

Fix: Remove base_url from grades_download overhangio/tutor#999

Closed

FahadKhalid210 mentioned this pull request Feb 12, 2024

fix: fixed base_url issue overhangio/tutor-minio#35

Merged

fghaas reviewed Feb 20, 2024

View reviewed changes

fghaas mentioned this pull request Feb 20, 2024

fix: Update django-storages options #71

Merged

CefBoud closed this Feb 20, 2024

fghaas mentioned this pull request Jun 14, 2024

chore: bump supported tutor versions to work on tutor 18 for redwood #76

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix STORAGE_KWARGS #70

fix STORAGE_KWARGS #70

CefBoud commented Feb 1, 2024

fghaas left a comment

fghaas Feb 1, 2024 •

edited

Loading

CefBoud Feb 1, 2024

fghaas Feb 1, 2024 •

edited

Loading

regisb Feb 1, 2024

FahadKhalid210 Feb 2, 2024

FahadKhalid210 Feb 2, 2024

fghaas Feb 2, 2024

regisb Feb 5, 2024

fghaas Feb 9, 2024

regisb Feb 9, 2024 •

edited

Loading

fghaas Feb 1, 2024

CefBoud Feb 1, 2024

fghaas Feb 2, 2024

fghaas Feb 20, 2024

CefBoud commented Feb 20, 2024

fix STORAGE_KWARGS #70

fix STORAGE_KWARGS #70

Conversation

CefBoud commented Feb 1, 2024

Description

Instructions to reproduce the error

Helpful

fghaas left a comment

Choose a reason for hiding this comment

fghaas Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fghaas Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regisb Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CefBoud commented Feb 20, 2024

fghaas Feb 1, 2024 •

edited

Loading

fghaas Feb 1, 2024 •

edited

Loading

regisb Feb 9, 2024 •

edited

Loading