Add config to deploymentBucket to support S3 bucket versioning #9912

mars-lan · 2021-09-04T13:01:48Z

Closes: #5797 (again)

The original issue asked to support both encryption & versioning for the S3 deployment bucket and #5800 only addressed the first former. This PR adds an additional versioning config to support the latter.

codecov · 2021-09-06T09:40:32Z

Codecov Report

Merging #9912 (25e8ad1) into master (db85602) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #9912      +/-   ##
==========================================
+ Coverage   85.38%   85.41%   +0.02%     
==========================================
  Files         333      333              
  Lines       13571    13586      +15     
==========================================
+ Hits        11588    11604      +16     
+ Misses       1983     1982       -1

Impacted Files	Coverage Δ
lib/plugins/aws/provider.js	`94.27% <ø> (+0.30%)`	⬆️
...ib/plugins/aws/package/lib/generateCoreTemplate.js	`95.55% <100.00%> (+0.20%)`	⬆️
lib/plugins/aws/remove/lib/bucket.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db85602...25e8ad1. Read the comment docs.

pgrzesik

Thanks @mars-lan - great call 👍 I have only minor comments, please let me know what do you think.

Additionally, it would be good to add a note about this property here: https://github.com/serverless/serverless/blob/master/docs/providers/aws/guide/deploying.md#tips

test/unit/lib/plugins/aws/package/lib/generateCoreTemplate.test.js

pgrzesik · 2021-09-06T14:19:39Z

Thanks @mars-lan 🙇 Before moving forward, I will test it out locally as I'm not 100% sure if bucket with versioning enabled will behave correctly when it comes to clearing previous deployment artifacts as well as during whole service teardown. Did you test that by any chance?

mars-lan · 2021-09-06T18:17:25Z

@pgrzesik you're correct. The current AWS bucket cleanup code is not version aware and will only delete the latest version when serverless remove is called. Will add support to remove all versions to this PR.

mars-lan · 2021-09-07T00:21:33Z

Thanks @mars-lan 🙇 Before moving forward, I will test it out locally as I'm not 100% sure if bucket with versioning enabled will behave correctly when it comes to clearing previous deployment artifacts as well as during whole service teardown. Did you test that by any chance?

Verified with the latest change,

Deploy with versioning set to false
Deploy with versioning set to true
Modify code, deploy again to create a new directory in bucket
Run serverless remove to teardown the stack

mars-lan · 2021-09-08T18:22:06Z

Ping @pgrzesik ?

pgrzesik · 2021-09-08T20:00:16Z

Thanks for the update @mars-lan 🙇 I'm a bit swamped with other things at the moment but I will try my best to review and test it before the end of the week.

mars-lan · 2021-09-12T13:28:24Z

Gentle ping, @pgrzesik

pgrzesik

Thank you @mars-lan, both for PR and your patience. Overall it looks good, I have minor comments about tests and one, a little bit more important, about the change being potentially breaking - @medikoo @mars-lan - please let me know what do you think

docs/providers/aws/guide/serverless.yml.md

lib/plugins/aws/remove/lib/bucket.js

test/unit/lib/plugins/aws/remove/lib/bucket.test.js

lib/plugins/aws/package/lib/generateCoreTemplate.js

lib/plugins/aws/remove/lib/bucket.js

mars-lan · 2021-09-24T00:05:15Z

Ping @pgrzesik & @medikoo

…ning is enabled

medikoo

Thank you @mars-lan ! It looks very good. I have just few very minor style concerns, and tests need to be updated to use our latest methodology

lib/plugins/aws/package/lib/generateCoreTemplate.js

lib/plugins/aws/provider.js

medikoo · 2021-10-04T09:12:14Z

test/unit/lib/plugins/aws/remove/lib/bucket.test.js

@@ -80,6 +80,66 @@ describe('emptyS3Bucket', () => {
    });
  });

