-
Notifications
You must be signed in to change notification settings - Fork 4k
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(codedeploy): CodeDeploy deployment config constructs for Lambda and ECS #22159
Conversation
…codedeploy.LambdaDeploymentConfig
Looks great! I proposed changes to deprecate the |
…onfig chore: deprecate codedeploy.CustomLambdaDeploymentConfig
Thanks @cplee! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think overall I would like to see an example of how this would be used to deploy an ECS service.
Since the lambda & ecs changes are very simliar I only commented on the ecs changes, but the
comments also apply to lambda as well.
Pull request has been modified.
@corymhall I think I addressed all your feedback, expect the pieces that I expect to cover in my next PR (L2 CodeDeploy deployment group constructs for ECS). If you think it would be easier to review deployment config + groups as a single PR, I can merge those changes into this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@corymhall I think I addressed all your feedback, expect the pieces that I expect to cover in my next PR (L2 CodeDeploy deployment group constructs for ECS). If you think it would be easier to review deployment config + groups as a single PR, I can merge those changes into this PR.
ah ok if you have another PR for deployment group that will address that then I am fine with holding
on that stuff.
Just a couple of minor comments and then I think we are good!
Pull request has been modified.
Pull request has been modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience on this! Just two minor things and then I think that is it!
Pull request has been modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last one
Pull request has been modified.
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…and ECS (aws#22159) CloudFormation now supports Lambda and ECS in the `AWS::CodeDeploy::DeploymentConfig` resource type. This PR adds L2 constructs specific to ECS and Lambda for that resource type. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
In #22159, a base class for all deployment configs was added. Classes that extended this base were calling static methods returning `IBaseDeploymentConfig` types where previously they returned more specific types, IE `IServerDeploymentConfig`. [This method](6840d8e#diff-93f73231716deb0056687ee155c13d03ccabe2222fd191a33be9caeb7ad87ebaL126) being previously implemented on `IServerDeploymentConfig` but now inheriting from `IBaseDeploymentConfig` caused the return type to change to `IBaseDeploymentConfig` In Typescript, this is fine as [`IServerDeploymentConfig`](6840d8e#diff-93f73231716deb0056687ee155c13d03ccabe2222fd191a33be9caeb7ad87ebaR12) extends `IBaseDeploymentConfig` with no additional properties. So an instance of `IBaseDeploymentConfig` satisfies the `IServerDeploymentConfig` interface implicitly. However, in Java where interfaces are explicit, this caused the error: ``` incompatible types: IBaseDeploymentConfig cannot be converted to IServerDeploymentConfig ``` Where inputs expect the more specific type.
Change all `deploymentConfig` static method implementations on `EcsDeploymentConfig`, `LambdaDeploymentConfig`, and `ServerDeploymentConfig` to return their corresponding specific interfaces instead of `IBaseDeploymentConfig`. This reverts breaking changes to Java users introduced in #22159
Change all `deploymentConfig` static method implementations on `EcsDeploymentConfig`, `LambdaDeploymentConfig`, and `ServerDeploymentConfig` to return their corresponding specific interfaces instead of `IBaseDeploymentConfig`. This reverts breaking changes to Java users introduced in #22159 Fixes #22566 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Change all `deploymentConfig` static method implementations on `EcsDeploymentConfig`, `LambdaDeploymentConfig`, and `ServerDeploymentConfig` to return their corresponding specific interfaces instead of `IBaseDeploymentConfig`. This reverts breaking changes to Java users introduced in #22159 Fixes #22566 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Change all `deploymentConfig` static method implementations on `EcsDeploymentConfig`, `LambdaDeploymentConfig`, and `ServerDeploymentConfig` to return their corresponding specific interfaces instead of `IBaseDeploymentConfig`. This reverts breaking changes to Java users introduced in aws#22159 Fixes aws#22566 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…and ECS (aws#22159) CloudFormation now supports Lambda and ECS in the `AWS::CodeDeploy::DeploymentConfig` resource type. This PR adds L2 constructs specific to ECS and Lambda for that resource type. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
CloudFormation now supports Lambda and ECS in the
AWS::CodeDeploy::DeploymentConfig
resource type. This PR adds L2 constructs specific to ECS and Lambda for that resource type.All Submissions:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license