Skip to content

Commit

Permalink
fix(ecs): cannot disable container insights of an ECS cluster (#9151)
Browse files Browse the repository at this point in the history
fixes #9149 

The clusterSettings should be explicitly defined even when container insights is disabled, because CFn doesn't allow to remove the existing settings (as described in the issue.)

By this change, users who sets `containerInsights` prop `false` will see a diff like below, but it won't be a problem because by default container insights is disabled hence no actual change to existing resources: 

```
Resources
[~] AWS::ECS::Cluster cluster cluster611F8AFF 
 └─ [+] ClusterSettings
     └─ [{"Name":"containerInsights","Value":"disabled"}]
```

To prevent the existing tests from failing, the clusterSettings is only defined when `containerInsight` prop is explicitly defined(i.e. `true` or `false`, not `undefined`.)

----

*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 Dec 9, 2020
1 parent 84c959c commit e328f22
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 3 deletions.
6 changes: 4 additions & 2 deletions packages/@aws-cdk/aws-ecs/lib/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,10 @@ export class Cluster extends Resource implements ICluster {
physicalName: props.clusterName,
});

const containerInsights = props.containerInsights !== undefined ? props.containerInsights : false;
const clusterSettings = containerInsights ? [{ name: 'containerInsights', value: 'enabled' }] : undefined;
let clusterSettings = undefined;
if (props.containerInsights !== undefined) {
clusterSettings = [{ name: 'containerInsights', value: props.containerInsights ? 'enabled' : 'disabled' }];
}

const cluster = new CfnCluster(this, 'Resource', {
clusterName: this.physicalName,
Expand Down
22 changes: 21 additions & 1 deletion packages/@aws-cdk/aws-ecs/test/test.ecs-cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1457,7 +1457,27 @@ export = {
test.done();
},

'default container insights undefined'(test: Test) {
'disable container insights'(test: Test) {
// GIVEN
const app = new cdk.App();
const stack = new cdk.Stack(app, 'test');

new ecs.Cluster(stack, 'EcsCluster', { containerInsights: false });

// THEN
expect(stack).to(haveResource('AWS::ECS::Cluster', {
ClusterSettings: [
{
Name: 'containerInsights',
Value: 'disabled',
},
],
}, ResourcePart.Properties));

test.done();
},

'default container insights is disabled'(test: Test) {
// GIVEN
const app = new cdk.App();
const stack = new cdk.Stack(app, 'test');
Expand Down

0 comments on commit e328f22

Please sign in to comment.