feat(aws-codedeploy): add instance tag filter support for server Deployment Groups by skinny85 · Pull Request #824 · aws/aws-cdk

skinny85 · 2018-10-02T01:31:30Z

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

rix0rrr · 2018-10-02T06:18:33Z

Did you have a look at other tagging PRs? Was the pattern there not sufficient??

yubangxi · 2018-10-02T16:23:09Z

packages/@aws-cdk/aws-codedeploy/lib/deployment-group.ts

  installAgent?: boolean;
+
+  /**
+   * All EC2 instances matching the given set of tags will be added to this Deployment Group.


Can we make the comments more clear - instances are expanded by tag during deployment only.

Good call. Will change.

yubangxi · 2018-10-02T16:24:25Z

packages/@aws-cdk/aws-codedeploy/lib/deployment-group.ts

+        } else {
+          tagsInGroup.push({
+            key: tagKey,
+            type: 'KEY_AND_VALUE',


should this be KEY_ONLY?

And also, are we going to support VALUE_ONLY type tag?

We absolutely should! Thanks for catching this.

skinny85 · 2018-10-02T17:53:04Z

Did you have a look at other tagging PRs? Was the pattern there not sufficient??

I did. The problem is that that API is for assigning tags, while here what we actually need are not tags per se, but tag filters - perhaps I should have made it clear in the naming, but I didn't want to make the attribute and class names longer than they already are.

For example, there isn't a (concise) way to represent with tags the 'or' semantics that a tag group filter has, nor the 'and' semantics of what I call a tag set filter here.

Does this explanation make sense?

skinny85 · 2018-10-02T18:45:24Z

Updated with Bangxi's feedback.

skinny85 · 2018-10-03T16:56:34Z

@rix0rrr ping

rix0rrr · 2018-10-03T21:04:02Z

packages/@aws-cdk/aws-codedeploy/lib/deployment-group.ts

+ * If the key is an empty string, any tag,
+ * regardless of its key, with any of the given values, will match.
+ */
+export interface InstanceTagGroup {


Don't know if this is going to translate well over jsii. I think it won't. Better definition would be:

export type InstanceTagGroup = {[key: string]: string[]};

Sure. Will change.

…oyment Groups.

skinny85 · 2018-10-04T18:24:54Z

Changed InstanceTagGroup from an interface to a type alias like @rix0rrr suggested.

skinny85 · 2018-10-05T17:36:01Z

@rix0rrr ping again :)

skinny85 · 2018-10-08T18:08:36Z

@rix0rrr ping ping

skinny85 · 2018-10-09T16:04:00Z

@rix0rrr ping ping ping

@eladb feel free to review as well (you were the original reviewer)

eladb · 2018-10-09T16:31:54Z

packages/@aws-cdk/aws-codedeploy/lib/deployment-group.ts

