Skip to content

Commit

Permalink
fix(cli): hotswap deploy fails on multiple CfnEvaluationException (aw…
Browse files Browse the repository at this point in the history
…s#22339)

closes aws#22323 

To avoid unhandled rejections, we run promises just before we call `Promise.all`.

The concern of this fix is that hotswap process may take longer time because now async tasks run lazily. However I don't think it will be a big problem since those tasks are not I/O bound, so most of them are already running sequentially, not in parallel.

----

### 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

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
  • Loading branch information
tmokmss authored and mrgrain committed Oct 24, 2022
1 parent 98a4115 commit 527305a
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 3 deletions.
7 changes: 4 additions & 3 deletions packages/aws-cdk/lib/api/hotswap-deployments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ async function findAllHotswappableChanges(
const resourceDifferences = getStackResourceDifferences(stackChanges);

let foundNonHotswappableChange = false;
const promises: Array<Array<Promise<ChangeHotswapResult>>> = [];
const promises: Array<() => Array<Promise<ChangeHotswapResult>>> = [];
const hotswappableResources = new Array<HotswapOperation>();

// gather the results of the detector functions
Expand All @@ -97,7 +97,8 @@ async function findAllHotswappableChanges(
} else if (resourceHotswapEvaluation === ChangeHotswapImpact.IRRELEVANT) {
// empty 'if' just for flow-aware typing to kick in...
} else {
promises.push([
// run isHotswappable* functions lazily to prevent unhandled rejections
promises.push(() => [
isHotswappableLambdaFunctionChange(logicalId, resourceHotswapEvaluation, evaluateCfnTemplate),
isHotswappableStateMachineChange(logicalId, resourceHotswapEvaluation, evaluateCfnTemplate),
isHotswappableEcsServiceChange(logicalId, resourceHotswapEvaluation, evaluateCfnTemplate),
Expand All @@ -111,7 +112,7 @@ async function findAllHotswappableChanges(
// resolve all detector results
const changesDetectionResults: Array<Array<ChangeHotswapResult>> = [];
for (const detectorResultPromises of promises) {
const hotswapDetectionResults = await Promise.all(detectorResultPromises);
const hotswapDetectionResults = await Promise.all(detectorResultPromises());
changesDetectionResults.push(hotswapDetectionResults);
}

Expand Down
87 changes: 87 additions & 0 deletions packages/aws-cdk/test/api/hotswap/hotswap-deployments.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Lambda, StepFunctions } from 'aws-sdk';
import { CfnEvaluationException } from '../../../lib/api/evaluate-cloudformation-template';
import * as setup from './hotswap-test-setup';

let hotswapMockSdkProvider: setup.HotswapMockSdkProvider;
Expand Down Expand Up @@ -414,3 +415,89 @@ test('A change to both a hotswappable resource and a stack output results in a f
expect(mockUpdateMachineDefinition).not.toHaveBeenCalled();
expect(mockUpdateLambdaCode).not.toHaveBeenCalled();
});

test('Multiple CfnEvaluationException will not cause unhandled rejections', async () => {
// GIVEN
setup.setCurrentCfnStackTemplate({
Resources: {
Func1: {
Type: 'AWS::Lambda::Function',
Properties: {
Code: {
S3Bucket: 'current-bucket',
S3Key: 'current-key',
},
Environment: {
key: 'old',
},
FunctionName: 'my-function',
},
Metadata: {
'aws:asset:path': 'old-path',
},
},
Func2: {
Type: 'AWS::Lambda::Function',
Properties: {
Code: {
S3Bucket: 'current-bucket',
S3Key: 'current-key',
},
Environment: {
key: 'old',
},
FunctionName: 'my-function',
},
Metadata: {
'aws:asset:path': 'old-path',
},
},
},
});
const cdkStackArtifact = setup.cdkStackArtifactOf({
template: {
Resources: {
Func1: {
Type: 'AWS::Lambda::Function',
Properties: {
Code: {
S3Bucket: 'current-bucket',
S3Key: 'current-key',
},
Environment: {
key: { Ref: 'ErrorResource' },
},
FunctionName: 'my-function',
},
Metadata: {
'aws:asset:path': 'new-path',
},
},
Func2: {
Type: 'AWS::Lambda::Function',
Properties: {
Code: {
S3Bucket: 'current-bucket',
S3Key: 'current-key',
},
Environment: {
key: { Ref: 'ErrorResource' },
},
FunctionName: 'my-function',
},
Metadata: {
'aws:asset:path': 'new-path',
},
},
},
},
});

// WHEN
const deployStackResult = hotswapMockSdkProvider.tryHotswapDeployment(cdkStackArtifact);

// THEN
await expect(deployStackResult).rejects.toThrowError(CfnEvaluationException);
expect(mockUpdateMachineDefinition).not.toHaveBeenCalled();
expect(mockUpdateLambdaCode).not.toHaveBeenCalled();
});

0 comments on commit 527305a

Please sign in to comment.