feat(events-targets): allow all ECS TaskOverrides #32344

blimmer · 2024-12-01T17:33:56Z

Issue #32217

Reason for this change

ECS targets can override any item in the TaskOverride structure via the input parameter, according to the docs.

However, today, only the containerOverrides option is exposed:

aws-cdk/packages/aws-cdk-lib/aws-events-targets/lib/ecs-task.ts

Lines 225 to 227 in 1b7265b

    
           const containerOverrides = this.props.containerOverrides && this.props.containerOverrides 
        
             .map(({ containerName, ...overrides }) => ({ name: containerName, ...overrides })); 
        
           const input = { containerOverrides };

Description of changes

This PR adds all TaskOverride properties to the input parameter.

Description of how you validated changes

I add unit tests. I also updated the existing integration tests to include additional overrides. I validated them in my AWS account.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

blimmer · 2024-12-01T17:34:44Z

packages/aws-cdk-lib/aws-events-targets/lib/ecs-task-properties.ts

+ */
+export interface EphemeralStorageOverride {
+  /**
+   * The total amount, in GiB, of ephemeral storage to set for the task.


I copied these docstrings from the Task Override docs: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_TaskOverride.html

blimmer · 2024-12-01T17:35:02Z

packages/aws-cdk-lib/aws-events-targets/lib/ecs-task.ts

@@ -50,6 +50,54 @@ export interface EcsTaskProps extends TargetBaseProps {
   */
  readonly containerOverrides?: ContainerOverride[];

+  /**
+   * The CPU override for the task.


Again, copied from the upstream docs: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_TaskOverride.html

blimmer · 2024-12-01T17:35:30Z

packages/aws-cdk-lib/aws-events-targets/lib/ecs-task.ts

    const taskCount = this.taskCount;
    const taskDefinitionArn = this.taskDefinition.taskDefinitionArn;
    const propagateTags = this.propagateTags;
    const tagList = this.tags;
    const enableExecuteCommand = this.enableExecuteCommand;
+    const input = this.createInput();


Since this logic is more complex now, I refactored it to a dedicated private method.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

codecov · 2024-12-01T17:53:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.92%. Comparing base (fbcb732) to head (b7f8bb4).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32344   +/-   ##
=======================================
  Coverage   80.92%   80.92%           
=======================================
  Files         236      236           
  Lines       14253    14253           
  Branches     2490     2490           
=======================================
  Hits        11534    11534           
  Misses       2434     2434           
  Partials      285      285

Flag	Coverage Δ
suite.unit	`80.92% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`79.73% <ø> (ø)`
packages/aws-cdk-lib/core	`82.20% <ø> (ø)`

aaythapa

Thanks for adding this! Looks good, just a few nits

aaythapa · 2025-02-05T22:14:45Z

packages/aws-cdk-lib/aws-events-targets/lib/ecs-task.ts

+   *
+   * @default - The task definition's ephemeral storage value
+   */
+  readonly ephemeralStorage?: EphemeralStorageOverride;


Nit: since there is only one property in the EphemeralStorageOverride interface would it be better to just support that there? Something like ephemeralStorageSizeInGiB: number

I copied the upstream CFN object, since it's an object: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_EphemeralStorage.html

Since they made it an object in CFN, my thinking is that there might be additional properties added later on, so I followed that pattern here.

However, if you'd prefer the L2 implementation to hide this detail, I can make it a single property. Just let me know.

packages/aws-cdk-lib/aws-events-targets/test/ecs/event-rule-target.test.ts

aaythapa · 2025-02-05T22:26:07Z

...aws-cdk-testing/framework-integ/test/aws-events-targets/test/ecs/integ.event-fargate-task.ts

@@ -47,6 +47,8 @@ rule.addTarget(new targets.EcsTask({
  taskDefinition,
  taskCount: 1,
  enableExecuteCommand: true,
+  cpu: '512',


nit: can we add assertions to ensure the CPU is overridden?

Sure - I've never used the assertions feature - let me look into how to do it 👍

@aaythapa I spent about 4 hours today trying to get this working and, unfortunately, I'm stumped. It seems that the integration assertion logic does not support the JSON-stringified Input parameter well. I've tried many iterations of things, but I can't extract the properties correctly.

const ruleDetails = integTest.assertions.awsApiCall('CloudWatchEvents', 'listTargetsByRule', { Rule: rule.ruleName, }); ruleDetails.assertAtPath('Targets.0.Input', integ.ExpectedResult.objectLike({ cpu: '512', }));

Fails with:

{ "Status": "SUCCESS", "Reason": "OK", "PhysicalResourceId": "AwsApiCallCloudWatchEventslistTargetsByRule9b8ce4aebbf85138ea0a2d2c72bf5f5a", "StackId": "arn:aws:cloudformation:us-east-1:291961553751:stack/EcsFargateTestDefaultTestDeployAssert36341BFB/cf6d79a0-e65d-11ef-818c-0affd8c950cf", "RequestId": "4f0bc12e-8155-4cf3-a37c-80272da0b959", "LogicalResourceId": "AwsApiCallCloudWatchEventslistTargetsByRule9b8ce4aebbf85138ea0a2d2c72bf5f5a", "NoEcho": false, "Data": { "failed": true, "assertion": "{\"status\":\"fail\",\"message\":\"!! Expected type object but received undefined\\nundefined\"}", "apiCallResponse.Targets.0.Input.containerOverrides.0.name": "TheContainer", "apiCallResponse.Targets.0.Input.containerOverrides.0.environment.0.name": "I_WAS_TRIGGERED", "apiCallResponse.Targets.0.Input.containerOverrides.0.environment.0.value": "From CloudWatch Events", "apiCallResponse.Targets.0.Input.cpu": "512", "apiCallResponse.Targets.0.Input.memory": "512" } }

I also can't do an exact match on the CPU like this:

const ruleDetails = integTest.assertions.awsApiCall('CloudWatchEvents', 'listTargetsByRule', { Rule: rule.ruleName, }); ruleDetails.assertAtPath('Targets.0.Input.cpu', integ.ExpectedResult.exact("512"));

This also doesn't work:

const ruleDetails = integTest.assertions.awsApiCall('CloudWatchEvents', 'listTargetsByRule', { Rule: rule.ruleName, }); ruleDetails.expect(integ.ExpectedResult.objectLike({ Targets: Match.arrayWith([ Match.objectLike({ Input: Match.objectLike({ cpu: '512', }), }), ]), }));

fails with:

"{\n!! Expected type object but received array\n \"Targets\": [\n { ... },\n { ... },\n { ... }\n ]\n}"}

I even copied the custom resource down and played with a ton of different iterations and nothing works.

I can confirm that the actual state of the integration test is correct. Could you assist me with:

a) providing the proper syntax for the integration test; or,
b) merge the PR without adding the assertion

Thanks in advance for taking a look!

…sk-overrides

aws-cdk-automation · 2025-02-08T23:10:38Z

AWS CodeBuild CI Report

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

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

blimmer added 6 commits November 30, 2024 12:46

feat: add all ecs task properties for overrides

8036fea

refactor: move input to a separate private method

c42d496

feat: add unit tests for ephemeral storage

6050489

feat: test all inputs

ddf3a13

fix: update ec2 integration test

b387072

fix: updatfargate integration test

9c657ae

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Dec 1, 2024

aws-cdk-automation requested a review from a team December 1, 2024 17:34

github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Dec 1, 2024

blimmer commented Dec 1, 2024

View reviewed changes

Merge branch 'main' into add-all-ecs-task-overrides

7a68fed

aws-cdk-automation previously requested changes Dec 1, 2024

View reviewed changes

docs: add README

b65c237

blimmer changed the title ~~feat(events-targets): add all ECS task properties for overrides~~ feat(events-targets): allow all ECS TaskOverrides Dec 1, 2024

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 1, 2024

Merge branch 'main' into add-all-ecs-task-overrides

5c179b5

github-actions bot mentioned this pull request Jan 1, 2025

Monthly PR metrics report #32703

Open

aaythapa reviewed Feb 5, 2025

View reviewed changes

blimmer and others added 3 commits February 7, 2025 08:28

Merge branch 'main' of github.com:blimmer/aws-cdk into add-all-ecs-ta…

92cd8d9

…sk-overrides

fix: test.each

7e60f87

Merge branch 'main' into add-all-ecs-task-overrides

b7f8bb4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(events-targets): allow all ECS TaskOverrides #32344

feat(events-targets): allow all ECS TaskOverrides #32344

blimmer commented Dec 1, 2024

blimmer Dec 1, 2024

blimmer Dec 1, 2024

blimmer Dec 1, 2024

aws-cdk-automation left a comment

codecov bot commented Dec 1, 2024 •

edited

Loading

aaythapa left a comment

aaythapa Feb 5, 2025

blimmer Feb 7, 2025

aaythapa Feb 5, 2025

blimmer Feb 7, 2025

blimmer Feb 8, 2025 •

edited

Loading

aws-cdk-automation commented Feb 8, 2025

	const containerOverrides = this.props.containerOverrides && this.props.containerOverrides
	.map(({ containerName, ...overrides }) => ({ name: containerName, ...overrides }));
	const input = { containerOverrides };

feat(events-targets): allow all ECS TaskOverrides #32344

Are you sure you want to change the base?

feat(events-targets): allow all ECS TaskOverrides #32344

Conversation

blimmer commented Dec 1, 2024

Issue #32217

Reason for this change

Description of changes

Description of how you validated changes

Checklist

blimmer Dec 1, 2024

Choose a reason for hiding this comment

blimmer Dec 1, 2024

Choose a reason for hiding this comment

blimmer Dec 1, 2024

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 1, 2024 • edited Loading

Codecov Report

aaythapa left a comment

Choose a reason for hiding this comment

aaythapa Feb 5, 2025

Choose a reason for hiding this comment

blimmer Feb 7, 2025

Choose a reason for hiding this comment

aaythapa Feb 5, 2025

Choose a reason for hiding this comment

blimmer Feb 7, 2025

Choose a reason for hiding this comment

blimmer Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 8, 2025

AWS CodeBuild CI Report

codecov bot commented Dec 1, 2024 •

edited

Loading

blimmer Feb 8, 2025 •

edited

Loading