Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): improved API for tags #3465

Merged
merged 3 commits into from
Jul 29, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions design/tagging-API-change.md
Original file line number Diff line number Diff line change
@@ -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 = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since tags are essentially maps, why not something like this:

Tag.add(scope, { sfoo: 'bar' })

This will allow people to add multiple tags like this:

Tag.add(scope, {
  tag1: 'value1',
  tag2: 'value2'
});

Copy link
Contributor Author

@iamhopaul123 iamhopaul123 Jul 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But tag also supports other properties like priority. See here. I would suggest Tag.add(scope, [
(tag1, 'value1', props)
(tag2, 'value2', props)
]);
but not sure if that's an over-engineer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, let's just leave it with Tag.add(scope, key, value[, options)

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');
```
16 changes: 15 additions & 1 deletion packages/@aws-cdk/core/lib/tag-aspect.ts
Original file line number Diff line number Diff line change
@@ -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';

/**
Expand Down Expand Up @@ -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
*/
Expand Down
27 changes: 27 additions & 0 deletions packages/@aws-cdk/core/test/test.tag-aspect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', {
Expand Down