feat(cli): support hotswapping Lambda function tags by offbyone · Pull Request #17818 · aws/aws-cdk

offbyone · 2021-12-02T23:26:28Z

Fixes #17664

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

gitpod-io · 2021-12-02T23:26:30Z

mergify · 2021-12-02T23:27:22Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

offbyone · 2021-12-03T17:22:11Z

This will need at least one more revision before it's ready; hold off on review til EOD today, at least, please.

skinny85 · 2021-12-03T19:57:56Z

This will need at least one more revision before it's ready; hold off on review til EOD today, at least, please.

Sound good @offbyone, let us know when you consider this ready for review!

offbyone · 2021-12-03T23:27:37Z

I'd call it ready for review. Despite our discussion about the asset tags in the metadata, I was unable to avoid removing that check. I believe the property checks in here are sufficient safety.

skinny85

Looks great @offbyone! A few comments, but nothing major.

packages/aws-cdk/jest.config.js

packages/aws-cdk/lib/api/hotswap/common.ts

packages/aws-cdk/lib/api/hotswap/lambda-functions.ts

packages/aws-cdk/test/assets.test.ts

packages/aws-cdk/test/api/hotswap/hotswap-test-setup.ts

packages/aws-cdk/test/api/hotswap/hotswap-deployments.test.ts

packages/aws-cdk/test/api/hotswap/lambda-hotswap-deployments.test.ts

skinny85 · 2021-12-06T20:53:46Z

BTW, the build is failing with:

aws-cdk: FAIL test/api/exec.test.ts
aws-cdk:   â—� Test suite failed to run
aws-cdk:     TypeError: Cannot read property 'DEFAULT' of undefined
aws-cdk:       14 | };
aws-cdk:       15 |
aws-cdk:     > 16 | export let logLevel = LogLevel.DEFAULT;
aws-cdk:          |                                ^
aws-cdk:       17 |
aws-cdk:       18 | export function setLogLevel(newLogLevel: LogLevel) {
aws-cdk:       19 |   logLevel = newLogLevel;
aws-cdk:       at Object.<anonymous> (lib/logging.ts:16:32)
aws-cdk:       at Object.<anonymous> (lib/api/cxapp/exec.ts:6:1)

So, you might have to revert your changes to the test setup.

packages/aws-cdk/lib/api/hotswap/lambda-functions.ts

Pull request has been modified.

Implements aws#17664 For Lambda functions, identify tag changes in the CF diff, and attach tag removal / set operations for all tags that changed.

skinny85

Looks great overall! Some minor comments, mainly around formatting.

@rix0rrr do you mind taking a quick look at the test setup changes? Thanks!

skinny85 · 2021-12-10T22:16:12Z

packages/aws-cdk/lib/api/hotswap/common.ts

   */
  public readonly propertyUpdates: { [key: string]: cfn_diff.PropertyDifference<any> };

+


Let's revert this one:

Suggested change

skinny85 · 2021-12-10T22:16:53Z

packages/aws-cdk/lib/api/hotswap/lambda-functions.ts

+    const functionArn = await evaluateCfnTemplate.evaluateCfnExpression({
+      'Fn::Sub': 'arn:${AWS::Partition}:lambda:${AWS::Region}:${AWS::AccountId}:function:' + functionName,
+    });
+


Let's kill this empty line (I'm actually surprised the linter didn't flag this one):

Suggested change

skinny85 · 2021-12-10T22:17:35Z

packages/aws-cdk/lib/api/hotswap/lambda-functions.ts

+  let code: LambdaFunctionCode | void = undefined;
+  let tags: LambdaFunctionTags | void = undefined;


Suggested change

let code: LambdaFunctionCode | void = undefined;

let tags: LambdaFunctionTags | void = undefined;

let code: LambdaFunctionCode | undefined = undefined;

let tags: LambdaFunctionTags | undefined = undefined;

skinny85 · 2021-12-10T22:18:19Z

packages/aws-cdk/lib/api/hotswap/lambda-functions.ts

+          tags = {
+            tagUpdates,
+          };


Suggested change

tags = {

tagUpdates,

};

tags = { tagUpdates };

skinny85 · 2021-12-10T22:19:35Z

packages/aws-cdk/lib/api/hotswap/lambda-functions.ts

+  readonly code: LambdaFunctionCode | void,
+  readonly tags: LambdaFunctionTags | void,


Suggested change

readonly code: LambdaFunctionCode | void,

readonly tags: LambdaFunctionTags | void,

readonly code?: LambdaFunctionCode;

readonly tags?: LambdaFunctionTags;

skinny85 · 2021-12-10T22:20:41Z

packages/aws-cdk/lib/api/hotswap/lambda-functions.ts

+      // It's really unfortunate, but Lambda allows function _names_ for code
+      // updates, but we need the function ARN if we're managing tags. We also
+      // can't assume we'll get it from the preceding code update, since there
+      // might not _be_ a code update.


No need for this comment anymore I think:

Suggested change

// It's really unfortunate, but Lambda allows function _names_ for code

// updates, but we need the function ARN if we're managing tags. We also

// can't assume we'll get it from the preceding code update, since there

// might not _be_ a code update.

skinny85 · 2021-12-10T22:21:10Z

packages/aws-cdk/lib/api/hotswap/lambda-functions.ts

+    }
+
+    // run all of our updates in parallel
+    await Promise.all(operations);


