feat(s3-deployment): extract flag to disable automatic unzipping #21805

wanjacki · 2022-08-29T15:18:52Z

Currently S3 Deployment will unzip all files when deployed to S3. This PR adds an additional optional prop extract, that when set to false will allow files to remain zipped when deployed to S3.

Adds the feature described in issue: #8065

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Co-authored-by: Alex Makoviecki [email protected]

gitpod-io · 2022-08-29T15:19:00Z

TheRealAmazonKendra · 2022-08-30T17:26:56Z

In the issue you linked has a discussion of why we have not gone in this direction. Please take another look at this comment.

Given that we will not be going in this direction, I'm going to close this particular PR but, if a fix for this issue is something you'd like to continue pursuing, we can certainly continue the conversation in that linked issue.

TheRealAmazonKendra · 2022-09-09T14:47:48Z

@Mergifyio update

mergify · 2022-09-09T14:48:24Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra · 2022-09-11T01:10:46Z

When you commit, please don't use force-push. It makes seeing the changes between commits more difficult.

TheRealAmazonKendra · 2022-09-11T01:10:55Z

@Mergifyio update

mergify · 2022-09-11T01:11:22Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests).

Additionally, please see my comments inline.

TheRealAmazonKendra · 2022-09-11T01:47:25Z

packages/@aws-cdk/aws-s3-deployment/lib/lambda/index.py

+                logger.info("| markers: %s" % markers)
+                extract_and_replace_markers(archive, contents_dir, markers)
+            else:
+                aws_command("s3", "cp", s3_source_zip, contents_dir)


Let's still providing logging info in the else.

Logging added.

TheRealAmazonKendra · 2022-09-11T01:49:55Z

packages/@aws-cdk/aws-s3-deployment/test/integ.bucket-deployment.ts

+      extract: false,
+      retainOnDelete: false,
+    });
+


This should probably have some sort of assertion to test that it's both uploaded properly and that it hasn't been extracted.

I made use of the AwsApiCall construct and assertions feature provided by the integ-tests module to assert that the archive for the website folder is in the target bucket after deployment of the integ stack. Let me know if this is a solid assertion and proper use of the testing constructs.

I think this looks reasonable.

TheRealAmazonKendra · 2022-09-11T01:51:07Z

packages/@aws-cdk/aws-s3-deployment/test/lambda/aws

-# if "cp" is called with a local destination, copy a test zip file to the destination or 
-if sys.argv[2] == "cp" and not sys.argv[4].startswith("s3://"):
+# if "cp" is called to contents, copy a test zip file to contents
+# else if "cp" is called with a local destination, copy a test zip file to the destination or


This is kind of a nit, but these comments are the wrong order for what's the if statement and what's the else if statement.

I attempted to correct this. Let me know if the order makes more sense now.

Pull request has been modified.

kaizencc

Remember to update the title and PR description to reflect the correct property name of extract rather than unzipFile

mackalex · 2022-09-14T17:45:29Z

@Mergifyio update

mergify · 2022-09-14T17:46:02Z

@mackalex is not allowed to run commands

rix0rrr · 2022-09-15T08:34:02Z

packages/@aws-cdk/aws-s3-deployment/README.md

+## Keep Files Zipped
+
+By default, files are zipped, then extracted into the destination bucket.
+You can use the option `extract: false` to disable this behavior, in which case, files will remain in a zip file when deployed to S3.


Say something about the target filename and how it can be acquired by users.

I think by target filename you are referring to the *.zip file that is found in the target S3 bucket after bucket deployment. Is that correct?

rix0rrr · 2022-09-15T08:35:15Z

packages/@aws-cdk/aws-s3-deployment/lib/lambda/index.py

+                aws_command("s3", "cp", s3_source_zip, archive)
+                logger.info("| extracting archive to: %s\n" % contents_dir)
+                logger.info("| markers: %s" % markers)
+                extract_and_replace_markers(archive, contents_dir, markers)


It looks like marker replacement won't work anymore if extract == false.

Check for the combination of markers and extract == false in the constructor of the BucketDeployment and throw an error that these features cannot be combined.

