Make canned ACL optional #20
Conversation
So, in this case, no acl will be set to the object, causing it to inherit the bucket rules.
There was a problem hiding this comment.
This is a good idea and the implementation is really simple 👌
I think this library isn't used anywhere except the sbt plugin 😅 So I'm more concerned about keeping the interface nice (see the comment below) and not afraid of breaking things here (also it's still not v1.x).
Also, if you could write a simple test for this case, it would be much appreciated. Smth like having a mock-bucket with a preset ACL and then putting an object (without ACL parameter) and checking that the resulting ACL is the one of the bucket.
|
|
||
| if (acl != null){ | ||
| request.setCannedAcl(acl); | ||
| } |
There was a problem hiding this comment.
This check looks good on its own, but I don't like the idea of passing null when you want to leave ACL unset.
How about changing its type in the S3Repository constructor to Optional<CannedAccessControlList> and adjusting the other constructors accordingly?
There was a problem hiding this comment.
I second that. I'd even encourage making that change comprehensively wherever applicable.
There was a problem hiding this comment.
While I totally agree with that, I avoided breaking the contract with what is currently stablished.
This seemed the minimum effort to make it happen.
Nonetheless, makes sense. I'll fix this.
notes/0.12.1.markdown
Outdated
| @@ -0,0 +1,2 @@ | |||
| * Allowed `CannedAccessControlList` to be null, in which case no specific ACL is set for the object; | |||
There was a problem hiding this comment.
Thanks for keeping changelog 👍
Rename the file to changelog.md, please. Then the release pipeline will pick it up and rename to the corresponding version. I think it will be 0.13.0 rather than 0.12.1, because it's not a bug fix, but a feature 😆
There was a problem hiding this comment.
Great, makes sense. I'll change that as well.
Most of the requested changes are in place, except for the test. I'm not really sure we can test the result ACL is the one of the bucket since, AFAIK, this happens on server side, when you send a request with no ACL attached. |
| @@ -94,6 +95,26 @@ public S3Repository( | |||
| .withRegion(region.toString()) | |||
| .build(), | |||
| overwrite, | |||
| Optional.of(acl), | |||
There was a problem hiding this comment.
Just in case, change this to .ofNullable
Makes sense. I forgot that |
|
I did some checks locally and the S3Resolver was missing. Changed there as well. Thanks for accepting the issue and both PRs. |
|
Great! Thanks @hkupty! |
So, in this case, no acl will be set to the object,
causing it to inherit the bucket rules.
This won't break anything, but allow ohnosequences/sbt-s3-resolver#54 to be solved.