cp,pipe: don't omit some of the metadata flags - #658
Conversation
seruman
left a comment
seruman
left a comment
There was a problem hiding this comment.
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 |
| // 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.
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.
c177bb2, unfortunately minio does not support ACL features. We should add this test to the backlog.
4 participants
#657 mentioned that
aclflag was broken inv2.2.1. After looking at the issue, it was discovered that not only theaclflag 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.