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[file]: add s3 functionality #659

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

m-mattia-m
Copy link

  • Configure S3 endpoint, bucket, base path, credentials
  • Upload file to S3
  • Get file from S3
  • delete file from S3

PS: Please bear in mind that I've never used NestJS before, so I'm not sure if everything is best practice, but it works. Don't hesitate to tell me what I should change or update.

@stonith404
Copy link
Owner

Thank you @m-mattia-m :) I’ll try to review the PR as soon as I can. Since I’ll be on vacation next week and have a lot going on, it may take me some time to review and test it fully.

Copy link
Owner

@stonith404 stonith404 left a comment

Choose a reason for hiding this comment

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

Sorry for my late response, I have much going on.

I noticed an issue with how chunked file uploads are being handled. Currently, only the first chunk of a file is successfully uploaded to S3. Pingvin Share relies on chunked uploads, where the frontend splits a file into multiple parts and uploads each part sequentially.

With your current implementation, only the initial chunk is uploaded. You can verify this by trying to upload a file larger than the chunk size (default is 10 MB).

@stonith404
Copy link
Owner

Additionally, I would suggest to create a separate S3 file service as the methods in the file service get a bit too complicated when both S3 and local file storage are handled.

@m-mattia-m
Copy link
Author

No worries :)

I can see what you mean by "a bit too complicated" :D. I will create a separate service for the S3 functionality. Yes you are right, large files are failing during the upload. I've never tested it with large files which I should have done. I will implement this feature in a few weeks but I have also much going on :).

@stonith404
Copy link
Owner

@m-mattia-m Sure, take your time :)

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.

2 participants