add S3ForcePathStyle support for s3 binding #1360
Conversation
|
|
| s.uploader = s3manager.NewUploader(session) | ||
| s.s3Client = s3.New(session, cfg) | ||
| s.downloader = s3manager.NewDownloaderWithClient(s.s3Client) | ||
| s.uploader = s3manager.NewUploaderWithClient(s.s3Client) |
There was a problem hiding this comment.
is there a reason to change to NewDownloaderWithClient from NewDownloader ?
There was a problem hiding this comment.
S3ForcePathStyle only can be set with s3 client or session. I couldn't find a way to set it up in NewDownloader.
There was a problem hiding this comment.
S3ForcePathStyleonly can be set with s3 client or session. I couldn't find a way to set it up inNewDownloader.
@rainfd if it can be set in the session why not set it there, since session is already being used?
There was a problem hiding this comment.
@mukundansundar session get by GetClient in authentication/aws/aws.go, setting ForcePathStyle here is also ok, like
func GetClient(accessKey string, secretKey string, sessionToken string, region string, endpoint string, forcePathStyle bool) (*session.Session, error) {
but GetClient is used by other aws componment. Should I add modify here?
Or add more options to GetClient, like
func GetClient(accessKey string, secretKey string, sessionToken string, region string, endpoint string, opts...Option) (*session.Session, error)
There was a problem hiding this comment.
Thanks fir explaining that ... And no no need to make change in the common method.
Can you add a docs PR for this and also unit tests to make sure that the metadata is parsed correctly?
There was a problem hiding this comment.
@mukundansundar test and docs have already added.
dapr/docs#2054
|
@mukundansundar please see comment and re-review. |
|
@rainfd we now need to sign off our commits, please see here: https://github.com/src-d/guide/blob/master/developer-community/fix-DCO.md#how-to-add-sign-offs-retroactively. |
Signed-off-by: rainfd <[email protected]>
rainfd
force-pushed
the
s3-binding-path-style-access
branch
from
d737ff8 to
3b36d0e
Compare
|
@yaron2 comment has been modified. |
Signed-off-by: rainfd <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1360 +/- ##
==========================================
- Coverage 35.02% 34.96% -0.07%
==========================================
Files 148 149 +1
Lines 12825 12931 +106
==========================================
+ Hits 4492 4521 +29
- Misses 7853 7925 +72
- Partials 480 485 +5
Continue to review full report at Codecov.
|
|
@rainfd can you please open a docs PR that updates the metadata? |
dapr/docs#2054 What's missing? |
Oh didn't see that, all good. |
* add S3ForcePathStyle support for s3 binding Signed-off-by: rainfd <[email protected]> * add S3ForcePathStyle unit test Signed-off-by: rainfd <[email protected]> Signed-off-by: jigargandhi <[email protected]>
Description
Add S3ForcePathStyle support for S3 API compatible products, like Minio.
Issue reference
#1322
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: