feat(lambda-layer-awscli): Dynamically load asset for AwsCliLayer, with bundled fallback #21938

madeline-k · 2022-09-07T04:56:02Z

This PR introduces a change to dynamically load the large Asset zip file used by the AwsCliLayer construct from a separate package: https://www.npmjs.com/package/@aws-cdk/asset-awscli-v1/. This change is part of aws/aws-cdk-rfcs#430

This may not work in some customer's environments, so we will release this feature in two steps.

This PR, which adds all of the dynamic-loading logic, but bundles the separate @aws-cdk/asset-awscli-v1 package into aws-cdk-lib to use as a fallback for customers whose build environments are not able to install from npm. For customers who will experience a breaking change when that fallback is removed, warnings and CLI notices will be printed for them linking to instructions to update and prepare for the version without the fallback. The first phase will not significantly affect the size of the aws-cdk-lib package.
Remove the fallback, and actually reduce the size of aws-cdk-lib.

Remaining:

test that verify no-network and jsii-target lang scenarios - In Progress by @kaizencc
Add language-specific instructions for using explicit dependency as a workaround

When this is released, we will also create a CLI Notice. The PR for that notice is open here: cdklabs/aws-cdk-notices#21

The github issue will be here: #22470

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

…ts from Asset Constructs

gitpod-io · 2022-09-07T04:56:09Z

…awscli

…de objects from Asset Constructs" This reverts commit 217a199.

This reverts commit 3819c26.

… integration tests can actually run

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

…awscli

Pull Request updated. Dissmissing previous PRLinter Review.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/21938/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

madeline-k · 2022-09-27T16:52:19Z

This change is exempt integ test changes because one hard requirement is that it does not change the behavior or content of existing stacks.

Pull Request updated. Dissmissing previous PRLinter Review.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/21938/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

Pull Request updated. Dissmissing previous PRLinter Review.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/21938/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

packages/@aws-cdk/lambda-layer-awscli/layer/download-fallback.sh

packages/@aws-cdk/lambda-layer-awscli/lib/awscli-layer.ts

rix0rrr · 2022-10-11T09:02:25Z

packages/@aws-cdk/lambda-layer-awscli/lib/awscli-layer.ts

+    if (assetPackage) {
+      // ask for feedback here
+      const asset = new assetPackage.AwsCliAsset(scope, `${id}-asset`);
+      code = lambda.Code.fromBucket(asset.bucket, asset.s3ObjectKey);
+    }


It all happens at runtime in JavaScript, which doesn't have types. So: we can check for the presence of the symbol, but never its type.

if (!(assetPackage as any).AwsCliAsset) { throw new Error('Ruh roh'); }

This is probably good enough, though we might need to stick an API version somewhere...

packages/@aws-cdk/lambda-layer-awscli/lib/awscli-layer.ts

rix0rrr · 2022-10-11T09:18:04Z

packages/@aws-cdk/lambda-layer-awscli/lib/awscli-layer.ts

+    }
+    availableVersion = AwsCliLayer.requireWrapper(path.join(assetPackagePath, '../../package.json'), logs).version;
+
+    if (targetVersion === availableVersion) {


Do we care exactly? Do we need semver in here to check range compatibility?

Needs to log if the version is there but we rejected it btw. That's good info to know.

packages/@aws-cdk/lambda-layer-awscli/lib/awscli-layer.ts

…ed in testing

madeline-k · 2022-10-12T17:52:18Z

packages/@aws-cdk/lambda-layer-awscli/package.json

+  "bundledDependencies": [
+    "semver"


I'm not sure I did this correctly... Adding the non-JSII package as a dependency forced me to add it here as well. And then having a bundled dependency led me to add it to the "nohoist" list in our top-level package.json using yarn build --fix. It's strange because semver is already a dependency of aws-cdk-lib, so this will get ignored in the release anyway.

Potentially, we could add a utility function in @aws-cdk/core that does the version range check and uses the semver bundled there. Just available to TS/JS, nothing else would be good enough to start. Something like:

export function versionMatchesRange(version: string, range: string) { // Use semver here }

…erated by the build

aws-cdk-automation · 2022-10-13T23:30:53Z

The pull request linter fails with the following errors:

❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/21938/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

aws-cdk-automation · 2022-10-13T23:44:28Z

AWS CodeBuild CI Report

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

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

rix0rrr · 2022-10-14T08:17:57Z

packages/@aws-cdk/lambda-layer-awscli/layer/pre-build.sh

+cp $dir/lib/layer.zip ./lib
+
+target_version=$(node -pe 'require("@aws-cdk/asset-awscli-v1/package.json").version')
+echo "export const TARGET_VERSION = '${target_version}';" > lib/asset-package-version.ts


Suggested change

echo "export const TARGET_VERSION = '${target_version}';" > lib/asset-package-version.ts

echo "export const RECOMMENDED_VERSION = '${target_version}';" > lib/asset-package-version.ts

?

rix0rrr

Modulo tiny things I think this looks good!

rix0rrr · 2022-10-14T08:20:10Z

packages/@aws-cdk/lambda-layer-awscli/lib/awscli-layer.ts

+    }
+
+    if (fallback) {
+      Annotations.of(this).addWarning(`[ACTION REQUIRED] Your CDK application is using ${this.constructor.name}. Add a dependency on ${AwsCliLayer.assetPackageName}, or the equivalent in your language, to remove this warning. See https://github.com/aws/aws-cdk/issues/22470 for more information.`);


Suggested change

Annotations.of(this).addWarning(`[ACTION REQUIRED] Your CDK application is using ${this.constructor.name}. Add a dependency on ${AwsCliLayer.assetPackageName}, or the equivalent in your language, to remove this warning. See https://github.com/aws/aws-cdk/issues/22470 for more information.`);

Annotations.of(this).addWarning(`[ACTION REQUIRED] Your CDK application is using ${this.constructor.name} and synthesizes in an environment where ${AwsCliLayer.assetPackageName} cannot be automatically downloaded. Add a dependency on ${AwsCliLayer.assetPackageName}, or the equivalent in your language to remove this warning. See https://github.com/aws/aws-cdk/issues/22470 for more information.`);

corymhall · 2022-11-30T15:33:26Z

@madeline-k / @kaizencc should this be closed?

kaizencc · 2022-12-06T20:26:06Z

Closing for now. @madeline-k had thoughts to poc a different solution, but can revive it later.

madeline-k added 3 commits September 6, 2022 17:20

feat(lambda): add Code.fromAssetConstruct API for creating Code objec…

217a199

…ts from Asset Constructs

add to README

3819c26

feat: Dynamically load asset for AwsCliLayer, with bundled fallback

6d45a8f

madeline-k marked this pull request as draft September 7, 2022 04:56

github-actions bot added the p2 label Sep 7, 2022

aws-cdk-automation requested a review from a team September 7, 2022 04:56

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 7, 2022

madeline-k mentioned this pull request Sep 7, 2022

feat: Reduce aws-cdk-lib package size by not bundling awscli Lambda Layer #21799

Closed

8 tasks

madeline-k changed the title ~~feat: Dynamically load asset for AwsCliLayer, with bundled fallback~~ feat(lambda-layer-awscli): Dynamically load asset for AwsCliLayer, with bundled fallback Sep 7, 2022

madeline-k requested review from kaizencc and rix0rrr September 7, 2022 05:01

Merge branch 'main' into madeline-k/reduce-package-size/lambda-layer-…

7e68272

…awscli

madeline-k changed the base branch from madeline-k/lambda/fromAssetConstruct to main September 20, 2022 16:20

madeline-k added 4 commits September 20, 2022 09:21

Revert "feat(lambda): add Code.fromAssetConstruct API for creating Co…

c244bc8

…de objects from Asset Constructs" This reverts commit 217a199.

Revert "add to README"

a829e28

This reverts commit 3819c26.

update to use latest name and api from @aws-cdk/asset-awscli-v1 package

b01414b

temporarily adjust coverage thresholds so that jest succeeds, and the…

dcaf2ce

… integration tests can actually run

aws-cdk-automation previously requested changes Sep 20, 2022

View reviewed changes

madeline-k added 3 commits September 20, 2022 21:09

wip

5a35d5a

Merge branch 'main' into madeline-k/reduce-package-size/lambda-layer-…

da1f1ba

…awscli

updates

1f856c7

aws-cdk-automation previously requested changes Sep 27, 2022

View reviewed changes

madeline-k added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Sep 27, 2022

aws-cdk-automation previously requested changes Sep 27, 2022

View reviewed changes

rix0rrr reviewed Oct 11, 2022

View reviewed changes

madeline-k added 6 commits October 11, 2022 12:12

add set -eu

6d5dd99

modify pre-build script to copy files from local node_modules

7657203

refactor functions into a separate file so they can be private and us…

bda4f88

…ed in testing

more updates

8c2edba

get target version at build time

b2af0c6

update warning message

01f194e

madeline-k requested a review from rix0rrr October 12, 2022 15:08

madeline-k added 5 commits October 12, 2022 08:15

remove WARNING from the warning. It's a warning.

4b11d81

fix comma placement, and add real github issue link

a5e35f6

use semver

7415ae8

yarn build --fix

729f9cb

fix string match in test

9227e93

madeline-k requested a review from a team October 12, 2022 15:44

madeline-k commented Oct 12, 2022

View reviewed changes

madeline-k and others added 6 commits October 12, 2022 10:59

use compatible semver range

af77de9

update asset-awscli dependency, add error handling for mkdirSync

33df916

remove requirement.txt from being tracked by git, since it is now gen…

70d9fb7

…erated by the build

temporarily lower coverage thresholds

b9d5bd8

fix: remove node_modules folder after test adds it in

2ab6282

fix download location to avoid duplicate node_modules folders

e97124c

rix0rrr reviewed Oct 14, 2022

View reviewed changes

rix0rrr approved these changes Oct 14, 2022

View reviewed changes

kaizencc marked this pull request as draft December 6, 2022 20:25

kaizencc closed this Dec 6, 2022

RomainMuller deleted the madeline-k/reduce-package-size/lambda-layer-awscli branch July 4, 2023 08:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lambda-layer-awscli): Dynamically load asset for AwsCliLayer, with bundled fallback #21938

feat(lambda-layer-awscli): Dynamically load asset for AwsCliLayer, with bundled fallback #21938

madeline-k commented Sep 7, 2022 •

edited

Loading

gitpod-io bot commented Sep 7, 2022

aws-cdk-automation left a comment

aws-cdk-automation left a comment

madeline-k commented Sep 27, 2022

aws-cdk-automation left a comment

aws-cdk-automation left a comment

rix0rrr Oct 11, 2022

rix0rrr Oct 11, 2022

rix0rrr Oct 11, 2022

madeline-k Oct 12, 2022 •

edited

Loading

rix0rrr Oct 14, 2022

aws-cdk-automation commented Oct 13, 2022

aws-cdk-automation commented Oct 13, 2022

rix0rrr Oct 14, 2022

rix0rrr left a comment

rix0rrr Oct 14, 2022

corymhall commented Nov 30, 2022

kaizencc commented Dec 6, 2022

	echo "export const TARGET_VERSION = '${target_version}';" > lib/asset-package-version.ts
	echo "export const RECOMMENDED_VERSION = '${target_version}';" > lib/asset-package-version.ts

feat(lambda-layer-awscli): Dynamically load asset for AwsCliLayer, with bundled fallback #21938

feat(lambda-layer-awscli): Dynamically load asset for AwsCliLayer, with bundled fallback #21938

Conversation

madeline-k commented Sep 7, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Sep 7, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

madeline-k commented Sep 27, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

rix0rrr Oct 11, 2022

Choose a reason for hiding this comment

rix0rrr Oct 11, 2022

Choose a reason for hiding this comment

rix0rrr Oct 11, 2022

Choose a reason for hiding this comment

madeline-k Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

rix0rrr Oct 14, 2022

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 13, 2022

aws-cdk-automation commented Oct 13, 2022

AWS CodeBuild CI Report

rix0rrr Oct 14, 2022

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr Oct 14, 2022

Choose a reason for hiding this comment

corymhall commented Nov 30, 2022

kaizencc commented Dec 6, 2022

madeline-k commented Sep 7, 2022 •

edited

Loading

madeline-k Oct 12, 2022 •

edited

Loading