That's correct. I used the existing hasMarkers const in a new conditional that checks if sources have markers and that extract is false, and then it throws an error on construction of BucketDeployment. I added a unit test for this as well.

rix0rrr · 2022-09-15T08:40:53Z

...teg.snapshot/asset.bc935fa4a9499a4b71f067e37a7f05ed05bbcdbf4dc33a44747bc8da38db7e92/index.py

+                logger.info("| markers: %s" % markers)
+                extract_and_replace_markers(archive, contents_dir, markers)
+            else:
+                aws_command("s3", "cp", s3_source_zip, contents_dir)


Return an output property that contains the full generated object key (excluding the bucket name). This is going to entail adding a field to the responseData object here:

aws-cdk/packages/@aws-cdk/aws-s3-deployment/lib/lambda/index.py

Line 121 in 1f03e8c

cfn_send(event, context, CFN_SUCCESS, physicalResourceId=physical_id, responseData={

And making it accessible as a property via a this.cr.getAtt() like here:

aws-cdk/packages/@aws-cdk/aws-s3-deployment/lib/bucket-deployment.ts

Line 433 in 1f03e8c

this._deployedBucket = this._deployedBucket ?? s3.Bucket.fromBucketArn(this, 'DestinationBucket', cdk.Token.asString(this.cr.getAtt('DestinationBucketArn')));

It can be in a getter that returns a string. Don't forget to document it.

There are multiple object keys if multiple sources are provided to the BucketDeployment construct. Would it make the most sense to return a list of strings which are the object keys?

Yes, that makes sense

Pull request has been modified.

…zip file

mackalex · 2022-09-15T19:07:34Z

Force-push was required due to improper rebasing before pushing on my end.

… test

padaszewski · 2022-09-20T12:11:52Z

Would love to see any progress here, as the guys have a deadline for implementing the service catalog assets (depends on this PR) feature, which is extremly important for all cdk + service catalog users.

kaizencc

I have a few minor comments but other than that I think this PR is good to go :).

kaizencc · 2022-09-21T20:06:54Z

packages/@aws-cdk/aws-s3-deployment/test/bucket-deployment.test.ts

+  });
+});
+
+test('deploy without extracting files in destination', () => {


this test has the same name as above.

Good catch, thanks! Will update

kaizencc · 2022-09-21T20:14:19Z

packages/@aws-cdk/aws-s3-deployment/test/integ.bucket-deployment.ts

+      extract: false,
+      retainOnDelete: false,
+    });
+


I think this looks reasonable.

kaizencc · 2022-09-21T20:16:48Z

packages/@aws-cdk/aws-s3-deployment/lib/bucket-deployment.ts

