feat(ecs): support ecs tag propagation and ecs managed tags #3420

iamhopaul123 · 2019-07-24T19:36:26Z

Add two properties for ECS Service to support ECS tag propagation and ECS managed tags.

Please read the contribution guidelines and follow the pull-request checklist.

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

iamhopaul123 · 2019-07-24T19:37:38Z

@pkandasamy91 please review

piradeepk · 2019-07-24T20:47:09Z

@iamhopaul123 looks like the integration tests need to be updated as well.

piradeepk

Please add unit tests to verify that this works as expected and update integration tests to reflect this change.

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts

packages/@aws-cdk/aws-ecs/lib/fargate/fargate-service.ts

iamhopaul123 · 2019-07-24T21:56:03Z

@iamhopaul123 looks like the integration tests need to be updated as well.

@iamhopaul123 Please add unit tests to verify that this works as expected and update integration tests to reflect this change.

Done!

RomainMuller

This PR seems to introduce behavior-breaking changes, which is undesirable. We can discuss whether this has the potential to actually break users, or if the new behavior is functionally compatible, but my instinct right now is that we should preserve the current behavior.

Also, I couldn't find documentation for the CFN properties that you are now wiring, so I couldn't assess the validity of certain default behaviors (but I suspect they're not what I'll want, because they turn into breaking behavior).

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

RomainMuller · 2019-07-26T08:35:36Z

packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts

@@ -41,6 +41,8 @@ export = {
        DesiredCount: 1,
        LaunchType: LaunchType.FARGATE,
        LoadBalancers: [],
+        EnableECSManagedTags: true,
+        PropagateTags: PropagateTagsFromType.SERVICE,


The fact you had to introduce these means you're making a breaking change. We don't want that.

iamhopaul123 · 2019-07-26T16:44:27Z

@RomainMuller after talking with my teammates, we think that it would not be causing breaking changes for adding default values for those two Service properties. Because from the customers' perspective they don't have a reason for not needing those ECS managed tag. However, we agree we should add "no_propagate" option for those who don't want to propagate tags to tasks.

piradeepk · 2019-08-13T17:19:23Z

@eladb do you have any thoughts on this? Should this be considered a breaking change?

mergify · 2019-08-19T20:23:22Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

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>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

mergify · 2019-08-19T20:23:25Z

Pull Request Checklist

Testing

Unit test added (prefer to add a new test rather than modify existing tests)
CLI change? Re-run/add CLI integration tests

Documentation

Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
README: update module README
Design: for significant features, follow the design process

Title uses the format type(scope): text

Type: fix, feat, refactor go into CHANGELOG, chore is hidden
Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
Style: use all lower-case, do not end with a period

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>

Sensitive Modules (requires 2 PR approvers)

IAM document library (in @aws-cdk/aws-iam)
EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
Grant APIs (if not based on official documentation with a reference)

Pull request has been modified.

piradeepk

Looks good overall, had a couple comments.

packages/@aws-cdk/aws-ecs-patterns/test/fargate/integ.asset-image.expected.json

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts

packages/@aws-cdk/aws-ecs/lib/fargate/fargate-service.ts

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

mergify · 2019-08-22T21:42:54Z

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

piradeepk · 2019-08-22T21:52:52Z

Tests are failing, try to remove the empty ServiceRegistries and LoadBalancers properties. I think it's related to an error in resolving the merge conflicts.

Pull request has been modified.

packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-ecs-deploy.expected.json

packages/decdk/test/__snapshots__/synth.test.js.snap

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

Pull request has been modified.

piradeepk

LGTM

mergify · 2019-08-23T00:58:40Z

Thank you for contributing! Your pull request is now being automatically merged.

iamhopaul123 requested a review from a team as a code owner July 24, 2019 19:36

piradeepk requested changes Jul 24, 2019

View reviewed changes

iamhopaul123 requested a review from skinny85 as a code owner July 24, 2019 23:51

iamhopaul123 force-pushed the tag_ecs branch from af41e2d to 1ec0170 Compare July 25, 2019 17:06

iamhopaul123 mentioned this pull request Jul 25, 2019

design(aws-ecs): tagging support for ecs (WIP) #3440

Closed

RomainMuller requested a review from SoManyHs as a code owner July 26, 2019 08:13

RomainMuller previously requested changes Jul 26, 2019

View reviewed changes

iamhopaul123 force-pushed the tag_ecs branch from 5c151a9 to 0b47bb3 Compare July 26, 2019 16:42

iamhopaul123 force-pushed the tag_ecs branch 2 times, most recently from 87207a7 to 23f5b2e Compare July 29, 2019 23:00

iamhopaul123 added 9 commits July 29, 2019 16:04

Added two new properties for ECS Service

46023fa

Fix comment for PropagateTags

fa9c8ed

Remove Name-path tag for ECS Service

aea988d

Fix properties discription

7c66ee9

Add unit test and fix integ test

b9bb273

Fix integ test for ecs-pattern

65cd3d8

fix pipeline ecs integ test

5f2a363

Update snapshot for synth.test.js

e47056b

Change enum type name and add no_propagate option

49abf90

iamhopaul123 force-pushed the tag_ecs branch from 23f5b2e to 2367d43 Compare July 29, 2019 23:08

Change enum name and fix no propagate setting in fargate service

ae1318d

iamhopaul123 force-pushed the tag_ecs branch from 2367d43 to ae1318d Compare July 29, 2019 23:57

Change default value for propagateTags in comment

c37d846

NGL321 assigned SoManyHs and piradeepk Aug 1, 2019

piradeepk previously approved these changes Aug 6, 2019

View reviewed changes

Merge branch 'master' into tag_ecs

e31ec82

piradeepk and others added 6 commits August 13, 2019 10:19

Merge branch 'master' into tag_ecs

96367cd

Merge branch 'master' into tag_ecs

a0947de

Merge branch 'master' into tag_ecs

46b738b

Merge branch 'master' into tag_ecs

3b0a966

Merge branch 'master' into tag_ecs

9517d38

Merge branch 'master' into tag_ecs

2fe80dc

mergify bot added the automerge-enabled label Aug 19, 2019

Merge branch 'master' into tag_ecs

80930ea

piradeepk previously requested changes Aug 22, 2019

View reviewed changes

piradeepk removed the request for review from skinny85 August 22, 2019 22:02

Fix comments and tests and refactor propagatetags logic

4be8cf8

Merge branch 'master' into tag_ecs

bea7d25

piradeepk previously requested changes Aug 22, 2019

View reviewed changes

Fix propagateTags and enableECSManagedTags comments

473e32d

Sync design doc

d2ea196

piradeepk changed the title ~~feat(ecs): support ECS tag propagation and ECS managed tags~~ feat(ecs): support ecs tag propagation and ecs managed tags Aug 23, 2019

piradeepk approved these changes Aug 23, 2019

View reviewed changes

mergify bot merged commit 1e81053 into aws:master Aug 23, 2019

piradeepk mentioned this pull request Aug 30, 2019

fix(ecs): default ecsmanagedtags and propagatetags to be undefined #3887

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecs): support ecs tag propagation and ecs managed tags #3420