+ * in other words, sets follow 'and' semantics.
+ * You can have a maximum of 3 tag groups inside a set.
+ */
+export class InstanceTagSet {


Why is this type at all needed? Makes usage more convoluted without adding much value. I would expect:

new DeploymentGroup(this, 'gd', { ec2InstanceTags: { 'key1': [ 'tag1', 'tag2' ] } });

Also, since this is an array, we have seen it's useful to have a mutator, so add a method addEc2InstanceTag(tag, ...groups) and addOnPermiseInstanceTag(tag, ...group).

Because you need to have 'and' and 'or' semantics represented. Have you seen the example from the ReadMe I gave?

onPremiseInstanceTags: new codedeploy.InstanceTagSet( // only instances with tags (key1=v1 or key1=v2) AND key2=v3 will match this set { 'key1': ['v1', 'v2'], }, { 'key2': ['v3'], }, ),

eladb · 2018-10-09T16:32:39Z

packages/@aws-cdk/aws-codedeploy/lib/deployment-group.ts

+      ec2TagSetList: tagSet.instanceTagGroups.map(tagGroup => {
+        return {
+          ec2TagGroup: this.tagGroup2TagsArray(tagGroup) as
+            cloudformation.DeploymentGroupResource.EC2TagFilterProperty[],


Why do you need the as here? Just make sure tagGroup2TagsArray indicates the return type

Because you need two distinct types here (EC2TagFilterProperty for EC2 instance tags, TagFilterProperty for on-premise instance tags), while the structure of them is the same - so, I'm taking advantage of TypeScript's structural typing to reduce code duplication here.

eladb · 2018-10-09T16:33:34Z

packages/@aws-cdk/aws-codedeploy/lib/deployment-group.ts

+ * If the key is an empty string, any tag,
+ * regardless of its key, with any of the given values, will match.
+ */
+export type InstanceTagGroup = {[key: string]: string[]};


I don't know how this type will be exported through jsii. I don't think it's really needed.

@rix0rrr claims it should be fine. We have types like these already in the CDK.

Yes this works. Don't know the exact output, but the type alias is transparent to jsii users

skinny85 · 2018-10-10T16:58:48Z

@eladb ping?

### Highlights - __A new construct library for AWS Step Functions__ ([docs](https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-stepfunctions/README.md)). The library provides rich APIs for modeling state machines by exposing a programmatic interface for [Amazon State Language](https://docs.aws.amazon.com/step-functions/latest/dg/concepts-amazon-states-language.html). - __A new construct library for Amazon S3 bucket deployments__ ([docs](https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-s3-deployment/README.md)). You can use now automatically populate an S3 Bucket from a .zip file or a local directory. This is a building block for end-to-end support for static websites in the AWS CDK. ### Bug Fixes * **aws-apigateway:** make LambdaRestApi proxy by default ([#963](#963)) ([a5f5e2c](a5f5e2c)), closes [#959](#959) * **aws-cdk:** Allow use of assumed roles behind a proxy ([#898](#898)) ([f2b1048](f2b1048)) * **aws-cdk:** Auto-delete stacks that failed creating before new attempt ([#917](#917)) ([2af8309](2af8309)) * **aws-cloudfront:** expose distributionId ([#938](#938)) ([f58d98c](f58d98c)) * **aws-dynamodb:** don't emit empty array properties ([#909](#909)) ([841975a](841975a)) * **docs:** use ..code to display file structure in "writing constructs" ([#935](#935)) ([b743362](b743362)) ### Features * **assets:** isZipArchive indicates if this is a zip asset ([#944](#944)) ([65190f9](65190f9)) * **aws-cdk:** deploy supports CloudFormation Role ([#940](#940)) ([393be6f](393be6f)), closes [#735](#735) * **aws-cloudformation:** allow specifying custom resource type ([#943](#943)) ([9de3a84](9de3a84)) * **aws-cloudformation:** correctly handle the templateConfiguration property in the CreateUpdateStack Pipeline Action. ([#923](#923)) ([d251a46](d251a46)) * **aws-cloudfront:** add support for "webAclId" ([#969](#969)) ([3ec9d76](3ec9d76)) * **aws-codedeploy:** add auto rollback configuration to server Deployment Group. ([#925](#925)) ([7ee91cf](7ee91cf)) * **aws-codedeploy:** add instance tag filter support for server Deployment Groups. ([#824](#824)) ([e6e8c51](e6e8c51)) * **aws-codedeploy:** add support for setting CloudWatch alarms on a server Deployment Group. ([#926](#926)) ([27b26b1](27b26b1)) * add support for Step Functions ([#827](#827)) ([81b533c](81b533c)) * **aws-lambda:** add grantInvoke() method ([#962](#962)) ([1ee8135](1ee8135)), closes [#961](#961) * **aws-lambda:** improvements to the code and runtime APIs ([#945](#945)) ([36f29b6](36f29b6)), closes [#902](#902) [#188](#188) [#947](#947) [#947](#947) [#664](#664) * **aws-logs:** extractMetric() returns Metric object ([#939](#939)) ([5558fff](5558fff)), closes [#850](#850) * **aws-s3:** initial support for website hosting ([#946](#946)) ([2d3661c](2d3661c)) * **aws-s3-deployment:** bucket deployments ([#971](#971)) ([84d6876](84d6876)), closes [#952](#952) [#953](#953) [#954](#954) * **docs:** added link to CloudFormation concepts ([#934](#934)) ([666bbba](666bbba)) ### BREAKING CHANGES * **aws-apigateway:** specifying a path no longer works. If you used to provide a '/', remove it. Otherwise, you will have to supply `proxy: false` and construct more complex resource paths yourself. * **aws-lambda:** The construct `lambda.InlineJavaScriptLambda` is no longer supported. Use `lambda.Code.inline` instead; `lambda.Runtime.NodeJS43Edge` runtime is removed. CloudFront docs [stipulate](https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/lambda-requirements-limits.html#lambda-requirements-lambda-function-configuration) that you should use node6.10 or node8.10. It is always possible to use any value by instantiating a `lambda.Runtime` object.

skinny85 requested a review from eladb October 2, 2018 01:31

yubangxi reviewed Oct 2, 2018

View reviewed changes

skinny85 force-pushed the feature/codedeploy-tags branch from 859a86d to 14573f9 Compare October 2, 2018 18:44

rix0rrr reviewed Oct 3, 2018

View reviewed changes

feat(aws-codedeploy): add instance tag filter support for server Depl…

5333d31

…oyment Groups.

skinny85 force-pushed the feature/codedeploy-tags branch from 14573f9 to 5333d31 Compare October 4, 2018 18:24

skinny85 changed the title ~~feat(aws-codedeploy): tag support for Deployment Groups~~ feat(aws-codedeploy): add instance tag filter support for server Deployment Groups Oct 4, 2018

eladb reviewed Oct 9, 2018

View reviewed changes

rix0rrr approved these changes Oct 10, 2018

View reviewed changes

skinny85 merged commit e6e8c51 into aws:master Oct 12, 2018

skinny85 deleted the feature/codedeploy-tags branch October 12, 2018 22:57

eladb mentioned this pull request Oct 19, 2018

v0.13.0 #975

Merged

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Conversation

skinny85 commented Oct 2, 2018

Uh oh!

rix0rrr commented Oct 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 commented Oct 2, 2018

Uh oh!

skinny85 commented Oct 2, 2018

Uh oh!

skinny85 commented Oct 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 commented Oct 4, 2018

Uh oh!

skinny85 commented Oct 5, 2018

Uh oh!

skinny85 commented Oct 8, 2018

Uh oh!

skinny85 commented Oct 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 commented Oct 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

skinny85 Oct 9, 2018 •

edited

Loading