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.

Sign up for GitHub

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

Jump to bottom

fix STORAGE_KWARGS #70

Closed
wants to merge 1 commit into from
Closed

fix STORAGE_KWARGS #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 2 deletions tutors3/patches/openedx-common-settings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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("/")}
Copy link
Contributor

@fghaas fghaas Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@fghaas fghaas Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

@regisb sure

Copy link

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link

@regisb regisb Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

GRADES_DOWNLOAD["STORAGE_KWARGS"]["bucket_name"] = "{{ S3_GRADE_BUCKET }}"

ORA2_FILEUPLOAD_BACKEND = "s3"
FILE_UPLOAD_STORAGE_BUCKET_NAME = "{{ S3_FILE_UPLOAD_BUCKET }}"
Expand Down
5 changes: 1 addition & 4 deletions tutors3/patches/openedx-lms-common-settings
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,9 @@
PROFILE_IMAGE_BACKEND = {
"class": DEFAULT_FILE_STORAGE,
"options": {
"bucket": "{{ S3_PROFILE_IMAGE_BUCKET }}",
"bucket_name": "{{ S3_PROFILE_IMAGE_BUCKET }}",
"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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that makes sense. :)

},
}
{% endif %}
5 changes: 5 additions & 0 deletions tutors3/patches/openedx-lms-development-settings
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{% if S3_PROFILE_IMAGE_BUCKET %}
# A non empty base_url is necessary in development as it will be used
# to build a static url for the profile images.
PROFILE_IMAGE_BACKEND["options"]["base_url"] = "dummyprofileimagebaseurl"
{% endif %}
Loading