-
Notifications
You must be signed in to change notification settings - Fork 1
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
Minio #1767
Minio #1767
Changes from all commits
e9159aa
8d7a941
b6df308
5ac246a
1b5d770
e812339
6ab75b3
a119cbf
db6d336
dafee62
947ef93
354b50a
ee466ea
b0a5466
2498e4d
791d762
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
|
||
from django.conf import settings | ||
from django.core.files.uploadhandler import UploadFileException | ||
from django.http import StreamingHttpResponse | ||
|
||
from django_chunk_upload_handlers.s3 import S3FileUploadHandler | ||
|
||
|
@@ -83,3 +84,18 @@ class UploadFailed(UploadFileException): | |
|
||
class UnacceptableMimeTypeError(UploadFailed): | ||
pass | ||
|
||
|
||
def generate_file(result): | ||
for chunk in iter(lambda: result["Body"].read(settings.STREAMING_CHUNK_SIZE), b""): | ||
yield chunk | ||
|
||
|
||
def download_document_from_s3(s3_key, original_file_name): | ||
s3_response = s3_client().get_object(Bucket=settings.AWS_STORAGE_BUCKET_NAME, Key=s3_key) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like a good place for us to flip over to proxying the new download API endpoint There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. This is me initially picking up from a PR that I've had sat dormant for a long time but I definitely see this ending up being the function we switch out to read from the API, I think the only thing that this misses at the moment is the document id, which we'd want to also send to check for file permissions. |
||
_kwargs = {} | ||
if s3_response.get("ContentType"): | ||
_kwargs["content_type"] = s3_response["ContentType"] | ||
response = StreamingHttpResponse(generate_file(s3_response), **_kwargs) | ||
response["Content-Disposition"] = f'attachment; filename="{original_file_name}"' | ||
return response |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,10 +26,10 @@ EXPORTER_TEST_SSO_EMAIL=<<FROM_VAULT>> | |
EXPORTER_TEST_SSO_PASSWORD=<<FROM_VAULT>> | ||
|
||
# AWS | ||
AWS_ACCESS_KEY_ID=<<FROM_VAULT>> | ||
AWS_SECRET_ACCESS_KEY=<<FROM_VAULT>> | ||
AWS_STORAGE_BUCKET_NAME=<<FROM_VAULT>> | ||
AWS_REGION=<<FROM_VAULT>> | ||
AWS_S3_ENDPOINT_URL=http://s3:9000 | ||
AWS_ACCESS_KEY_ID=minio_username | ||
AWS_SECRET_ACCESS_KEY=minio_password | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the expectation here that minio will be present on the API docker-compose only? Just wrapping my head around how this works There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is correct. The API needs to have access to the same bucket so we need to have one single instance of minio running and it makes sense for that to be in the docker-compose of the API. We do the same for redis as well. It's what
is for to bridge between the two docker-compose clusters (or whatever they're called). |
||
AWS_STORAGE_BUCKET_NAME=uploads | ||
|
||
LITE_INTERNAL_HAWK_KEY=LITE_INTERNAL_HAWK_KEY | ||
|
||
|
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.
feels like a bad pattern to have settings.DEBUG in normal control flow.
Is there not any other way ?
not sure what though atm
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.
What is your concern?
The reason behind this was that when I was testing this locally I would sometimes get a generic error that was hard for me to then understand what was going on so when running in debug mode it was better to just return the error that came from the API so it was clear what just happened.
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.
just don't like seeing a different flow based on settings.DEBUG on production code.
no biggie so approving