feat(codedeploy) - Added ECS CodeDeploy support #22081
Conversation
|
|
cplee
force-pushed
the
add-ecs-deployment-group
branch
from
a9de5db
to
cc42e3f
Compare
…eployment configurations and deployment groups for ECS
cplee
force-pushed
the
add-ecs-deployment-group
branch
from
cc42e3f
to
cdf365e
Compare
There was a problem hiding this comment.
Haha I literally just finished up writing a similar construct! Great minds, etc
main...clareliguori:aws-cdk:in-progress-codedeploy-ecs
I've left a few comments where I saw differences between our implementations
| * | ||
| * @default a name will be auto-generated | ||
| */ | ||
| readonly trafficRoutingConfig?: ITrafficRoutingConfig; |
There was a problem hiding this comment.
I have a related open PR that creates LambdaDeploymentConfig class, and we should make sure the Props structures are consistent. I chose to copy over the structure that LambdaDeploymentConfig has, where percentage, interval, etc are in the root Props structure:
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_codedeploy.CustomLambdaDeploymentConfig.html
I'm not super opinionated either way if you feel like this is a better structure
There was a problem hiding this comment.
I preferred the pattern in the ServerDeploymentConfig. I worry about the pattern in LambdaDeploymentConfig assumes any potential future types would have a similar shape (type + percentage + interval). Seems if a future deployment config type didn't look like that, then it would be hard to extend the current implementation.
I may be overthinking though 🤷♂️
| // Remove revision from ECS Service task definition to allow Blue/Green deploys to work | ||
| cfnService.taskDefinition = service.taskDefinition.family; | ||
| service.node.addDependency(service.taskDefinition); |
There was a problem hiding this comment.
Can you explain more? I'm not following why this is necessary
There was a problem hiding this comment.
This is how I avoid CloudFormation trying to update the ECS service when the TaskDefinition revision changes. By removing revision and only using family, it is only deployed on creation and ignored going forward.
| * | ||
| * @default - blank if empty on test traffic route. Required for prod traffic route. | ||
| */ | ||
| readonly listener: elbv2.IApplicationListener | elbv2.INetworkListener | undefined; |
There was a problem hiding this comment.
I ended up creating a new IListener interface to avoid using a union type here:
clareliguori@c5eb191#diff-1ce9801f6492a5d0a765325bd3737f3622605d4c7a6910547e075ce01ca06fcb
|
Thanks @clareliguori for the feedback. I pushed a bunch of changes to address your comments. When your PR is ready, please drop a link in here and I can close this PR in favor of yours. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Closing this in favor of #22295 |
CloudFormation now supports creating
AWS::CodeDeploy::DeploymentGroupfor ECS services. There is an existing L2 construct that only supported imports. This PR allows the creation ofEcsDeploymentGroupandEcsDeploymentConfigAll Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license