Deployment Configuration Construct for CodeDeploy L2 #653

skinny85 · 2018-08-31T22:57:53Z

Part of the work on a AWS Construct Library for CodeDeploy.

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

eladb · 2018-09-04T06:11:54Z

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

+/**
+ * Represents a Deployment Configuration for an EC2/on-premise Deployment Group.
+ */
+export interface ServerDeploymentConfiguration {


This should be IXxx because it's not a data type.

Also, why do you need both the name and the ARN here? Are both needed downstream?

Also, why do you need both the name and the ARN here? Are both needed downstream?

Yep, both are needed at different spots.

This should be IXxx because it's not a data type.

It's an interface because there are 2 kinds of Configs - the predefined ones (OneAtATime, AllAtOnce, etc.) and the custom ones (the ones you define with minimumHealthyHosts). Both implementations are data types. Naming this IServerDeploymentConfiguration seems incredibly ugly to me :-(. But if you think that's better, I can change it.

As mentioned, our convention is that only data types (like "props" bags) can omit the I prefix. All other interfaces should be prefixed with I.

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

eladb · 2018-09-04T06:14:04Z

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

+    }
+}
+
+function deploymentConfigurationName2Arn(deploymentConfigurationName: DeploymentConfigurationName):


Nit: Keep concise naming if possible - the type is part of your contract, so no need to repeat youreslf, rename deploymentConfigurationName to name.

eladb · 2018-09-04T06:15:49Z

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

+/**
+ * A custom Deployment Configuration for an EC2/on-premise Deployment Group.
+ */
+export class CustomServerDeploymentConfiguration extends CustomServerDeploymentConfigurationRef {


Should should be called DeploymentConfig and DeploymentConfigRef (to align with the CFN resource name).

Fair enough. I wanted to make it more consistent than CFN, because, for instance, the auto-rollback property and type is called AutoRollbackConfiguration, and it seems weird to me to have Configuration and Config both used in the same class. But I can rename it if you like Config here better.

Oh, also, the Server prefix is required (other compute platform like Lambda have completely different Deployment Configurations - it should not be possible to mix them).

eladb · 2018-09-04T06:18:45Z

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

+    /**
+     * The minimum healhty hosts setting for this EC2/on-premise Deployment Configuration.
+     */
+    minimumHealthyHosts: MinimumHealthyHosts;


I think flatting this into two mutually exclusive properties would make the API more concise and less convoluted. I understand this means it's a "synth-time" error as oppose to compile-time, but it's still cheap.

new DeploymentConfig(this, 'MyConfig', { minHealthyCount: 100 });

or:

new DeploymentConfig(this, 'MyConfig', { minHealthyPercent: 50 });

I did it this way because it's more aligned with the CFN way of doing it, which seems important (see above ^^). I don't feel super strongly about it though - if you like the 2 mutually exclusive properties better, I have no problem changing it.

We have no goal that our APIs will have the same structure as CFN. In fact, in most cases, the CDK offers an opportunity to simplify, improve discoverability and ergonomics. It's very common when we design L2s to flatten the declarative API in CFN.

eladb · 2018-09-04T06:21:03Z

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

+ * @see #AllAtOnce
+ */
+export class PredefinedServerDeploymentConfiguration implements ServerDeploymentConfiguration {
+    public static readonly OneAtATime = new PredefinedServerDeploymentConfiguration('CodeDeployDefault.OneAtATime');


Intuitively, I'd expect these to be available on DeploymentConfig:

DeploymentConfig.OneAtAtTime DeploymentConfig.AllAtOnes DeploymentConfig.AllAtOnces

Well. They can't be on DeploymentConfig for sure, because they make no sense for, for example, Lambda deployments.

I can move them to ServerDeploymentConfig though (or IServerDeploymentConfig, if we decide to rename ServerDeploymentConfig, but customers having to write IServerDeploymentConfig.OneAtATime seems quite ugly...).

I agree that IServerDeploymentConfig is ugly. Why isn't ServerDeploymentConfig an abstract base class?

eladb · 2018-09-04T06:24:56Z

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

+/**
+ * Reference to a custom Deployment Configuration for an EC2/on-premise Deployment Group.
+ */
+export abstract class CustomServerDeploymentConfigurationRef extends cdk.Construct


Rename to DeploymentConfigRef

Like I said above, the Server prefix is needed here.

ServerDeploymentConfigRef?

Yes, I guess

eladb · 2018-09-04T06:26:17Z

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

+        });
+
+        this.deploymentConfigurationName = resource.ref;
+        this.deploymentConfigurationArn = deploymentConfigurationName2Arn(this.deploymentConfigurationName);


