feat(cli): add message when resource is hotswapped by corymhall · Pull Request #18058 · aws/aws-cdk

corymhall · 2021-12-16T18:26:02Z

Add additional messages to indicate that a hotswap deployment is
occurring (or not) along with what resources are being hotswapped.

fix #17778

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

Add additional messages to indicate that a hotswap deployment is occurring (or not) along with what resources are being hotswapped. fix #17778

gitpod-io · 2021-12-16T18:26:05Z

skinny85

Looks good @corymhall! A few comments on the first pass 🙂.

skinny85 · 2021-12-16T18:40:18Z

packages/aws-cdk/lib/api/deploy-stack.ts

+    // if we can skip deployment and we are performing a hotswap, let the user know
+    // that no hotswap deployment happened
+    if (options.hotswap) {
+      print(`\n ${ICON} %s\n`, colors.bold('no hotswap deployment'));


I feel like this message needs a little polish... We have to make sure it's clear that the deployment isn't happening, because no changes were detected.

Something like:

Suggested change

print(`\n ${ICON} %s\n`, colors.bold('no hotswap deployment'));

print(`\n ${ICON} %s\n`, colors.bold('deployment skipped - no changes were detected (use --force to override)'));

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

packages/aws-cdk/lib/api/hotswap/ecs-services.ts

packages/aws-cdk/lib/api/hotswap/stepfunctions-state-machines.ts

skinny85 · 2021-12-16T18:44:44Z

packages/aws-cdk/lib/api/hotswap/stepfunctions-state-machines.ts

  public async apply(sdk: ISDK): Promise<any> {
    // not passing the optional properties leaves them unchanged
+    const name = this.stepFunctionResource.stateMachineArn.split(':')[6];
+    print(` ${ICON} hotswapping state machine: %s`, colors.bold(name));


I don't love the fact that we're putting these print statements inside of the hotswap operations themselves.

How about we change the return type of the apply() method in HotswapOperation to return the identifier of the hotswapped resource (or maybe even resources), and then the print() logic will only be inside hotswap-deployments.ts?

I feel like that's a nicer separation of concerns than having the print() statements here.

Moved these back to hotswap-deployments as we discussed.

skinny85

Looks great! Minor comments on the style, but otherwise looks awesome!

packages/aws-cdk/lib/api/hotswap/ecs-services.ts

packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts

packages/aws-cdk/lib/api/hotswap/stepfunctions-state-machines.ts

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

skinny85

Looks great, thanks @corymhall!

mergify · 2021-12-17T19:17:43Z

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).

mergify · 2021-12-17T20:01:01Z

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-17T20:01:10Z

AWS CodeBuild CI Report

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

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

eladb · 2021-12-19T08:47:03Z

Looks good. For next time, when doing UI changes, I would recommend sharing a screenshot or screencast of how this looks in the PR description or comment.

skinny85 · 2021-12-20T18:53:56Z

To be fair, Cory did actually share a gif of the experience with us, it was just in our private Slack 🙂.

eladb · 2021-12-20T19:53:31Z

Sharing is caring

This extends the existing hotswapping support for Lambda Functions to also work for Versions and Aliases. Implementation-wise, this required quite a bit of changes, as Versions are immutable in CloudFormation, and the only way you can change them is by replacing them; so, we had to add the notion of replacement changes to our hotswapping logic (as, up to this point, they were simply a pair of "delete" and "add" changes, which would result in hotswapping determining it can't proceed, and falling back to a full CloudFormation deployment). I also modified the main hotswapping algorithm: now, a resource change is considered non-hotswappable if all detectors return `REQUIRES_FULL_DEPLOYMENT` for it; if at least one detector returns `IRRELEVANT`, we ignore this change. This allows us to get rid of the awkward `EmptyHotswapOperation` that we had to use before in these situations. I also made a few small tweaks to the printing messages added in #18058: I no longer prefix them with the name of the service, as now hotswapping can affect different resource types, and it looked a bit awkward with that prefix present. Fixes #17043 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Add additional messages to indicate that a hotswap deployment is occurring (or not) along with what resources are being hotswapped. fix aws#17778 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…8145) This extends the existing hotswapping support for Lambda Functions to also work for Versions and Aliases. Implementation-wise, this required quite a bit of changes, as Versions are immutable in CloudFormation, and the only way you can change them is by replacing them; so, we had to add the notion of replacement changes to our hotswapping logic (as, up to this point, they were simply a pair of "delete" and "add" changes, which would result in hotswapping determining it can't proceed, and falling back to a full CloudFormation deployment). I also modified the main hotswapping algorithm: now, a resource change is considered non-hotswappable if all detectors return `REQUIRES_FULL_DEPLOYMENT` for it; if at least one detector returns `IRRELEVANT`, we ignore this change. This allows us to get rid of the awkward `EmptyHotswapOperation` that we had to use before in these situations. I also made a few small tweaks to the printing messages added in aws#18058: I no longer prefix them with the name of the service, as now hotswapping can affect different resource types, and it looked a bit awkward with that prefix present. Fixes aws#17043 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(cli): add messages to hotswap deployments

53436cf

Add additional messages to indicate that a hotswap deployment is occurring (or not) along with what resources are being hotswapped. fix #17778

mergify bot added the contribution/core This is a PR that came from AWS. label Dec 16, 2021

corymhall requested a review from skinny85 December 16, 2021 18:26

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

github-actions bot assigned rix0rrr Dec 16, 2021

skinny85 reviewed Dec 16, 2021

View reviewed changes

corymhall added 2 commits December 16, 2021 21:26

updates based on review

343e000

Merge branch 'master' into corymhall/hotswap-complete

446b93e

skinny85 reviewed Dec 17, 2021

View reviewed changes

packages/aws-cdk/lib/api/hotswap/ecs-services.ts Outdated Show resolved Hide resolved

packages/aws-cdk/lib/api/hotswap/s3-bucket-deployments.ts Outdated Show resolved Hide resolved

packages/aws-cdk/lib/api/hotswap/stepfunctions-state-machines.ts Outdated Show resolved Hide resolved

skinny85 changed the title ~~feat(cli): add messages to hotswap deployments~~ feat(cli): add message when resource is hotswapped Dec 17, 2021

skinny85 assigned skinny85 and unassigned rix0rrr Dec 17, 2021

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

github-actions bot assigned rix0rrr Dec 17, 2021

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

skinny85 unassigned rix0rrr Dec 17, 2021

Apply suggestions from code review

2db456e

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

skinny85 approved these changes Dec 17, 2021

View reviewed changes

Merge branch 'master' into corymhall/hotswap-complete

f1c771f

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

skinny85 mentioned this pull request Dec 23, 2021

feat(cli): support for hotswapping Lambda Versions and Aliases #18145

Merged

	print(`\n ${ICON} %s\n`, colors.bold('no hotswap deployment'));
	print(`\n ${ICON} %s\n`, colors.bold('deployment skipped - no changes were detected (use --force to override)'));

Conversation

corymhall commented Dec 16, 2021

Uh oh!

gitpod-io bot commented Dec 16, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

skinny85 Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

corymhall Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

corymhall Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Dec 17, 2021

Uh oh!

mergify bot commented Dec 17, 2021

Uh oh!

aws-cdk-automation commented Dec 17, 2021

AWS CodeBuild CI Report

Uh oh!

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

Uh oh!

Uh oh!

skinny85 commented Dec 20, 2021

Uh oh!

eladb commented Dec 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eladb commented Dec 19, 2021 •

edited

Loading