Put object options #4219
Put object options #4219
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4219 +/- ##
=======================================
Coverage 38.27% 38.28%
=======================================
Files 776 776
Lines 34323 34328 +5
Branches 5424 5424
=======================================
+ Hits 13137 13141 +4
- Misses 20007 20008 +1
Partials 1179 1179
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@sir-sigurd What else can I check besides that the args get passed to |
There was a problem hiding this comment.
the general direction seems ok, tho it looks like you forgot to actually use put_options in one place (data_transfer._upload_file).
plus some formatting nitpicks.
also, i can't comment on the completeness of this change (i.e. whether you covered all the places where an object may be written o s3)
| @@ -301,7 +301,7 @@ def _copy_local_file(ctx: WorkerContext, size: int, src_path: str, dest_path: st | |||
| ctx.done(PhysicalKey.from_path(dest_path), None) | |||
|
|
|||
|
|
|||
| def _upload_file(ctx: WorkerContext, size: int, src_path: str, dest_bucket: str, dest_key: str): | |||
| def _upload_file(ctx: WorkerContext, size: int, src_path: str, dest_bucket: str, dest_key: str, put_options=None): | |||
There was a problem hiding this comment.
i don't see where this argument is used.
also, this case is somewhat less straightforward, because there may be (and usually are) operations other than PutObject: create / update MPU, upload part -- what's the plan regarding those?
| bucket = Bucket('s3://test-bucket') | ||
| bucket.put_file(key='README.md', path='./README') # put local file to bucket | ||
| bucket.put_file(key='README.md', path='./README', put_options=opts) | ||
| # put local file to bucket |
There was a problem hiding this comment.
the comment should be either above or on the line it refers to
There was a problem hiding this comment.
- let's add type hints for new parameter (and don't specify type in docstring)
- use case is not quite clear, instead of specifying options per operation it might be sufficient and more ergonomic to set them globally or other way round use case might need a way to specify options per object
Co-authored-by: Alexei Mochalov <[email protected]>
|
NOTE: This is too much for me to digest. Unless someone wants to take it on tomorrow, I'll target it for 12/20. |
Update
pushet al to take optional arguments to pass to PutObject.