Closed subho007 closed 1 year ago
thank you very much for the PR, @subho007 - looks like a very high quality set of changes. i'll review it ASAP
@theriverman I apologise for not being clear about the issue.
The old implementation takes the hardcoded value written in settings.py
to generate the URL, which unfortunately breaks if you use AWS S3 as an endpoint, since AWS S3 endpoint is a location aware endpoint.
Which means, if my S3 bucket is hosted in ap-south-1
region, the GET url should be s3.ap-south-1.amazonaws.com
and not s3.amazonaws.com
. This is handled in minio-py
implementation to get the generated location aware URL for GET
request. This is also true if you have S3 acceleration on, and other S3 compliant implementation of aliyuncs
The reference to this is here: https://github.com/minio/minio-py/blob/master/minio/helpers.py#L426-L458
The region specific parameters are added here: https://github.com/minio/minio-py/blob/master/minio/helpers.py#L535-L572
And for S3 accelerated URL parameters are added here: https://github.com/minio/minio-py/blob/master/minio/helpers.py#L555-L578
Common hosted minio solution may not have any affect, but projects which uses minio instead of boto
requires the URLs to be build in specific way with region aware URLs.
Hence, I am using the minio-client
itself to get the generated URLs instead of relying on the value in settings.py
_Note: You may say why don't I put in MINIO_HOST=s3.ap-south-1.amazonaws.com
? Unfortunately, Minio only understands s3.amazonaws.com
and then reconstructs the URLs based on Region/Accelerate properties_
@subho007 thank you very much for the changes and for the brief explanation of your request. i've missed that initial detail you aim to support communication towards AWS S3 too. When I was implementing this package I was focusing solely on MinIO, but I don't have anything against this rather minor change.
@subho007 v3.4.0 pushed to PyPI. thanks a lot for your contribution!
Similar PR was raised at https://github.com/theriverman/django-minio-backend/pull/38 but unfortunately it was not a complete implementation.
The following were changed and added:
MINIO_REGION
value from settings or set it tous-east-1
which is the default value (Minio itself uses that as a default value)README.md
is updatedThis Fixes few problems:
I have already tested it out with local/hosted instance of minio and with AWS S3 service. Forked version of this is published in my PyPI: https://pypi.org/project/ak-django-minio-backend/ (I'll delete as soon as this gets merged, tested, approved released)