Allow setting the acl for uploaded files (s3)

[Gitii]

Hi @emgarten,

I made some changes based on your suggestions.
Please take a look at it.
I still need to figure out how to add some tests.

Thanks!

[Gitii]

@emgarten As far as I understand, the aws s3 related tests are all using a real aws account? I do not have an aws account. Can you recommend what tests I can add? Thanks!

[emgarten]

@emgarten As far as I understand, the aws s3 related tests are all using a real aws account? I do not have an aws account. Can you recommend what tests I can add? Thanks!

Take this test that creates and bucket and pushes, copy it, and update it so it passes a public ACL which will effectively be the same, but still hit your code paths.

Sleet/test/Sleet.AmazonS3.Tests/BasicTests.cs

Line 75 in 610cb10

public async Task GivenAStorageAccountWithNoContainerVerifyPushSucceeds()

It won't run automatically as part of your PR validation, but I'll kick the regular CI validation off and verify it against a real s3 account.

Are the changes working for you with scaleway?

[Gitii]

@emgarten Yes, it works 🙂!
That said, I could not test the bucket creation route because my bucket already exists.
As far as I can tell, bucket creation is part of the test suite, so this should be covered by the automated tests.

Right now, if the acl is not found (not a canned acl) then the acl will not be set. Is this behaviour ok for you or should that throw an error or log a warning?

[emgarten]

Right now, if the acl is not found (not a canned acl) then the acl will not be set. Is this behaviour ok for you or should that throw an error or log a warning?

Make it fail fast and throw when it can't find the canned acl.

If someone was trying to use the acl to make it private this would be something they would want to fail on. Also, it is better to make it strict to start and default it later. The other way around would be a more breaking change.

[Gitii]

@emgarten I bug a bit through the aws s3 sdk and realized, that there isn't an easy way to check if the acl is actually valid (or predefined). The FindValue method constructs a new "constant value" when no predefined is found 🤷‍♂️. I do not want to use reflection because that will break AOT.
That means that there will be a run time exception during upload. That exception is not thrown by my code, but from the aws sdk (with an invalid acl):

Reading feed [...]/
Reading feed
Replace existing package: Hardware.Info 200.0.0
Replace existing package: Hardware.Info.Aot 200.0.0
Processing feed changes
Committing changes to [...]/
Failed to upload '[...]/flatcontainer/hardware.info.aot/200.0.0/hardware.info.aot.nuspec'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info.aot/200.0.0/hardware.info.aot.200.0.0.nupkg'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info/200.0.0/hardware.info.200.0.0.nupkg'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info/200.0.0/hardware.info.nuspec'. Retrying.
Failed to upload '[...]/registration/hardware.info.aot/200.0.0.json'. Retrying.
Failed to upload '[...]/registration/hardware.info/200.0.0.json'. Retrying.
Failed to upload '[...]/sleet.packageindex.json'. Retrying.
Failed to upload '[...]/registration/hardware.info.aot/index.json'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info.aot/200.0.0/hardware.info.aot.200.0.0.nupkg'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info.aot/200.0.0/hardware.info.aot.nuspec'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info/200.0.0/hardware.info.200.0.0.nupkg'. Retrying.
Failed to upload '[...]/registration/hardware.info.aot/200.0.0.json'. Retrying.
Failed to upload '[...]/sleet.packageindex.json'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info/200.0.0/hardware.info.nuspec'. Retrying.
Failed to upload '[...]/registration/hardware.info/200.0.0.json'. Retrying.
Failed to upload '[...]/registration/hardware.info.aot/index.json'. Retrying.
Failed to upload '[...]/registration/hardware.info.aot/200.0.0.json'. Retrying.
Failed to upload '[...]/registration/hardware.info/200.0.0.json'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info/200.0.0/hardware.info.200.0.0.nupkg'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info.aot/200.0.0/hardware.info.aot.200.0.0.nupkg'. Retrying.
Failed to upload '[...]/sleet.packageindex.json'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info.aot/200.0.0/hardware.info.aot.nuspec'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info/200.0.0/hardware.info.nuspec'. Retrying.
Failed to upload '[...]/registration/hardware.info.aot/index.json'. Retrying.
Failed to upload '[...]/registration/hardware.info/200.0.0.json'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info/200.0.0/hardware.info.nuspec'. Retrying.
Failed to upload '[...]/sleet.packageindex.json'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info/200.0.0/hardware.info.200.0.0.nupkg'. Retrying.
Failed to upload '[...]/registration/hardware.info.aot/200.0.0.json'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info.aot/200.0.0/hardware.info.aot.200.0.0.nupkg'. Retrying.
Failed to upload '[...]/registration/hardware.info.aot/index.json'. Retrying.
Failed to upload '[...]/flatcontainer/hardware.info.aot/200.0.0/hardware.info.aot.nuspec'. Retrying.
[Amazon.S3.AmazonS3Exception] Invalid argument.

At the end, a single Invalid argument. exception is shown. Is there a way to provide more details to the user? The exception object might contain more error details that can be useful for the user.

[emgarten]

Does it fail on upload if the acl is invalid?

I would avoid any reflection like that.

[Gitii]

Does it fail on upload if the acl is invalid?

I would avoid any reflection like that.

Yes, it fails on upload (like shown in the code block).
The issue is that I cannot determine whether the acl is valid until the actual upload (so no fast fail).

[emgarten]

The issue is that I cannot determine whether the acl is valid until the actual upload (so no fast fail).

That's a good behavior here. I would still consider that failing fast, it just doesn't have up front validation.

[Gitii]

@emgarten Ok, if you are fine with the current behaviour then that's fine for me, too. I made this PR ready for final review.
If there is anything else, just say so and I will look into it.

[emgarten]

The new functional test is passing along with all the others. Great work on this useful feature 🏆