-
Notifications
You must be signed in to change notification settings - Fork 239
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
add cache-control and expires options #318
add cache-control and expires options #318
Conversation
Signed-off-by: tombokombo <[email protected]>
@aykutfarsak @ilkinulas guys could you please review, thanks. |
@tombokombo there has been a lot of development in s5cmd these days, this PR is waiting in the queue, I will take a look at it soon. |
Signed-off-by: tombokombo <[email protected]>
With invalid expires date value, aws cli returns an error:
So I am not sure if we should ignore the invalid expire date error. Maybe we can increase the log level to What do you think @ilkinulas? |
Good catch @aykutfarsak 👍 Let's add a validation for |
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.
With invalid expires date value, aws cli returns an error:
➜ aws s3 cp file s3://bucket/prefix --expires "2024-13-01T20:30:00Z" upload failed: ./Makefile to s3://bucket/prefix/file Parameter validation failed: Invalid type for parameter Expires, value: 2024-13-01T20:30:00Z, type: <class 'str'>, valid types: <class 'datetime.datetime'>, timestamp-string ➜ echo $? 1
So I am not sure if we should ignore the invalid expire date error. Maybe we can increase the log level to
error
or return the error immediately.
What do you think @ilkinulas?Good catch @aykutfarsak Let's add a validation for
--expires
parameter
Actually, date string validation is already in place, I just choose to print debug message in case of invalid date, so maybe log.Error instead of log.Debug would by enough?
storage/s3.go
Outdated
if err == nil { | ||
input.Expires = aws.Time(t) | ||
} else { | ||
msg := log.DebugMessage{Err: err.Error()} |
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.
I was trying invalid time formats to see s3 behavior, here are my findings:
- if we upload a new object s3 ignores invalid
Expires
metadata. - if we override an existing s3 objects (which has a valid
Expires
metadata) with an invalid expires value, s3 deletes the current object's Expires metadata.
So, it would be better to return the error and do not cp/mv any objects in case of unparsable --expires values.
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.
ok, done
Signed-off-by: tombokombo <[email protected]>
Co-authored-by: Aykut Farsak <[email protected]>
Co-authored-by: Aykut Farsak <[email protected]>
@tombokombo thank you for the contribution. |
Hello, this PR add support for setting cache-control and expires header to s3 object, same as aws cli