@@ -339,6 +346,10 @@ export class BucketDeployment extends Construct {
    // the sources actually has markers.
    const hasMarkers = sources.some(source => source.markers);

+    // Markers are not replaced if zip sources are not extracted, so throw an error
+    // if extraction is not wanted and sources have markers.
+    if (hasMarkers && props.extract == false) { throw new Error('Extract cannot be false if sources have markers'); }


Suggested change

if (hasMarkers && props.extract == false) { throw new Error('Extract cannot be false if sources have markers'); }

if (hasMarkers && props.extract === false) {

throw new Error('Some sources are incompatible with extract=false; sources with deploy-time values (such as \'snsTopic.topicArn\') must be extracted.');

}

My problem here is that markers is an internal property that the user is not really privy to. Getting an error that says that your source has markers is pretty cryptic. Markers are substitutes for Ref and Fn::GetAtt objects, which come from deploy-time values. So lets create an error message that reflects that.

Thank you for the suggestion. I'll incorporate this into a better error message.

Pull request has been modified.

mergify · 2022-09-22T14:15:59Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-09-22T18:02:44Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: f566d6b
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-09-22T18:03:04Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…#21805) Currently S3 Deployment will unzip all files when deployed to S3. This PR adds an additional optional prop `extract`, that when set to false will allow files to remain zipped when deployed to S3. Adds the feature described in issue: aws#8065 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* ---- Co-authored-by: Alex Makoviecki [[email protected]](mailto:[email protected])

eknochenhauer · 2023-05-26T17:15:46Z

This change has not been added to the documentation just an FYI

scr2em · 2023-08-15T07:07:01Z

Is this documented ?

aws-cdk-automation requested a review from a team August 29, 2022 15:19

github-actions bot added the p2 label Aug 29, 2022

TheRealAmazonKendra closed this Aug 30, 2022

iliapolo mentioned this pull request Sep 7, 2022

aws-s3-deployment: Transfer asset without extracting contents of .zip files #8065

Closed

2 tasks

TheRealAmazonKendra reopened this Sep 8, 2022

mackalex force-pushed the s3-deployment branch from c3201c0 to 97f1839 Compare September 9, 2022 02:20

wanjacki force-pushed the s3-deployment branch from 1223c03 to fa49c09 Compare September 9, 2022 21:54

TheRealAmazonKendra previously requested changes Sep 11, 2022

View reviewed changes

kaizencc reviewed Sep 14, 2022

View reviewed changes

wanjacki changed the title ~~feat(s3-deployment): Added unzipFile flag to disable automatic unzipping~~ feat(s3-deployment): Added extract flag to disable automatic unzipping Sep 14, 2022

rix0rrr previously requested changes Sep 15, 2022

View reviewed changes

wanjacki and others added 4 commits September 15, 2022 12:06

feat(s3-deployment): Added extract flag to disable automatic unzipping

5ba7836

Add logging to extract false case

495206d

Fix comment readability in Lambda test

0e2e033

Add integ test assertion to check that deployment to bucket contains …

a4ff605

…zip file

mackalex force-pushed the s3-deployment branch from c207c61 to a4ff605 Compare September 15, 2022 19:07

Throw an error if extract is false and sources have markers, add unit…

1667422

… test

wanjacki mentioned this pull request Sep 20, 2022

feat(servicecatalog): Add Product Stack Asset Support #22143

Closed

4 tasks

Alex Makoviecki added 3 commits September 20, 2022 11:49

Add objectKeys getter for referencing source object keys

cdc3ac2

Update README to include method of obtaining object key for source file

3ee3393

Fix spacing in README

fb6893b

wanjacki requested a review from rix0rrr September 20, 2022 20:10

kaizencc changed the title ~~feat(s3-deployment): Added extract flag to disable automatic unzipping~~ feat(s3-deployment): extract flag to disable automatic unzipping Sep 21, 2022

kaizencc previously requested changes Sep 21, 2022

View reviewed changes

Rename test, improve error message

d889474

rix0rrr approved these changes Sep 22, 2022

View reviewed changes

Merge branch 'main' into s3-deployment

f566d6b

mergify bot merged commit 91898b5 into aws:main Sep 22, 2022

-    if (hasMarkers && props.extract == false) { throw new Error('Extract cannot be false if sources have markers'); }
+    if (hasMarkers && props.extract === false) {
+      throw new Error('Some sources are incompatible with extract=false; sources with deploy-time values (such as \'snsTopic.topicArn\') must be extracted.');
+    }

feat(s3-deployment): extract flag to disable automatic unzipping #21805

feat(s3-deployment): extract flag to disable automatic unzipping #21805

Conversation

wanjacki commented Aug 29, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Aug 29, 2022

TheRealAmazonKendra commented Aug 30, 2022

TheRealAmazonKendra commented Sep 9, 2022

mergify bot commented Sep 9, 2022

✅ Branch has been successfully updated

TheRealAmazonKendra commented Sep 11, 2022

TheRealAmazonKendra commented Sep 11, 2022

mergify bot commented Sep 11, 2022

✅ Branch has been successfully updated

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaizencc left a comment

Choose a reason for hiding this comment

mackalex commented Sep 14, 2022

mergify bot commented Sep 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mackalex commented Sep 15, 2022

padaszewski commented Sep 20, 2022

kaizencc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Sep 22, 2022

aws-cdk-automation commented Sep 22, 2022

AWS CodeBuild CI Report

mergify bot commented Sep 22, 2022

eknochenhauer commented May 26, 2023

scr2em commented Aug 15, 2023

wanjacki commented Aug 29, 2022 •

edited

Loading