From e15d391898c986230b5bbe4dd172d3354dd78f29 Mon Sep 17 00:00:00 2001
From: Penghao He <penghaoh@amazon.com>
Date: Mon, 29 Jul 2019 14:30:18 -0700
Subject: [PATCH] feat(core): improved API for tags (#3465)

Improved tagging API.
---
 design/tagging-API-change.md                  | 73 +++++++++++++++++++
 packages/@aws-cdk/core/lib/tag-aspect.ts      | 16 +++-
 .../@aws-cdk/core/test/test.tag-aspect.ts     | 27 +++++++
 3 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 design/tagging-API-change.md

diff --git a/design/tagging-API-change.md b/design/tagging-API-change.md
new file mode 100644
index 0000000000000..6663c83574afc
--- /dev/null
+++ b/design/tagging-API-change.md
@@ -0,0 +1,73 @@
+# Tagging API Change
+
+CDK support tagging and can cascade tags to all its taggable children (see [here](https://docs.aws.amazon.com/cdk/latest/guide/tagging.html)). The current CDK tagging API is shown below:
+
+``` ts
+myConstruct.node.applyAspect(new Tag('key', 'value'));
+
+myConstruct.node.applyAspect(new RemoveTag('key', 'value'));
+```
+
+As we can see, the current tagging API is not nice and grammatically verbose for using, since there is no reason to expose `node` to users and `applyAspect` does not indicate anything towards tags, which leaves room for improvement. Also, users need to create two objects to add tag and remove tag which causes confusion to some degree.
+
+## General approach
+
+For the tagging behavior part, we propose using just one entry point `Tag` for the new tagging API:
+
+``` ts
+Tag.add(myConstruct, 'key', 'value');
+
+Tag.remove(myConstruct, 'key');
+```
+
+## Code changes
+
+Given the above, we should make the following changes:
+  1. Add two methods `add` and `remove` to `Tag` class, which calls `applyAspect`to add tags or remove tags.
+
+# Part1: Change CDK Tagging API
+
+Implementation for the new tagging API is shown below:
+
+``` ts
+/**
+ * The Tag Aspect will handle adding a tag to this node and cascading tags to children
+ */
+export class Tag extends TagBase {
+
+  /**
+   * add tags to the node of a construct and all its the taggable children
+   */
+  public static add(scope: Construct, key: string, value: string, props: TagProps = {}) {
+    scope.node.applyAspect(new Tag(key, value, props));
+  }
+
+  /**
+   * remove tags to the node of a construct and all its the taggable children
+   */
+  public static remove(scope: Construct, key: string, props: TagProps = {}) {
+    scope.node.applyAspect(new RemoveTag(key, props));
+  }
+
+  ...
+}
+```
+
+And below is an example use case demonstrating how the adjusted tagging API works:
+
+``` ts
+// Create Task Definition
+const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDef');
+
+// Create Service
+const service = new ecs.Ec2Service(stack, "Service", {
+  cluster,
+  taskDefinition,
+});
+
+Tag.add(taskDefinition, 'tfoo', 'tbar');
+Tag.remove(taskDefinition, 'foo', 'bar');
+
+Tag.add(service, 'sfoo', 'sbar');
+Tag.remove(service, 'foo', 'bar');
+```
diff --git a/packages/@aws-cdk/core/lib/tag-aspect.ts b/packages/@aws-cdk/core/lib/tag-aspect.ts
index b21a8806c571e..0c2a2b0d7eeaa 100644
--- a/packages/@aws-cdk/core/lib/tag-aspect.ts
+++ b/packages/@aws-cdk/core/lib/tag-aspect.ts
@@ -1,6 +1,6 @@
 // import cxapi = require('@aws-cdk/cx-api');
 import { IAspect } from './aspect';
-import { IConstruct } from './construct';
+import { Construct, IConstruct } from './construct';
 import { ITaggable, TagManager } from './tag-manager';
 
 /**
@@ -85,6 +85,20 @@ abstract class TagBase implements IAspect {
  */
 export class Tag extends TagBase {
 
+  /**
+   * add tags to the node of a construct and all its the taggable children
+   */
+  public static add(scope: Construct, key: string, value: string, props: TagProps = {}) {
+    scope.node.applyAspect(new Tag(key, value, props));
+  }
+
+  /**
+   * remove tags to the node of a construct and all its the taggable children
+   */
+  public static remove(scope: Construct, key: string, props: TagProps = {}) {
+    scope.node.applyAspect(new RemoveTag(key, props));
+  }
+
   /**
    * The string value of the tag
    */
diff --git a/packages/@aws-cdk/core/test/test.tag-aspect.ts b/packages/@aws-cdk/core/test/test.tag-aspect.ts
index 6b755616e3536..dcd1ffe27f08d 100644
--- a/packages/@aws-cdk/core/test/test.tag-aspect.ts
+++ b/packages/@aws-cdk/core/test/test.tag-aspect.ts
@@ -107,6 +107,33 @@ export = {
     test.deepEqual(res2.tags.renderTags(), [{key: 'first', value: 'there is only 1'}]);
     test.done();
   },
+  'add will add a tag and remove will remove a tag if it exists'(test: Test) {
+    const root = new Stack();
+    const res = new TaggableResource(root, 'FakeResource', {
+      type: 'AWS::Fake::Thing',
+    });
+    const res2 = new TaggableResource(res, 'FakeResource', {
+      type: 'AWS::Fake::Thing',
+    });
+    const asg = new AsgTaggableResource(res, 'AsgFakeResource', {
+      type: 'AWS::Fake::Thing',
+    });
+
+    const map = new MapTaggableResource(res, 'MapFakeResource', {
+      type: 'AWS::Fake::Thing',
+    });
+    Tag.add(root, 'root', 'was here');
+    Tag.add(res, 'first', 'there is only 1');
+    Tag.remove(res, 'root');
+    Tag.remove(res, 'doesnotexist');
+    ConstructNode.prepare(root.node);
+
+    test.deepEqual(res.tags.renderTags(), [{key: 'first', value: 'there is only 1'}]);
+    test.deepEqual(map.tags.renderTags(), {first: 'there is only 1'});
+    test.deepEqual(asg.tags.renderTags(), [{key: 'first', value: 'there is only 1', propagateAtLaunch: true}]);
+    test.deepEqual(res2.tags.renderTags(), [{key: 'first', value: 'there is only 1'}]);
+    test.done();
+  },
   'the #visit function is idempotent'(test: Test) {
     const root = new Stack();
     const res = new TaggableResource(root, 'FakeResource', {