feat(ecs-patterns): support propagateTags and ecsManagedTags

[piradeepk]

Add propagateTaskTagsFrom and EcsManagedTags properties to constructs in ecs-patterns.

Addresses: #3979

BREAKING CHANGE:

The property PropagateTags exists in the BaseServiceOptions, while both Ec2Service and FargateService have a property PropagateTaskTagsFrom that's mapped to PropagateTags. Having both is confusing for users and doesn't really make sense to have both configurable for Ec2Service and FargateService.

---

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

[mergify]

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

• PR title type(scope): text
  • type: fix, feat, refactor go into CHANGELOG, chore is hidden
  • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
  • text: use all lower-case, do not end with a period, do not include issue refs
• PR Description
  • Rationale: describe rationale of change and approach taken
  • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
  • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
• Testing
  • Unit test added. Prefer to add a new test rather than modify existing tests
  • CLI or init templates change? Re-run/add CLI integration tests
• Documentation
  • README: update module README to describe new features
  • API docs: public APIs must be documented. Copy from official AWS docs when possible
  • Design: for significant features, follow design process

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: FAILED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[rix0rrr]

I'm not sure the breaking change here is justified. @deprecate the old property (while still supporting it!), you'll get a chance to remove it when we go 2.x.

[rix0rrr]

That is to say, the "breaking change escape hatch" we have is not to be used willy-nilly. It's there if we've painted ourselves into a corner that we REALLY have no other way of getting out of. A stable API should be treated as such, or the guarantee means nothing.

[piradeepk]

That is to say, the "breaking change escape hatch" we have is not to be used willy-nilly. It's there if we've painted ourselves into a corner that we REALLY have no other way of getting out of. A stable API should be treated as such, or the guarantee means nothing.

I understand, IMO this breaking change would actually provide a better experience for users, as it wouldn't work today if they used that property.

As you can see in the Ec2Service construct: https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs/lib/ec2/ec2-service.ts#L138, propagateTags is passed in the value from propagateTaskTagsFrom. In this case, if a user passes in a value for propagateTags but is undefined for propagateTaskTagsFrom, their service will only have the value undefined. To resolve this and provide a better user experience, I decided to move the property out of the higher level service constructs.

I wasn't removing the property altogether, but rather moving it into the BaseServiceProps out of the BaseServiceOptions. Ec2Service/FargateService constructs already have the property propagateTaskTagsFrom and extend BaseServiceOptions.

Let me know if you disagree and think this should be approached another way?

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[rix0rrr]

I guess you prefer the term propagateTagsFrom, but if you just called it propagateTags and passed from the subclass to the superclass, everything would work as expected without the breaking API change, no?

[piradeepk]

I guess you prefer the term propagateTagsFrom, but if you just called it propagateTags and passed from the subclass to the superclass, everything would work as expected without the breaking API change, no?

Either way, we would have 2 properties that mean the same thing available to be used in the subclass.

The properties that are currently defined:

propagateTaskTagsFrom
https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs/lib/fargate/fargate-service.ts#L58
https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs/lib/ec2/ec2-service.ts#L80
propagateTags
https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs/lib/base/base-service.ts#L88

Users can see both properties when they try to create instances of FargateService and Ec2Service. Today, we only use propagateTaskTagsFrom:
https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs/lib/ec2/ec2-service.ts#L138
https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs/lib/fargate/fargate-service.ts#L97

I think we have 2 choices:

1. Leave it as is, and add logic that will throw an error if both are populated, and use the value in either to pass it to the super class (avoid a breaking change, but IMO not clean, and confusing for users)
2. Move the PropagateTags property to BaseServiceProps instead of BaseServiceOptions (Breaking change, but avoids confusion in the long term)

Thoughts?

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[rix0rrr]

1. Leave it as is, and add logic that will throw an error if both are populated, and use the value in either to pass it to the super class (avoid a breaking change, but IMO not clean, and confusing for users)

Yep, and we will @deprecate one of them (recommending the other).

I agree that it's not clean, but we have API stability for a reason. Again, we can clean up when we release 2.x. It's unfortunate that we let this slip through code review, but maybe it'll serve as a good lesson for the future.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: FAILED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[piradeepk]

1. Leave it as is, and add logic that will throw an error if both are populated, and use the value in either to pass it to the super class (avoid a breaking change, but IMO not clean, and confusing for users)

Yep, and we will @deprecate one of them (recommending the other).

I agree that it's not clean, but we have API stability for a reason. Again, we can clean up when we release 2.x. It's unfortunate that we let this slip through code review, but maybe it'll serve as a good lesson for the future.

Updated the PR to implement the check, we can decide which property to deprecate and update that separately.

[piradeepk]

@rix0rrr updated the PR to deprecate the propagateTaskTagsFrom properties.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[rix0rrr]

Ah, conflict unfortunately :(

[piradeepk]

Resolved conflicts and pushed!

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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