feat(cdk): aspect framework and tag implementation #1451

moofish32 · 2018-12-29T04:46:42Z

A generalized aspect framework is added. Aspects can be used to affect the construct tree without modifying the class hierarchy. This framework is designed to be generic for future use cases. Tagging is the first implementation.

Tagging has been reimplemented to leverage the aspect framework and simplify the original tag design. Tag Manager still exists, but is no longer intended for use by L2 construct authors. There are two new classes cdk.Tag and cdk.RemoveTag. As new resources are added tag support will be automatic as long as they implement one of the existing tag specifications. All L2 constructs have removed the TagManager.

Code generation has been modified to add tag support to any CloudFormation Resource with a matching specification, by creating a Tag Manager for that resource as a tags attribute. The schema code now includes the ability to detect 3 forms of tags which include the current CloudFormation Resources.

BREAKING CHANGE: if you are using TagManager the API for this object has completely changed. You should no longer use TagManager directly, but instead replace this with Tag Aspects. cdk.Tag has been renamed to cdk.CfnTag to enable cdk.Tag to be the Tag Aspect.

Fixes #1136
Fixes #1497

Pull Request Checklist

Testing
- Unit test added
- CLI change?: manually run integration tests and paste output as a PR comment
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

eladb · 2018-12-30T11:19:20Z

packages/@aws-cdk/aws-ec2/lib/vpc.ts

-    this.tags.setTag(NAME_TAG, this.path, { overwrite: false });
+    // set first and let initial tags overwrite
+    this.apply(new cdk.TagAspect(NAME_TAG, this.path));
+    const tags = props.tags || {};


Wasn't the whole idea if the aspect system that we wouldn't need to explicitly support tags in each construct?

This being a draft, I had left this for discussion about how much of a hard breaking change we want. I can remove all instances of tags and TagManager if we think this design is correct. I didn't want to overwrite the current PR just yet as I felt @rix0rrr and I were not quite on the same page yet.

eladb · 2018-12-30T11:20:19Z

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts

+  }
+}
+
+export class RemoveTag extends TagBaseAspect {


Probably should have an Aspect suffix

eladb · 2018-12-30T11:20:36Z

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts

+  exclude?: string[];
+}
+
+export abstract class TagBaseAspect implements IAspect {


I'd call this TagAspectBase (that's the convention we have for abstract base classes)

eladb · 2018-12-30T11:22:03Z

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts

