-
Notifications
You must be signed in to change notification settings - Fork 255
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
cp,pipe: don't omit some of the metadata flags #658
Conversation
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've missed these on the review, sorry. While you're at it, what do you think about adding test cases for each metadata flag if possible? I think we could have catch this early with failing tests.
I'm afraid that some of the metadata flags( |
This PR adds support for the missing metadata flags, except for the |
@seruman, 5394a50 covers all flags except the |
e2e/cp_test.go
Outdated
// NOTE: This test needs an implementation of the | ||
// `ACL` feature in gofakes3. | ||
func TestCopySingleFileToS3WithACLFlag(t *testing.T) { | ||
t.Skip() |
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.
Would you like to discuss a strategy on how should we test that feature ? Currently, gofakes3 does not implement ACL related features, we either can work on that feature for it or prepare an "integration" test where we run the test against a minio or localstack instance, really any s3 compatible storage service where it supports ACL features.
If it would not require much test setup i'm on the side of adding it. There's already an integration test for select
which only runs if an endpoint has given.
It is not a blocker though IMHO, if we can assert that it fixed -manually- we may add the automated tests later in some another PR.
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.
c177bb2, unfortunately minio
does not support ACL
features. We should add this test to the backlog.
#657 mentioned that
acl
flag was broken inv2.2.1
. After looking at the issue, it was discovered that not only theacl
flag was broken, there are couple of more flags that were being omitted during the mentioned commands, which all of them caused by this faulty PR.Resolves #657.