This code isn't needed. As mentioned in #30751, the task definition without the trailing :* isn't a valid ARN.

Hi @msambol , thanks for taking time to review. It is possible for taskDefinitionArn to have a revision number that isn't *, hence, this if block. When there is already a revision number, we shouldn't append :*. I left a comment in the PR you linked about this case.

The * means it will match all revision numbers (it's a wildcard).

Yes. That is correct. It is also possible that customer may want to restrict the IAM policy resource to specific revision number.

Sample code of such case where customer is specifying the revision number MyTask:19:

import { Duration, Stack, type StackProps } from 'aws-cdk-lib';
import { Cluster, FargateTaskDefinition, NetworkMode } from 'aws-cdk-lib/aws-ecs';
import { Rule, Schedule } from 'aws-cdk-lib/aws-events';
import { EcsTask } from 'aws-cdk-lib/aws-events-targets';
import { Role } from 'aws-cdk-lib/aws-iam';
import { Construct } from 'constructs';

export class TestCdkStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    const cluster = new Cluster(this, 'Cluster');
    const taskDefinition = FargateTaskDefinition.fromFargateTaskDefinitionAttributes(this, "TaskDefImport", {
      taskDefinitionArn: "arn:aws:ecs:us-west-2:123456789101:task-definition/MyTask:19",
      taskRole: Role.fromRoleArn(this, "RoleImport", "arn:aws:iam::123456789101:role/MyTaskRole"),
      networkMode: NetworkMode.AWS_VPC,
    });
    new Rule(this, "Rule", {
      targets: [new EcsTask({
        cluster,
        taskDefinition
      })],
      schedule: Schedule.rate(Duration.days(1)),
    });
  }
}

apologies! I'm with you now. In https://github.com/msambol/aws-cdk/blob/main/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/ecs/run-task.ts, the revision number is stripped out.

No worries. Appreciate the time for checking. I will open an issue about the StepFunction ECS task.

That said, could you move this code to a function and write unit tests for it? I think it'd be easier to follow. Similar to https://github.com/msambol/aws-cdk/blob/main/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/ecs/run-task.ts#L379-L396.

    let needsRevisionWildcard = false;
    if (!cdk.Token.isUnresolved(this.taskDefinition.taskDefinitionArn)) {
      const revisionAtEndPattern = /:[0-9]+$/;
      const hasRevision = revisionAtEndPattern.test(this.taskDefinition.taskDefinitionArn);
      needsRevisionWildcard = !hasRevision;
    }

Obviously different purpose of the function but similar pattern.

Moving it out to a public method to allow unit testing means exposing it for potential public use, which will become a API contract that cannot have any breaking change. I would prefer keeping it private and local in this method for now. The unit tests added in this PR are primarily focusing on this block of code.

Does this policy have the trailing :* ?

This uses a CloudFormation (CFN for short) Ref token to reference the TaskDefinition resource. According to the CFN doc, the token will resolve to the TaskDefinition ARN >with< the revision number (but not *).

👍🏼 – I wanted to make sure there was either a revision number or a * at the end.

@samson-keung I think this integration test should in aws-event-targets , no ?

You are right! Let me move it.

This is what I wanted to see 👍🏼

nit: could you change the verbiage here? Based on the name, I'm not certain what it's testing.

Moving it out to a public method to allow unit testing means exposing it for potential public use, which will become a API contract that cannot have any breaking change. I would prefer keeping it private and local in this method for now. The unit tests added in this PR are primarily focusing on this block of code.