Why not just implement this as a calculated property in the DeploymentConfigRef? (seems like this is repeating everywhere)

Because you need to call super in the constructor as the first statement, and the name is a property of the constructed DeploymentConfigResource, so there is no way to pass it in the super call (well, technically we could do it, but it would be ugly).

eladb · 2018-09-16T13:08:58Z

Awaiting changes

skinny85 · 2018-09-18T01:44:52Z

Awaiting changes

Updated.

skinny85 · 2018-09-18T18:11:30Z

The fact that 0 is falsy in JavaScript bit me again ;p

eladb · 2018-09-19T18:02:29Z

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

+import cdk = require('@aws-cdk/cdk');
+import { cloudformation } from './codedeploy.generated';
+
+export class DeploymentConfigName extends cdk.CloudFormationToken {}


Those are deprecated. Just use strings!
See #712

#653 < #712 😢

Will rebase.

eladb · 2018-09-19T18:04:58Z

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

+
+function arnForDeploymentConfigName(name: DeploymentConfigName):
+        DeploymentConfigArn {
+    return cdk.Arn.fromComponents({


Arn renames to ArnUtils

eladb · 2018-09-19T18:06:07Z

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

+
+    /**
+     * The EC2/on-premise Deployment Configuration to use for this Deployment Group.
+     *


Since this is an interface. Provide some pointers/example on how to use a predefined one and how to define a custom one

Good point. Will fix.

eladb · 2018-09-19T18:07:34Z

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

@@ -112,7 +133,7 @@ export class ServerDeploymentGroup extends ServerDeploymentGroupRef {
    public readonly deploymentGroupName: DeploymentGroupName;

    constructor(parent: cdk.Construct, id: string, props?: ServerDeploymentGroupProps) {


Instead of having to null-check props everywhere, you can assign a default value of {}

eladb · 2018-09-19T18:08:49Z

packages/@aws-cdk/aws-codedeploy/package.json

@@ -39,6 +39,10 @@
  "cdk-build": {
    "cloudformation": "AWS::CodeDeploy"
  },
+  "nyc": {


Why can’t we use the default?

eladb · 2018-09-19T18:13:26Z

packages/@aws-cdk/aws-codedeploy/README.md

+
+The default Deployment Configuration is `ServerDeploymentConfig.OneAtATime`.
+
+You can also create a custom Deployment Configuration:


Maybe in a subsequent commit, but can we provide a sugar that allows me to do this:

deploymentConfig: ServerDeploymentConfig.custom({ minHealthyCount: 5 })

It will be the natural thing for people to look for when they are “browsing” the options.

The challenge would be how to propagate the parent construct. I think that the construct id can be just DeploymentConfig if you attach this to the DeploymentGroup because a deployment group can only have one deployment config.

Yeah, I don't see an easy way of allowing both that form, and deploymentConfig: new ServerDeploymentConfig(this, 'id', { minHealthyHostCount: 5 })...

skinny85 · 2018-09-19T22:05:34Z

Updated with the feedback from Elad.

skinny85 · 2018-09-20T19:43:25Z

Rebased.

eladb

Please make sure to use conventional commits when squash/merging

eladb · 2018-09-20T20:05:36Z

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

@@ -108,22 +131,26 @@ export class ServerDeploymentGroup extends ServerDeploymentGroupRef {
    public readonly deploymentGroupName: string;

    constructor(parent: cdk.Construct, id: string, props?: ServerDeploymentGroupProps) {


Tip, use { } as the default value for props:

constructor(parent: cdk.Construct, id: string, props: ServerDeploymentGroupProps = { })

Then, you can eliminate line 136

Good call, changed!

eladb · 2018-09-20T20:06:32Z

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

        });

-        this.deploymentGroupName = resource.ref;
+        this.deploymentGroupName = resource.ref.toString();


Don't you ref, there should be a strong-name such as resource.deploymentGroupName.
Also, FYI, ref is already a string.

Lol @ "Don't you ref" 🙃

Changed to use deploymentGroupName :)

Part of the work on a AWS Construct Library for CodeDeploy.

* **aws-codecommit:** typo in README ([#780](#780)) ([0e79c2d](0e79c2d)) * **aws-ec2:** fix capitalization of "VPCEndpointType" to "VpcEndpointType" ([#789](#789)) ([7a8ee2c](7a8ee2c)), closes [#765](#765) * **docs:** fix issue [#718](#718) (Aurora DB example) ([#783](#783)) ([016f3a8](016f3a8)) * **util:** remove [@aws-cdk](https://github.com/aws-cdk)/util ([#745](#745)) ([10015cb](10015cb)), closes [#709](#709) * **aws-cloudformation:** rename the CodePipeline actions ([#771](#771)) ([007e7b4](007e7b4)) * **aws-cloudformation:** update the README of the module to reflect the new action names ([#775](#775)) ([6c0e75b](6c0e75b)), closes [#771](#771) * **aws-codedeploy:** add auto-scaling groups property to ServerDeploymentGroup ([#739](#739)) ([0b28886](0b28886)) * **aws-codedeploy:** add deployment configuration construct ([#653](#653)) ([e6b67ad](e6b67ad)) * **aws-codepipeline, aws-codecommit, aws-s3:** change the convention for naming the source Actions to XxxSourceAction ([#753](#753)) ([9c3ce7f](9c3ce7f)) * **aws-elasticloadbalancingv2:** support for ALB/NLB ([#750](#750)) ([bd9ee01](bd9ee01)) * tagging support for AutoScaling/SecurityGroup ([#766](#766)) ([3d48eb2](3d48eb2)) * **core:** resource overrides (escape hatch) ([#784](#784)) ([5054eef](5054eef)), closes [#606](#606) * **toolkit:** stop creating 'empty' stacks ([#779](#779)) ([1dddd8a](1dddd8a)) * **cdk**: the constructor signature of `TagManager` has changed. `initialTags` is now passed inside a props object. * **util:** `@aws-cdk/util` is no longer available * **aws-elasticloadbalancingv2:** adds classes for modeling Application and Network Load Balancers. AutoScalingGroups now implement the interface that makes constructs a load balancing target. The breaking change is that Security Group rule identifiers have been changed in order to make adding rules more reliable. No code changes are necessary but existing deployments may experience unexpected changes. * **aws-cloudformation:** this renames all CloudFormation Actions for CodePipeline to bring them in line with Actions defined in other service packages. * **aws-codepipeline, aws-codecommit, aws-s3:** change the names of the source Actions from XxxSource to XxxSourceAction. This is to align them with the other Actions, like Build. Also, CodeBuild has the concept of Sources, so it makes sense to strongly differentiate between the two.

Bug Fixes --------- * **aws-apigateway:** allow + in path parts ([#769](#769)) ([0c50d27](0c50d27)), closes [#768](#768) * **aws-cdk:** continue after exceptions in stack monitor ([#791](#791)) ([b0f3298](b0f3298)), closes [#787](#787) * **aws-cloudfront:** check for undefined and determining of the defaultRootObject prop is set or not ([#801](#801)) ([32a74c6](32a74c6)) * **aws-cloudfront:** properly support loggingConfig ([#809](#809)) ([5512f70](5512f70)), closes [#721](#721) * **aws-codecommit:** typo in README ([#780](#780)) ([0e79c2d](0e79c2d)) * **aws-ec2:** Add Burstable Generation 3 Instances ([#812](#812)) ([d36ee6d](d36ee6d)) * **aws-ec2:** fix capitalization of "VPCEndpointType" to "VpcEndpointType" ([#789](#789)) ([7a8ee2c](7a8ee2c)), closes [#765](#765) * **aws-ec2:** fix typo in resource identifier ([#818](#818)) ([f529c80](f529c80)) * **aws-elbv2:** fix load balancer registration ([#890](#890)) ([8cc9abe](8cc9abe)) * **aws-s3:** properly export bucketDomainName ([#844](#844)) ([a65060d](a65060d)) * **aws-sqs:** Queue.import() doesn't return a value ([#885](#885)) ([c592b7f](c592b7f)), closes [#879](#879) * **cdk:** fix TagManager to evaluate to undefined if no tags are included ([#882](#882)) ([477c827](477c827)) * **cdk:** init templates were not upgraded to typescript ^3.0.0 ([#904](#904)) ([2cc7475](2cc7475)) * **cdk:** jsx support conflicts with React usage ([#884](#884)) ([76d8031](76d8031)), closes [#830](#830) * **cfn2ts:** expect Token instead of CloudFormationToken ([#896](#896)) ([6eee1d2](6eee1d2)) * **docs:** fix issue [#718](#718) (Aurora DB example) ([#783](#783)) ([016f3a8](016f3a8)) * **docs:** update supported languages in README ([#819](#819), [#450](#450)) ([#820](#820)) ([ffac98c](ffac98c)) * Correct heading level of CHANGELOG.md 0.10.0 ([40d9ef0](40d9ef0)) * Emit valid YAML-1.1 ([#876](#876)) ([ff857ea](ff857ea)), closes [#875](#875) * **toolkit:** improve error message for large templates ([#900](#900)) ([a41f48f](a41f48f)), closes [#34](#34) Code Refactoring ---------------- * **aws-iam:** move IAM classes cdk to aws-iam ([#866](#866)) ([d46a95b](d46a95b)), closes [#196](#196) * **util:** remove [@aws-cdk](https://github.com/aws-cdk)/util ([#745](#745)) ([10015cb](10015cb)), closes [#709](#709) * **framework:** remove app boilerplate and improvements to cx protocol ([#868](#868)) ([005beec](005beec)), closes [#216](#216) Features -------- * **aws-apigateway:** "LambdaRestApi" and "addProxy" routes ([#867](#867)) ([905a95d](905a95d)) * **aws-cdk:** add maven wrapper to java template ([#811](#811)) ([72aa872](72aa872)) * **aws-cloudformation:** rename the CFN CodePipeline Actions. ([#771](#771)) ([007e7b4](007e7b4)) * **aws-cloudformation:** update the ReadMe of the module to reflect the new Action names. ([#775](#775)) ([6c0e75b](6c0e75b)), closes [#771](#771) * **aws-cloudfront:** Support Security Policy ([#804](#804)) ([b39bf11](b39bf11)), closes [#795](#795) * **aws-codedeploy:** Add the auto-scaling groups property to ServerDeploymentGroup. ([#739](#739)) ([0b28886](0b28886)) * **aws-codedeploy:** Deployment Configuration Construct. ([#653](#653)) ([e6b67ad](e6b67ad)) * **aws-codedeploy:** support setting a load balancer on a Deployment Group. ([#786](#786)) ([e7af9f5](e7af9f5)) * **aws-codepipeline:** allow specifying the runOrder property when creating Actions. ([#776](#776)) ([d146c8d](d146c8d)) * **aws-codepipeline, aws-codecommit, aws-s3:** change the convention for naming the source Actions to XxxSourceAction. ([#753](#753)) ([9c3ce7f](9c3ce7f)) * **aws-dynamodb:** IAM grants support ([#870](#870)) ([c5a4200](c5a4200)) * **aws-dynamodb:** support Global Secondary Indexes ([#760](#760)) ([3601440](3601440)) * **aws-dynamodb:** tags support ([#814](#814)) ([924c84e](924c84e)) * **aws-dynamodB:** support Local Secondary Indexes ([#825](#825)) ([3175af3](3175af3)) * **aws-ec2:** add support for ICMP protocol's classification Types & Codes to SecurityGroupRule ([#893](#893)) ([85bd3c0](85bd3c0)) * **aws-ec2:** allow configuring subnets for NAT gateway ([#874](#874)) ([8ec761c](8ec761c)) * **aws-ec2:** support UDP port ranges in SecurityGroups ([#835](#835)) ([b42ef90](b42ef90)) * **aws-elasticloadbalancingv2:** support for ALB/NLB ([#750](#750)) ([bd9ee01](bd9ee01)) * **aws-s3:** support granting public access to objects ([#886](#886)) ([bdee191](bdee191)), closes [#877](#877) * **cdk:** Add support for UseOnlineResharding with UpdatePolicies ([#881](#881)) ([1f717e1](1f717e1)) * **cdk:** configurable default SSM context provider ([#889](#889)) ([353412b](353412b)) * **core:** resource overrides (escape hatch) ([#784](#784)) ([5054eef](5054eef)), closes [#606](#606) * **aws-codepipeline**: Manage IAM permissions for (some) CFN CodePipeline actions ([#843](#843)) ([4c69118](4c69118)) * **toolkit:** Stop creating 'empty' stacks ([#779](#779)) ([1dddd8a](1dddd8a)) * **aws-autoscaling, aws-ec2:** Tagging support for AutoScaling/SecurityGroup ([#766](#766)) ([3d48eb2](3d48eb2)) ### BREAKING CHANGES * **framework:** The `cdk.App` constructor doesn't accept any arguments, and `app.run()` does not return a `string` anymore. All AWS CDK apps in all languages would need to be modified to adhere to the new API of the `cdk.App` construct. Instead of: const app = new App(process.argv); // ERROR // add stacks process.stdout.write(app.run()); // ERROR The new usage is: const app = new App(); // add stacks app.run(); * **framework:** The CDK is no longer shipped with built-in support for JSX. You can still use JSX but you will have to manually configure it. * **aws-iam:** `PolicyDocument`, `PolicyStatement` and all `PolicyPrincipal` classes moved from the @aws-cdk/cdk module and into the @aws-cdk/aws-iam module. * **aws-codepipeline-api**: `Artifact.subartifact` method of the CodePipeline API was renamed to `Artifact.atPath`. * constructor signature of `TagManager` has changed. `initialTags` is now passed inside a props object. * **util:** @aws-cdk/util is no longer available * **aws-elasticloadbalancingv2:** Adds classes for modeling Application and Network Load Balancers. AutoScalingGroups now implement the interface that makes constructs a load balancing target. The breaking change is that Security Group rule identifiers have been changed in order to make adding rules more reliable. No code changes are necessary but existing deployments may experience unexpected changes. * **aws-cloudformation:** this renames all CloudFormation Actions for CodePipeline to bring them in line with Actions defined in other service packages. * **aws-codepipeline, aws-codecommit, aws-s3:** change the names of the source Actions from XxxSource to XxxSourceAction. This is to align them with the other Actions, like Build. Also, CodeBuild has the concept of Sources, so it makes sense to strongly differentiate between the two.

skinny85 requested review from RomainMuller, rix0rrr and eladb August 31, 2018 22:57

eladb suggested changes Sep 4, 2018

View reviewed changes

skinny85 force-pushed the feature/l2-for-codedeploy branch from 21deb67 to 9c3ba93 Compare September 18, 2018 01:44

skinny85 force-pushed the feature/l2-for-codedeploy branch from 9c3ba93 to 30a40da Compare September 18, 2018 18:05

eladb reviewed Sep 19, 2018

View reviewed changes

skinny85 force-pushed the feature/l2-for-codedeploy branch from 30a40da to 6a24ce3 Compare September 19, 2018 22:05

skinny85 force-pushed the feature/l2-for-codedeploy branch from 6a24ce3 to 16a9b5a Compare September 20, 2018 19:43

eladb approved these changes Sep 20, 2018

View reviewed changes

feat(aws-codedeploy): Deployment Configuration Construct.

c3c9045

Part of the work on a AWS Construct Library for CodeDeploy.

skinny85 force-pushed the feature/l2-for-codedeploy branch from 16a9b5a to c3c9045 Compare September 20, 2018 22:31

skinny85 merged commit e6b67ad into aws:master Sep 20, 2018

skinny85 deleted the feature/l2-for-codedeploy branch September 20, 2018 23:19

eladb mentioned this pull request Sep 27, 2018

v0.10.0 #800

Merged

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

		@@ -112,7 +133,7 @@ export class ServerDeploymentGroup extends ServerDeploymentGroupRef {
		public readonly deploymentGroupName: DeploymentGroupName;

		constructor(parent: cdk.Construct, id: string, props?: ServerDeploymentGroupProps) {


		The default Deployment Configuration is `ServerDeploymentConfig.OneAtATime`.

		You can also create a custom Deployment Configuration:

		@@ -108,22 +131,26 @@ export class ServerDeploymentGroup extends ServerDeploymentGroupRef {
		public readonly deploymentGroupName: string;

		constructor(parent: cdk.Construct, id: string, props?: ServerDeploymentGroupProps) {

Deployment Configuration Construct for CodeDeploy L2 #653

Deployment Configuration Construct for CodeDeploy L2 #653

Conversation

skinny85 commented Aug 31, 2018

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

skinny85 Sep 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 Sep 10, 2018 • edited

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 Sep 16, 2018

skinny85 commented Sep 18, 2018

skinny85 commented Sep 18, 2018

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

skinny85 commented Sep 19, 2018

skinny85 commented Sep 20, 2018

eladb left a 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

skinny85 Sep 10, 2018 •

edited

skinny85 Sep 10, 2018 •

edited