Suggested change

await Promise.all(operations);

return Promise.all(operations);

skinny85 · 2021-12-10T22:21:39Z

packages/aws-cdk/lib/logging.ts

+  DEBUG = 1,
+  /** Extremely verbose */
+  TRACE = 2
+}


Let's revert this move too (unless there's a good reason for it...?).

Pull request has been modified.

mergify · 2021-12-17T17:01:37Z

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

aws-cdk-automation · 2021-12-17T17:45:17Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: e10caef
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-12-17T17:45:36Z

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

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

github-actions bot assigned kaizencc Dec 2, 2021

github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Dec 2, 2021

offbyone changed the title ~~Hotswap support for tags on Lambda functions~~ feat(cli): Hotswap support for tags on Lambda functions Dec 2, 2021

github-actions bot assigned rix0rrr Dec 2, 2021

github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Dec 2, 2021

offbyone force-pushed the feature-hotswap-tags branch 3 times, most recently from 9bb95cb to 03b8775 Compare December 3, 2021 16:39

kaizencc removed the @aws-cdk/aws-lambda Related to AWS Lambda label Dec 3, 2021

kaizencc assigned skinny85 and unassigned rix0rrr and kaizencc Dec 3, 2021

chore(cli): use ts-jest to test the CLI

02b356e

offbyone force-pushed the feature-hotswap-tags branch from 03b8775 to b280bce Compare December 3, 2021 23:26

offbyone force-pushed the feature-hotswap-tags branch from b280bce to 09799b2 Compare December 3, 2021 23:31

github-actions bot assigned rix0rrr Dec 6, 2021

skinny85 previously requested changes Dec 6, 2021

View reviewed changes

skinny85 reviewed Dec 7, 2021

View reviewed changes

packages/aws-cdk/lib/api/hotswap/lambda-functions.ts Outdated Show resolved Hide resolved

offbyone force-pushed the feature-hotswap-tags branch from 09799b2 to 8188ffe Compare December 8, 2021 22:16

offbyone force-pushed the feature-hotswap-tags branch 2 times, most recently from 826a7f6 to cc18ae6 Compare December 8, 2021 23:35

feat(cli): support tags on hotswapped lambda

71ef767

Implements aws#17664 For Lambda functions, identify tag changes in the CF diff, and attach tag removal / set operations for all tags that changed.

offbyone force-pushed the feature-hotswap-tags branch from cc18ae6 to 71ef767 Compare December 9, 2021 00:53

offbyone marked this pull request as ready for review December 9, 2021 16:22

skinny85 reviewed Dec 10, 2021

View reviewed changes

skinny85 added 2 commits December 16, 2021 18:09

Merge remote-tracking branch 'origin/master' into feature-hotswap-tags

f512ad4

await -> return

7aeab1b

skinny85 previously approved these changes Dec 17, 2021

View reviewed changes

skinny85 changed the title ~~feat(cli): Hotswap support for tags on Lambda functions~~ feat(cli): support hotswapping Lambda function tags Dec 17, 2021

skinny85 added the pr-linter/exempt-readme The PR linter will not require README changes label Dec 17, 2021

skinny85 approved these changes Dec 17, 2021

View reviewed changes

Merge branch 'master' into feature-hotswap-tags

e10caef

mergify bot merged commit e4485f4 into aws:master Dec 17, 2021

		*/
		public readonly propertyUpdates: { [key: string]: cfn_diff.PropertyDifference<any> };

		let code: LambdaFunctionCode \| void = undefined;
		let tags: LambdaFunctionTags \| void = undefined;

		readonly code: LambdaFunctionCode \| void,
		readonly tags: LambdaFunctionTags \| void,

	await Promise.all(operations);
	return Promise.all(operations);

Conversation

offbyone commented Dec 2, 2021 • edited by skinny85 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitpod-io bot commented Dec 2, 2021

Uh oh!

mergify bot commented Dec 2, 2021

Uh oh!

offbyone commented Dec 3, 2021

Uh oh!

skinny85 commented Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

offbyone commented Dec 3, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 commented Dec 6, 2021

Uh oh!

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Dec 17, 2021

Uh oh!

aws-cdk-automation commented Dec 17, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Dec 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

offbyone commented Dec 2, 2021 •

edited by skinny85

Loading

skinny85 commented Dec 3, 2021 •

edited

Loading