+        child.apply(this);
+      }
+    }
+    if (node instanceof Resource && (node as Resource).tagType !== TagType.NotTaggable) {


Use of instanceof is not allowed since breaks when different version of Resource are used. Use duck-typing to identify if the type adheres to the interface you are after (e.g. does it have a tagType property?).

Will change, good information that I didn't know.

eladb · 2018-12-30T11:22:44Z

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts

+  }
+
+  public visit(node: Construct): void {
+    if (this.visitedBy[node.uniqueId] === true) {


Is that something we can add to the framework instead of the aspect itself?

I was debating this, will move.

eladb · 2018-12-30T11:25:21Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

+   *
+   * This object is rendered via a call to "renderProperties(this.properties)".
+   */
+  public readonly properties: any;


We don't want this to be public.

Yep, I'm going to have refactor renderProperties to make a super call in the generated code. I'll propose something in the next refactor.

eladb · 2018-12-30T11:25:37Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

@@ -239,6 +242,47 @@ export class Resource extends Referenceable {
  }
 }

+export enum TagType {
+  Standard = "StandardTag",


style: Use single quotes

eladb · 2018-12-30T11:26:17Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

+/**
+ * Handles converting TagManager tags to AutoScalingGroupTags
+ */
+export class AutoScalingGroupTagFormatter implements ITagFormatter {


Can we make all these formatters internal to the aspect code? No one else should touch those

Yes they are already in the aspect this is just not cleaned up yet do to my rush on the draft.

Yes, these are redundant now, right?

eladb · 2018-12-30T11:27:51Z

packages/@aws-cdk/cdk/lib/core/construct.ts

+   * TODO docs
+   * can this be private?
+   */
+  public visit(): void {


Not sure about apply and visit. "apply" is not actually applying (it's "attaching") and "visit" is actually applying.

agree - even in Wikipedia they call apply Accept and Visit does the work. I'm open to name changes.

My naming suggestion here is semantic meaning from the perspective of the user, not mechanic meaning from the perspective of the implementor.

For the user, it looks like they are applying a tag to a construct, and the word apply is nice and generic enough that we can apply it (hah) to other types of aspects as well.

A mechanically correct alternative name would be attachAspect(), but it doesn't read as nicely.

Compare:

// Customer first (what it does) construct.apply(new cdk.Tag('key', 'value')); // Implementor first (how it works) construct.attachAspect(new cdk.TagAspect('key', 'value'));

At least, im(ns)ho :)

Also, I wouldn't call this particular function visit(), I would call this something like invokeAspects().

And yes, it shouldn't be public, that will only serve to confuse people. It's not too bad, it should be invoked from App.run() which is itself a construct so it can access protected members.

eladb · 2018-12-30T11:28:06Z

packages/@aws-cdk/cdk/lib/core/construct.ts

+ * Aspects that can be applied to Constructs.
+ */
+export interface IAspect {
+  readonly type: string;


Why do we need this "type"?

This type was included so we could filter on aspect types. For example, TagAspects check to see if there is another TagAspect that takes precedence. It's hard because parents and children can apply tags out of order and I had to rely on more than array order. There are other designs that could be done to make this work. For example, we could make the TagAspect collect all others TagAspects into an array and write a comparison function to always sort priority.

rix0rrr · 2018-12-31T09:57:05Z

packages/@aws-cdk/aws-ec2/lib/vpc.ts

+    // set first and let initial tags overwrite
+    this.apply(new cdk.TagAspect(NAME_TAG, this.path));
+    const tags = props.tags || {};
+    for (const key of Object.keys(tags)) {


I'm not sure if it makes sense to support tags in the props object anymore.

rix0rrr · 2018-12-31T09:59:52Z

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts

+
+export interface TagProperties {
+  // TODO docs
+  propagate?: number;


Still not sure that propagate makes sense, and the number is "how deep"? All this machinery is purpose-built to allow selective tagging for VPCs, given that we know its internal construct layout, isn't it?

rix0rrr · 2018-12-31T10:01:37Z

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts

+    }
+    this.visitedBy[node.uniqueId] = true;
+    const propagate = this.propagate > 0 ? this.propagate - 1 : this.propagate;
+    this.parent = this.parent || node;


If you need this, it should also become part of the framework.

f.ex:

public visit(currentNode: Construct, applicationNode: Construct) {

fwiw I think it makes sense.

I'll review and incorporate if the other suggestions don't make this obsolete.

rix0rrr · 2018-12-31T10:02:08Z

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts

+    if (propagate !== 0) {
+      for (const child of node.children) {
+        this.propagate = propagate;
+        child.apply(this);


Agreed, I think the framework should recursively apply.

rix0rrr · 2018-12-31T10:04:30Z

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts

+  }
+}
+
+export class TagAspect extends TagBaseAspect {


I know this is going to be contentious but I think I'd prefer this to not be suffixed with Aspect.

construct.apply(new cdk.Tag('key', 'value'));

Reads better to me.

I'll agree this reads better and I originally I started here. For this draft I didn't want to rename the current cdk.cloudformation.Tag class that we have. I did some looking and the specific class is mostly used in generated code. Would we prefer that we rename this to CfnTag. I realize Cfn<resource> now has special meaning and I think this is similar, but not identical. I also felt like the fact apply (or another name we choose) will already set the mental context of the developer that this is an Aspect and suffix is not necessary.

rix0rrr · 2018-12-31T10:16:51Z

packages/@aws-cdk/cdk/lib/core/construct.ts

+   * TODO docs
+   * can this be private?
+   */
+  public visit(): void {


My naming suggestion here is semantic meaning from the perspective of the user, not mechanic meaning from the perspective of the implementor.

For the user, it looks like they are applying a tag to a construct, and the word apply is nice and generic enough that we can apply it (hah) to other types of aspects as well.

A mechanically correct alternative name would be attachAspect(), but it doesn't read as nicely.

Compare:

// Customer first (what it does) construct.apply(new cdk.Tag('key', 'value')); // Implementor first (how it works) construct.attachAspect(new cdk.TagAspect('key', 'value'));

At least, im(ns)ho :)

rix0rrr · 2018-12-31T10:18:55Z

packages/@aws-cdk/cdk/lib/core/construct.ts

+   * TODO docs
+   * can this be private?
+   */
+  public visit(): void {


Also, I wouldn't call this particular function visit(), I would call this something like invokeAspects().

And yes, it shouldn't be public, that will only serve to confuse people. It's not too bad, it should be invoked from App.run() which is itself a construct so it can access protected members.

rix0rrr · 2018-12-31T10:23:53Z

packages/@aws-cdk/cdk/lib/core/tag-manager.ts

+ * specialized CloudFormation Resources (e.g. AutoScalingGroups, Serverless and
+ * DAX)
+ */
+export interface ITagFormatter {


Deprecated, right?

rix0rrr · 2018-12-31T10:26:34Z

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts

+    if (propagate !== 0) {
+      for (const child of node.children) {
+        this.propagate = propagate;
+        child.apply(this);


Also, in my head the Tags where physically attached only to the construct the user apply()d them to, and only during visit()ation would they also visit() downward in the tree. Because... (:asterisk:, see below)

rix0rrr · 2018-12-31T10:35:29Z

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts

+
+  protected abstract applyTag(resource: Resource): void;
+
+  private hasPrecedence(aspects: IAspect[]): boolean {


*️⃣, if we apply in order top-down in the tree, tags applied lower in the tree will naturally overwrite the tags applied higher in the tree, without having to write this function. Let's assume the following tree:

┌────────────────┐ │ │ │ Parent │ Tag(key, value1) │ │ └────────────────┘ │ ▼ ┌────────────────┐ │ │ │ Child │ Tag(key, value2) │ │ └────────────────┘ │ ▼ ┌────────────────┐ │ │ │TaggableResource│ │ │ └────────────────┘

This would resolve in the following way:

The tag with value1 would apply first. It would try to apply to Parent (doesn't do anything), then it would try to apply to Child (doesn't do anything), then it would apply to TaggableResource (sets key: value1).

Next, the tag with value2 would apply. It would try to apply to Child (doesn't do anything), then it would apply to TaggableResource. The value for tag key would be overwritten with value2.

Presto, the more specific tag has taken precedence without any extra work on the aspect implementor's part.

I think you are correct. Let me work this in and see if my test cases still work.

moofish32 · 2019-01-02T06:24:18Z

@rix0rrr @eladb thanks for the comments. I hope to have them addressed tomorrow. Neither of you commented on include/exclude API. I've debated some name changes, but I think @rix0rrr is indicating we can kill propagate as it was previously defined. I believe the include/exclude feature handles my selective application needs (without needing propagate). Any additional thoughts on include/exclude or should I interpret the lack of comment as support?

moofish32

Feedback on comments inline.

moofish32 · 2019-01-02T06:56:45Z

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts

+    }
+    this.visitedBy[node.uniqueId] = true;
+    const propagate = this.propagate > 0 ? this.propagate - 1 : this.propagate;
+    this.parent = this.parent || node;


I'll review and incorporate if the other suggestions don't make this obsolete.

rix0rrr · 2019-01-02T09:32:57Z

Neither of you commented on include/exclude API [...] I believe the include/exclude feature handles my selective application needs

I would prefer a more generic selector mechanism that also works for other aspects. However I recognize that we don't have a great solution for this right now, and at least it doesn't harm to have a mechanism inside the Tag aspect system as well; we can always refactor later. So I'm okay with keeping it in for now.

rix0rrr · 2019-01-02T09:36:29Z

And yes, it shouldn't be public, that will only serve to confuse people. It's not too bad, it should be invoked from App.run() which is itself a construct so it can access protected members.

That may get you into trouble for unit tests, btw. But in those cases we can always do the pragmatic thing of casting to any to get rid of any type checking :) or declaring a public method on App which does the "pre-synthesis" work of invoking aspects, and rewriting tests to use App explicitly.

* The base Aspect class is added and is an idempotent propagating visitor, that recurses during the visit action * The first aspects are for tags * TagManager has been re-implemented to only handle setting removing and formatting tags * Tags can be included or excluded based on Resource Type, this replaces the the need for all previous tag properties * VPC, Security Group, KMS Keys, and AutoScaling are updated to use tags * Tag Aspects overwrite tags passed into the L1 during initialization

moofish32

After some tricky merges/rebases I think I've addressed the comments. @eladb @rix0rrr at times you guys had conflicting advice. I tried to favor simple code and blend the advice.

I owe some more docs and test improvements.

moofish32 · 2019-01-14T05:44:49Z

packages/@aws-cdk/cdk/test/aspects/test.tag-aspect.ts

+    test.deepEqual(map.tags.renderTags(), {foo: 'bar'});
+    test.done();
+  },
+  'Aspects are mutually exclusive with tags created by L1 Constructor'(test: Test) {


This is an important behavior to note. I could make this more complicated and solve merges. However, I think it's easy to move to the new Tag and keeps the code simple.

packages/@aws-cdk/cdk/test/core/test.tag-manager.ts

* The base Aspect class is added and is an idempotent propagating visitor, that recurses during the visit action * The first aspects are for tags * TagManager has been re-implemented to only handle setting removing and formatting tags * Tags can be included or excluded based on Resource Type, this replaces the the need for all previous tag properties * VPC, Security Group, KMS Keys, and AutoScaling are updated to use tags * Tag Aspects overwrite tags passed into the L1 during initialization

…are involved

moofish32 · 2019-01-16T06:56:43Z

Ok I'm at test passing, unit test complete, and jsdocs complete.

I added a TestStack to assert that will handle the invoking of aspects. Plus it appeared we were creating this entity in a lot of places. I also cleaned up some left over tslint flags as I was searching for my own. I had to update the ECS tests because it appears the tag from the ASG wasn't reaching the child Lambdas. I think this was a bug, however, if that was intentional we can revert.

I didn't create any documentation for Aspect README yet. Where should I put that and should I write it?

moofish32 · 2019-01-16T08:32:17Z

packages/@aws-cdk/assert/lib/test-stack.ts

@@ -0,0 +1,7 @@
+import cdk = require('@aws-cdk/cdk');
+
+export class TestStack extends cdk.Stack {


Is this a good idea to put in a reusable place?

I started to find all the places we should use this, but I missed a few and now I'm questioning if this is too much to include here?

I think this class is no longer needed but the test clean up seems like it was a good idea?

eladb · 2019-01-16T09:44:21Z

packages/@aws-cdk/assert/lib/test-stack.ts

+
+export class TestStack extends cdk.Stack {
+  public testInvokeAspects() {
+    this.invokeAspects();


Should we move invokeAspects behind the all new “prepareTree”?

Yes I think that makes sense.

Just for clarity, I am doing:

remove invokeAspects() from app.ts

putting invokeAspects() inside of prepareTree()

removing the TestStack because prepareTree() is public and called during expect

Is there any concern about the conditional wrapped around the block that calls prepareTree()?

if (!skipValidation) { // any concern here - I haven't found any but just double checking // Do a prepare-and-validate run over the given stack stack.node.prepareTree(); const errors = stack.node.validateTree(); if (errors.length > 0) { throw new Error(`Stack validation failed:\n${errors.map(e => `${e.message} at: ${e.source.node.scope}`).join('\n')}`); } } // this will be removed if (isTestStack(stack)) { stack.testInvokeAspects(); } // end of block to be removed

A simple thumbs up to verify is fine.

Lgtm; Curious why prepare tree is skipped if validation is skipped but that’s for a different issue (raise one?).

eladb · 2019-01-16T09:47:05Z

packages/@aws-cdk/cdk/lib/aspects/aspect.ts

+   * Node will only be visited once.
+   */
+  public visit(construct: IConstruct): void {
+    if (this.visitedBy[construct.node.uniqueId] === true) {


I’d rename this to “visitTree” and “visitAction” to “visit”

A little clarification. Does this also mean change:

/** * Represents an Aspect */ export interface IAspect { /** * The type of Aspect */ readonly type: string; /** * All aspects can visit by IConstructs */ visit(node: IConstruct): void; }

to

/** * Represents an Aspect */ export interface IAspect { /** * The type of Aspect */ readonly type: string; /** * All aspects can visit the construct tree starting from this construct */ visitTree(node: IConstruct): void; }

I wouldn’t say so. I think aspects are only expected to deal with their own visit. The tree visitation is handled by the framework, no?

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts

eladb · 2019-02-04T20:34:06Z

Please update the PR title + description to match the commit description you wish us to merge eventually

EC2 task definitions can now be used as CloudWatch event targets. ALSO IN THIS COMMIT * Improve hash calculation of Docker images. * Add `grantPassRole()` method to iam.Role Fixes aws#1370.

Add source map support to the TypeScript app entry point template. This won't retroactively fix existing CDK apps, but it will ensure that apps generated in the future via `cdk init --app` will have it enabled. Fixes aws#1579

The property was modeled in the wrong place, rendering it unusable. Moved it to the correct location instead, and added a validation test. Fixes aws#1608

Constructs can now take a dependency on any other construct. Before, only `Resource`s could take dependencies, and they would depend on `IDependable` which had to be implemented explicitly. In this change we generalize the concept of dependencies from construct trees to other construct trees; all constructs now take dependencies and also implement `IDependable`. The semantics are that any resource in the depending tree will depend on all resources in the depended tree. Dependencies are cross-stack aware If you take a dependency on a construct in another stack, the dependency does not get rendered in the template, but is instead added as a dependency between stacks. Fixes aws#1568, fixes aws#95. BREAKING CHANGE: `resource.addDependency()` has been moved onto `ConstructNode`. You now write `resource.node.addDependency()`. VPC's `internetDependency` has been moved to the subnets as `internetConnectivityEstablished`. Target Group's `loadBalancerAssociationDependencies` has been renamed to `loadBalancerAttached`.

See CHANGELOG

Adds `LambdaApplication` and `LambdaDeploymentGroup` such that an `Alias` to a Lambda `Function` can be deployed via CodeDeploy; supports pre/post hook functions, traffic-shifting and alarm monitoring and rollback.

…#1664) Adds option to enable all block public access settings on the bucket (`blockPublicAccess`). Also throws an error when trying to use grantPublicAccess if blockPublicAccess is set to true. https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html

…ource Type

A generalized aspect framework is added. Aspects can be used to affect the construct tree without modifying the class hierarchy. This framework is designed to be generic for future use cases. Tagging is the first implementation. Tagging has been reimplemented to leverage the aspect framework and simplify the original tag design. Tag Manager still exists, but is no longer intended for use by L2 construct authors. There are two new classes `cdk.Tag` and `cdk.RemoveTag`. As new resources are added tag support will be automatic as long as they implement one of the existing tag specifications. All L2 constructs have removed the TagManager. Code generation has been modified to add tag support to any CloudFormation Resource with a matching specification, by creating a Tag Manager for that resource as a `tags` attribute. The schema code now includes the ability to detect 3 forms of tags which include the current CloudFormation Resources. BREAKING CHANGE: if you are using TagManager the API for this object has completely changed. You should no longer use TagManager directly, but instead replace this with Tag Aspects. `cdk.Tag` has been renamed to `cdk.CfnTag` to enable `cdk.Tag` to be the Tag Aspect. Fixes aws#1136.

packages/@aws-cdk/cdk/test/core/test.tag-manager.ts

moofish32 · 2019-02-06T08:37:49Z

@eladb - I think tests are finally passing. Is there anything else?

moofish32 requested a review from a team as a code owner December 29, 2018 04:46

moofish32 mentioned this pull request Dec 29, 2018

An aspect system for the construct tree #1136

Closed

eladb reviewed Dec 30, 2018

View reviewed changes

rix0rrr reviewed Dec 31, 2018

View reviewed changes

moofish32 commented Jan 2, 2019

View reviewed changes

moofish32 force-pushed the f-tags-aspects branch from b05e492 to 7eda0c2 Compare January 14, 2019 05:12

moofish32 commented Jan 14, 2019

View reviewed changes

rix0rrr mentioned this pull request Jan 15, 2019

feat(cfn2ts) Add support for Tags on L1 constructs #1007

Closed

2 tasks

moofish32 added 4 commits January 15, 2019 21:17

fixing public access to resource properties

419a701

fixing dynamo tag implementation

0a0d010

refactoring to add in a TestStack for easy unit testing when aspects …

0109956

…are involved

moofish32 force-pushed the f-tags-aspects branch from 0478e6b to 0109956 Compare January 16, 2019 05:25

moofish32 requested a review from SoManyHs as a code owner January 16, 2019 05:25

moofish32 added 3 commits January 15, 2019 21:40

adding documentation to classes

2188764

cleaning up some extraneous comments

ce95e64

adding tag-manager unit tests

f3a7e37

moofish32 changed the title ~~Aspects Code Exploration~~ feat(cdk): add aspects for cdk Jan 16, 2019

moofish32 commented Jan 16, 2019

View reviewed changes

moofish32 added 2 commits January 16, 2019 01:04

convert dynamo test to use expect

6cc19db

test stack cleanup for kms

67a738b

eladb reviewed Jan 16, 2019

View reviewed changes

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts Outdated Show resolved Hide resolved

eladb reviewed Jan 16, 2019

View reviewed changes

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts Outdated Show resolved Hide resolved

fulghum assigned eladb Feb 4, 2019

moofish32 changed the title ~~feat(cdk): aspects and tagging~~ feat(cdk): aspect framework and tag implementation Feb 5, 2019

moofish32 and others added 13 commits February 4, 2019 23:20

adding cdk.json for tag example

12cfe19

feat(aws-ecs): add support for Event Targets (aws#1571)

dcc5303

EC2 task definitions can now be used as CloudWatch event targets. ALSO IN THIS COMMIT * Improve hash calculation of Docker images. * Add `grantPassRole()` method to iam.Role Fixes aws#1370.

fix(apig): Move 'selectionPattern to integrationResponses` (aws#1636)

e3f5ad3

The property was modeled in the wrong place, rendering it unusable. Moved it to the correct location instead, and added a validation test. Fixes aws#1608

v0.23.0 (aws#1665)

fec19b6

See CHANGELOG

removing tag aspect props and just using tag props

3bc66f2

feat(codedeploy) lambda application and deployment groups (aws#1628)

aeb24f1

Adds `LambdaApplication` and `LambdaDeploymentGroup` such that an `Alias` to a Lambda `Function` can be deployed via CodeDeploy; supports pre/post hook functions, traffic-shifting and alarm monitoring and rollback.

chore(docs): move developer guide to docs.aws.amazon.com (aws#1470)

db75215

refactor to eliminate TagAspectProps, requires TagManager to know Res…

a91a4f1

…ource Type

Merge branch 'master' into f-tags-aspects

395a83f

moofish32 requested review from RomainMuller and skinny85 as code owners February 5, 2019 07:59

minor fix to the example

bafff0f

eladb approved these changes Feb 5, 2019

View reviewed changes

packages/@aws-cdk/cdk/test/core/test.tag-manager.ts Outdated Show resolved Hide resolved

moofish32 added 3 commits February 5, 2019 08:34

fix test in tag-manager to add resource type string for clarity

fd94030

Merge branch 'master' into f-tags-aspects

3bfa467

updating integ test that now tags the lambda function

d0fd751

eladb merged commit f7c8531 into aws:master Feb 6, 2019

eladb mentioned this pull request Feb 6, 2019

docs: Create a "How-To Tag my Resources" section #360

Closed

fulghum added the effort/medium Medium work item – several days of effort label Feb 11, 2019

moofish32 mentioned this pull request Feb 11, 2019

Unable to set child tags when parent tags are inherited #1725

Closed

This was referenced Mar 3, 2019

Document aspects #1930

Closed

Ability to specify Tags across all resources #1516

Closed


		protected abstract applyTag(resource: Resource): void;

		private hasPrecedence(aspects: IAspect[]): boolean {

		@@ -0,0 +1,7 @@
		import cdk = require('@aws-cdk/cdk');

		export class TestStack extends cdk.Stack {

feat(cdk): aspect framework and tag implementation #1451

feat(cdk): aspect framework and tag implementation #1451

Conversation

moofish32 commented Dec 29, 2018 • edited Loading

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 commented Jan 2, 2019

moofish32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Jan 2, 2019

rix0rrr commented Jan 2, 2019

moofish32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 commented Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Feb 4, 2019

moofish32 commented Feb 6, 2019

moofish32 commented Dec 29, 2018 •

edited

Loading

moofish32 commented Jan 16, 2019 •

edited

Loading