[static-website] fix: stop silent fail on s3 deleteObjects

[fargito]

Hello and thanks a lot for this awesome project!

When using the static-website construct on my project, I realized that I had forgotten the s3:deleteObject IAM permission of my deployment policy. Therefore, the s3 files synced with lift were never deleted. However, no error was thrown and the deployement failed silently.

This silent fail is due to a particularity in the S3 deleteObjects SDK and the corresponding HTTP API. Since this API can encounter partial failures, the call never fails but returns the errors in a Errors key.

I think that Lift should explicitely fail when the Errors key is defined and the list of errors is not empty. In this case, the proposed error is the following:

Deleting index.html
{
  Key: 'index.html',
  Code: 'AccessDenied',
  Message: 'Access Denied'
}
 
 Serverless Error ----------------------------------------
 
  Unable to delete files
 
  Get Support --------------------------------------------
     Docs:          docs.serverless.com
     Bugs:          github.com/serverless/serverless/issues
     Issues:        forum.serverless.com
 
  Your Environment Information ---------------------------
     Operating System:          linux
     Node Version:              14.16.1
     Framework Version:         2.61.0 (local)
     Plugin Version:            5.4.5
     SDK Version:               4.3.0
     Components Version:        3.17.1
 

[t-richard]

The only thing I'm worried about is: what are the other cases a delete might fail ?

The current state is not ideal but is it better than a randomly failing deploy ? Would a warning be enough (probably not) ?

Would love to hear about what others think.

[t-richard]

Also kinda linked to #111 because this originated from misconfigured deploy credentials

[fargito]

Hello @t-richard and thanks for the review.

The s3Sync function is used in the serverSideWebsite and staticWebsite constructs, so this change would only affect them.

Maybe a warning could be enough, but I would argue that the deployment failure would only happen once (most probably at the second deploy). The alternative with the silent fail is that the list of files to delete grows at each deploy and the deployment fails at one point because the XML body sent to the s3 api is too large.

Concerning the explicit error, I logged the error returned by the S3 api (as in the description of the PR).

I saw that the tests were failing, I will try and fix them :)

[t-richard]

@fargito the test failure is unrelated to your change 🙂
See #116

[fargito]

Ah ok ! Nonetheless I think my change broke the s3 sync test, so I fixed it

[t-richard]

I also think failing is somewhat better than a warning because it could lead to unnecessary objects being stored and billed which could become a significant cost over time.

Concerning the explicit error, I logged the error returned by the S3 api (as in the description of the PR).

I know you logged the errors but what I was suggesting is to add instructions on how to solve the issue

Trying to give users some guidance on what may be wrong will help to get a good developer experience and reduce frustration (and IAM is a very good frustration generator 😅).

See some examples:

• https://github.com/getlift/lift/blob/master/src/constructs/aws/StaticWebsite.ts#L187-L190
• https://github.com/getlift/lift/blob/master/src/constructs/aws/StaticWebsite.ts#L86-L90

[fargito]

Great ! I'll do that !

[fargito]

@t-richard done :)

[mnapoli]

Thank you, I added minor suggestions for wording.

[fargito]

@mnapoli thanks a lot, I applied your suggestions

[t-richard]

LGTM

[mnapoli]

master is green again, you should be able to rebase or merge it into your branch, and then I think we'll be good to merge 💪

[fargito]

Hello @mnapoli I think you can relaunch the workflow

[fargito]

@mnapoli actually it is also used by the "server-side-website" contruct so I updated the tests and added it to the error message

[mnapoli]

Thank you!