feat(ecs): support ecs tag propagation and ecs managed tags #3420

iamhopaul123 commented Jul 24, 2019 •

edited

Loading

iamhopaul123 commented Jul 24, 2019 •

edited

Loading

piradeepk commented Jul 24, 2019

piradeepk left a comment

iamhopaul123 commented Jul 24, 2019

RomainMuller left a comment

RomainMuller Jul 26, 2019

iamhopaul123 commented Jul 26, 2019 •

edited

Loading

piradeepk commented Aug 13, 2019

mergify bot commented Aug 19, 2019

mergify bot commented Aug 19, 2019 •

edited by iamhopaul123

Loading

piradeepk left a comment

mergify bot commented Aug 22, 2019

piradeepk commented Aug 22, 2019

piradeepk left a comment

mergify bot commented Aug 23, 2019

feat(ecs): support ecs tag propagation and ecs managed tags #3420

feat(ecs): support ecs tag propagation and ecs managed tags #3420

Conversation

iamhopaul123 commented Jul 24, 2019 • edited Loading

iamhopaul123 commented Jul 24, 2019 • edited Loading

piradeepk commented Jul 24, 2019

piradeepk left a comment

Choose a reason for hiding this comment

iamhopaul123 commented Jul 24, 2019

RomainMuller left a comment

Choose a reason for hiding this comment

RomainMuller Jul 26, 2019

Choose a reason for hiding this comment

iamhopaul123 commented Jul 26, 2019 • edited Loading

piradeepk commented Aug 13, 2019

mergify bot commented Aug 19, 2019

Pull Request Checklist

mergify bot commented Aug 19, 2019 • edited by iamhopaul123 Loading

Pull Request Checklist

piradeepk left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 22, 2019

piradeepk commented Aug 22, 2019

piradeepk left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 23, 2019

iamhopaul123 commented Jul 24, 2019 •

edited

Loading

iamhopaul123 commented Jul 24, 2019 •

edited

Loading

iamhopaul123 commented Jul 26, 2019 •

edited

Loading

mergify bot commented Aug 19, 2019 •

edited by iamhopaul123

Loading