+  describe('#listObjectVersions()', () => {
+    it('should resolve if no object versions are present', () => {
+      const listObjectVersionsStub = sinon.stub(awsRemove.provider, 'request').resolves();


All new tests should be configured with runServerless util.

It's documented here: https://github.com/serverless/test/blob/master/docs/run-serverless.md

Also search the code for many examples on how it's used.

I believe in this given case we need to stub AWS SDK with awsRequestStubMap option. And via stubs we should confirm that expected objest from S3 bucket were attempted to be removed

Tried my best to follow the new convesion base on the examples I could find. Let me know if I'm doing it wrong.

medikoo

@mars-lan great thanks for update. It looks great!

Let's just use async/await syntax and it's good to take

test/unit/lib/plugins/aws/remove/lib/bucket.test.js

mars-lan · 2021-10-11T17:37:27Z

The failed test seems flakey and unrelated: https://github.com/serverless/serverless/pull/9912/checks?check_run_id=3861777865

medikoo

Thank you @mars-lan ! There's still some parts where async/await is not applied.

We've also fixed flaky test, so please also rebase against master

lib/plugins/aws/remove/lib/bucket.js

mars-lan · 2021-10-12T13:29:01Z

Thank you @mars-lan ! There's still some parts where async/await is not applied.

We've also fixed flaky test, so please also rebase against master

Merged

lib/plugins/aws/remove/lib/bucket.js

medikoo

Thank you @mars-lan ! That looks great

medikoo · 2021-10-12T14:08:17Z

@pgrzesik please review (it looks that it's needed to clear the status)

pgrzesik

Looks great, well done @mars-lan 👏

mars-lan · 2021-10-12T18:05:07Z

@medikoo @pgrzesik could you merge this? Thanks.

mars-lan · 2021-10-14T03:38:01Z

@medikoo & @pgrzesik any idea when the next release will be cut to include this PR?

pgrzesik · 2021-10-14T07:02:17Z

Hey @mars-lan - I plan to do a release today, it was supposed to come out yesterday, but Github had issues for the better part of the day so it was delayed.

mars-lan · 2021-10-15T01:58:16Z

@pgrzesik gentle ping.

pgrzesik · 2021-10-15T13:34:24Z

@mars-lan It has been released with 2.63.0 release

Add config to deploymentBucket to support S3 bucket versioning

9fe1158

pgrzesik added cat/deployment enhancement labels Sep 6, 2021

pgrzesik suggested changes Sep 6, 2021

View reviewed changes

test/unit/lib/plugins/aws/package/lib/generateCoreTemplate.test.js Outdated Show resolved Hide resolved

Use async test & update deployment.md

1aa4b3a

Delete all versions when emptying deployment bucket

9c30ba9

pgrzesik suggested changes Sep 13, 2021

View reviewed changes

docs/providers/aws/guide/serverless.yml.md Outdated Show resolved Hide resolved

lib/plugins/aws/remove/lib/bucket.js Outdated Show resolved Hide resolved

test/unit/lib/plugins/aws/remove/lib/bucket.test.js Outdated Show resolved Hide resolved

medikoo reviewed Sep 14, 2021

View reviewed changes

lib/plugins/aws/package/lib/generateCoreTemplate.js Outdated Show resolved Hide resolved

lib/plugins/aws/remove/lib/bucket.js Outdated Show resolved Hide resolved

Address reviewer's comments

a7b0968

Use listObjectVersions to empty deployment bucket only when S3 versio…

3955882

…ning is enabled

mars-lan requested review from medikoo and pgrzesik October 3, 2021 22:09

Merge branch 'origin'

df29963

medikoo suggested changes Oct 4, 2021

View reviewed changes

Address reviewer comments

54edcf5

mars-lan requested a review from medikoo October 11, 2021 14:52

medikoo suggested changes Oct 11, 2021

View reviewed changes

test/unit/lib/plugins/aws/remove/lib/bucket.test.js Outdated Show resolved Hide resolved

Use async/await syntax in tests

00f05a8

mars-lan requested a review from medikoo October 11, 2021 16:56

medikoo mentioned this pull request Oct 12, 2021

Test: Upgrade AWS IOT test #10079

Merged

medikoo suggested changes Oct 12, 2021

View reviewed changes

lib/plugins/aws/remove/lib/bucket.js Outdated Show resolved Hide resolved

lib/plugins/aws/remove/lib/bucket.js Outdated Show resolved Hide resolved

mars-lan added 2 commits October 12, 2021 06:18

Use async in actual code

e8a4d16

Merge branch 'origin'

0bb815a

mars-lan requested a review from medikoo October 12, 2021 13:29

medikoo reviewed Oct 12, 2021

View reviewed changes

lib/plugins/aws/remove/lib/bucket.js Outdated Show resolved Hide resolved

Remove redundant resolve

25e8ad1

mars-lan requested a review from medikoo October 12, 2021 13:37

medikoo approved these changes Oct 12, 2021

View reviewed changes

pgrzesik approved these changes Oct 12, 2021

View reviewed changes

medikoo merged commit c4cb0f3 into serverless:master Oct 12, 2021

This was referenced Oct 18, 2021

Add Serverless definitions for v2.63.0 serverless/typescript#52

Closed

Enable deployment bucket versioning breaks encryption & public access block #10130

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config to deploymentBucket to support S3 bucket versioning #9912

Add config to deploymentBucket to support S3 bucket versioning #9912

mars-lan commented Sep 4, 2021

codecov bot commented Sep 6, 2021 •

edited

pgrzesik left a comment

pgrzesik commented Sep 6, 2021

mars-lan commented Sep 6, 2021

mars-lan commented Sep 7, 2021 •

edited

mars-lan commented Sep 8, 2021

pgrzesik commented Sep 8, 2021

mars-lan commented Sep 12, 2021

pgrzesik left a comment

mars-lan commented Sep 24, 2021

medikoo left a comment

medikoo Oct 4, 2021

mars-lan Oct 11, 2021

medikoo left a comment

mars-lan commented Oct 11, 2021

medikoo left a comment

mars-lan commented Oct 12, 2021

medikoo left a comment

medikoo commented Oct 12, 2021

pgrzesik left a comment

mars-lan commented Oct 12, 2021

mars-lan commented Oct 14, 2021

pgrzesik commented Oct 14, 2021

mars-lan commented Oct 15, 2021

pgrzesik commented Oct 15, 2021

Add config to deploymentBucket to support S3 bucket versioning #9912

Add config to deploymentBucket to support S3 bucket versioning #9912

Conversation

mars-lan commented Sep 4, 2021

codecov bot commented Sep 6, 2021 • edited

Codecov Report

pgrzesik left a comment

Choose a reason for hiding this comment

pgrzesik commented Sep 6, 2021

mars-lan commented Sep 6, 2021

mars-lan commented Sep 7, 2021 • edited

mars-lan commented Sep 8, 2021

pgrzesik commented Sep 8, 2021

mars-lan commented Sep 12, 2021

pgrzesik left a comment

Choose a reason for hiding this comment

mars-lan commented Sep 24, 2021

medikoo left a comment

Choose a reason for hiding this comment

medikoo Oct 4, 2021

Choose a reason for hiding this comment

mars-lan Oct 11, 2021

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

mars-lan commented Oct 11, 2021

medikoo left a comment

Choose a reason for hiding this comment

mars-lan commented Oct 12, 2021

medikoo left a comment

Choose a reason for hiding this comment

medikoo commented Oct 12, 2021

pgrzesik left a comment

Choose a reason for hiding this comment

mars-lan commented Oct 12, 2021

mars-lan commented Oct 14, 2021

pgrzesik commented Oct 14, 2021

mars-lan commented Oct 15, 2021

pgrzesik commented Oct 15, 2021

codecov bot commented Sep 6, 2021 •

edited

mars-lan commented Sep 7, 2021 •

edited