Skip to content
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

Merged
merged 14 commits into from
Sep 29, 2022

Conversation

clareliguori
Copy link
Member

@clareliguori clareliguori commented Sep 21, 2022

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

  • Have you added the new feature to an integration test?
    • 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

@gitpod-io
Copy link

gitpod-io bot commented Sep 21, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team September 21, 2022 02:54
@github-actions github-actions bot added the p2 label Sep 21, 2022
@cplee
Copy link
Contributor

cplee commented Sep 21, 2022

Looks great! I proposed changes to deprecate the CustomLambdaDeploymentConfig and address the tests.

@clareliguori
Copy link
Member Author

Thanks @cplee!

Copy link
Contributor

@corymhall corymhall left a 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.

@mergify mergify bot dismissed corymhall’s stale review September 26, 2022 17:58

Pull request has been modified.

@clareliguori
Copy link
Member Author

@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.

Copy link
Contributor

@corymhall corymhall left a 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!

@mergify mergify bot dismissed corymhall’s stale review September 27, 2022 16:10

Pull request has been modified.

@mergify mergify bot dismissed corymhall’s stale review September 28, 2022 18:03

Pull request has been modified.

Copy link
Contributor

@corymhall corymhall left a 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!

@mergify mergify bot dismissed corymhall’s stale review September 28, 2022 20:01

Pull request has been modified.

Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last one

@mergify mergify bot dismissed corymhall’s stale review September 29, 2022 13:50

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Sep 29, 2022

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).

@mergify mergify bot merged commit 6840d8e into aws:main Sep 29, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 29, 2022

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: a8112fb
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@clareliguori clareliguori deleted the ecs-lambda-deployment-config branch September 29, 2022 15:33
arewa pushed a commit to arewa/aws-cdk that referenced this pull request Oct 8, 2022
…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*
MrArnoldPalmer added a commit that referenced this pull request Oct 19, 2022
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.
MrArnoldPalmer added a commit that referenced this pull request Oct 20, 2022
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
mergify bot pushed a commit that referenced this pull request Oct 20, 2022
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*
madeline-k pushed a commit that referenced this pull request Oct 21, 2022
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*
mrgrain pushed a commit to mrgrain/aws-cdk that referenced this pull request Oct 24, 2022
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*
homakk pushed a commit to homakk/aws-cdk that referenced this pull request Dec 1, 2022
…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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants