feat(aws-ecs-patterns): Add tagList and propagateTags to scheduled tasks

[rdbatch]

Note: I set out to add this to the aws-ecs-patterns package but that required adding support to aws-events-targets as well. Let me know if this should be separated into two separate PRs or if it can be managed through this single one.

This adds the ability to pass a tagList and the propagateTags flag to scheduled EC2 and Fargate ECS tasks. Users can leverage these attributes to apply tags an ECS task that's triggered through a scheduled Event. Both of these attributes are defined in the EcsParameters for a Rule https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-events-rule-ecsparameters.html.

This is a non-breaking change. These new fields are added as optional with default values such that it won't impact existing stacks.

This closes #9823.

---

All Submissions:

• Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

• This PR adds new construct runtime dependencies following the process described here

New Features

• Have you added the new feature to an integration test?
  • 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

[gitpod-io]

[rdbatch]

I did see that there was previously a PR opened for this in the past which was closed for staleness (#19583). There was a comment on there asking if this could be added to the existing tag system (#19583 (review)). I'm not sure if that would make sense in this case since the resource being created at deploy-time by CloudFormation isn't the resource being tagged. The event rule itself is instead being given the flag to pass along tags onto the Task that it's creating at some time in the future when the schedule triggers.

Happy to discuss this further if the CDK team disagrees, but from an implementation perspective it seems like it would make sense to keep the tagList and propagateTags fields as configuration attributes on scheduled task to more closely map to how it's being created by CloudFormation.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[TheRealAmazonKendra]

Thank you for your contribution! Please do separate this into two PRs so that we can give it a better quality review. It's just too easy to miss small details when PRs are this big.

My initial feedback, however, is that propagateTags seems to rely on tagList having values supplied. That interdependency should be baked into the contract.

Also, just a question here but in my quick scroll, I see that you're using CfnTag instead of a Tag resource. Would it be feasible to implement the resource so that we didn't have use the underlying L1 there?

As I mentioned, I just scrolled really quickly because I knew I was going to ask you to break this up. We'll provide a much more meaningful review once that has been done.

Appreciate all your work here!

[rdbatch]

Thanks @TheRealAmazonKendra for taking a look! I understand where you're coming from in terms of splitting it up into two separate PRs (one for the changes to aws-events-targets and one for aws-ecs-patterns). I've submitted the first one as #23838 and I can submit the other one pending the first being merged since it's dependent on those changes.

My initial feedback, however, is that propagateTags seems to rely on tagList having values supplied. That interdependency should be baked into the contract.

I've clarified this in the description for the second PR, but these are separate fields that can be passed independently of each other and aren't interdependent.

Also, just a question here but in my quick scroll, I see that you're using CfnTag instead of a Tag resource. Would it be feasible to implement the resource so that we didn't have use the underlying L1 there?

Good call out, this was a miss on my part and was updated in the new PR. Thanks.

With that, we can probably close out this PR and move any subsequent conversation to the new one.