-
Notifications
You must be signed in to change notification settings - Fork 14
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: [AXM-749] Implement s3 storage supporting #2581
feat: [AXM-749] Implement s3 storage supporting #2581
Conversation
@@ -80,33 +81,20 @@ def create_subdirectories_for_asset(file_path): | |||
|
|||
def remove_old_files(xblock): | |||
""" | |||
Removes the 'assets' directory, 'index.html', and 'offline_content.zip' files | |||
in the specified base path directory. | |||
Removes the old 'offline_content.zip' files from media storage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file isn't called offline_content anymore, please update docstring with describing this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -18,85 +19,95 @@ | |||
log = logging.getLogger(__name__) | |||
|
|||
|
|||
def generate_offline_content(xblock, html_data): | |||
class SaveOfflineContentToStorage: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class name should be changed to be in OOP paradigm. The name is stating an action, and suitable for function, but not for class.
Also the class should be moved outside of utils file into a file similar to class name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. If you don't mind, I'll take that class name: OfflineContentStorageManager
& storage_management.py file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to name the class something like OfflineContentGenerator
instead of renaming the main method to generate
.
@@ -80,33 +81,20 @@ def create_subdirectories_for_asset(file_path): | |||
|
|||
def remove_old_files(xblock): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this function name to clean_outdated_xblock_files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually asking to make Assets manager as class, and not just utils.py
Maybe it's worth to combine the class you've created and some functions from this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to push the latest changes. Based on this, it will be possible to decide what can be refactored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address comments and change the files structure as suggested, the tests can be created in the follow up task
ea68480
to
c4b669a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
Adjust file upload engine to work with s3
We adjust developed code to work natively with both local media storage and s3 or other django storages backends.
More info there: https://raccoongang.atlassian.net/wiki/spaces/tech/pages/4236705816/Offline+mode+with+Minio+and+s3+bucket
YouTrack
https://youtrack.raccoongang.com/issue/AXM-749/FollowUp-Adjust-file-upload-engine-to-work-with-s3