feat(codedeploy): CodeDeploy deployment construct for ECS #22455

cplee · 2022-10-11T07:26:40Z

Extends #22295 by adding support for creating Deployments for ECS Deployment Groups.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

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 · 2022-10-11T07:26:44Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

cplee · 2022-10-19T06:38:10Z

This PR is showing a failure in PR Linter due to the fact that the prlint tool is not paginating files in the PR. This is fixed in #22555

Currently, the `prlint` tool lists the files on a PR to enforce new integration tests are created for `feat` changes. The tool is not using pagination for list files and is using the default of `30` files per page. This means a PR could be incorrectly flagged for missing integration tests if the test files occur after the first 30 files in the PR. Example of this can be seen in #22455 This PR enables pagination on the `listFiles` call to ensure the validation rules are looking at all files in the PR, not just the first 30. ---- ### 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*

Naumel · 2022-10-19T13:15:56Z

Hi, the referenced PR has been merged, I'd say fix the conflicts, try again!
We'll start a review as soon as the work is no longer a draft.

And, as always, thank you for contributing!

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

cplee · 2022-10-19T18:02:04Z

@Naumel - thanks, ready for review!

corymhall

@cplee thanks for the PR! This round I've only focused on the deployment provider.

corymhall · 2022-10-21T12:57:26Z

packages/@aws-cdk/aws-codedeploy/lambda-packages/ecs-deployment-provider/lib/on-event.ts

@@ -0,0 +1,116 @@
+import { Logger } from '@aws-lambda-powertools/logger';
+import { CodeDeploy } from '@aws-sdk/client-codedeploy';


For now lets not introduce any new dependencies. I think we should be able to just use the libraries that come built in with the lambda runtime.

Sure. It'll take a bit of time to rewrite all the tests to use the older style of mocks though.

These changes have been completed and are ready to review.

corymhall · 2022-10-21T13:01:46Z

packages/@aws-cdk/aws-codedeploy/lambda-packages/ecs-deployment-provider/lib/on-event.ts

+      });
+      // cancel deployment and rollback
+      try {
+        const resp = await codedeployClient.stopDeployment({


Should we first look to see if the deployment with the given deploymentId is in process? If it isn't then there is nothing to do.

If there is an in progress deployment that we stop, I think we should then track the progress of that.

Should we first look to see if the deployment with the given deploymentId is in process? If it isn't then there is nothing to do.

This can be unreliable in the event of race conditions. Better to try to stop and handle situations where there isn't a deployment in progress as a success and final event.

If there is an in progress deployment that we stop, I think we should then track the progress of that.

This is already properly handled in the is-complete.ts handler.

corymhall · 2022-10-21T13:05:15Z

packages/@aws-cdk/aws-codedeploy/lambda-packages/ecs-deployment-provider/lib/on-event.ts

+  });
+  switch (event.RequestType) {
+    case 'Create':
+    case 'Update': {


What happens if we create a deployment while there is one in progress? I don't think we should just create a new one on top.

Should we look for an existing deployment and if there is one:

throw an error to fail the stack operation?

start tracking that one instead of creating a new one?

something else?

The CodeDeploy service will throw an error if a deployment is already in progress. I'd rather rely on the service to own enforcing this.

corymhall · 2022-10-21T13:18:19Z

packages/@aws-cdk/aws-codedeploy/lambda-packages/ecs-deployment-provider/lib/is-complete.ts

+  RequestType: string;
+
+  /**
+   * The physical resource id. Could be undefined for 'Create' events.


IsComplete should be called with the results of the on-event right? So the physicalResourceId should never be undefined.

Good call. I will update the comments to clarify.

corymhall · 2022-10-21T13:18:35Z

packages/@aws-cdk/aws-codedeploy/lambda-packages/ecs-deployment-provider/lib/is-complete.ts

+ */
+export interface IsCompleteResponse {
+  /**
+   * True iff the deployment is in a final state.


Suggested change

* True iff the deployment is in a final state.

* True if the deployment is in a final state.

This was intended to say iff...will replace with longer version of if and only if

corymhall · 2022-10-21T13:24:02Z

packages/@aws-cdk/aws-codedeploy/lambda-packages/ecs-deployment-provider/lib/is-complete.ts

+  } catch (e) {
+    logger.error('Unable to determine deployment status', e as Error);
+    if (event.RequestType === 'Delete') {
+      logger.warn('Ignoring error - nothing to do', { complete: true });


I think we do want to fail under certain scenarios. If there is still a deployment in process there is some error deleting it, but we make this resource as successfully deleted we might run into errors trying to delete downstream resources (can you delete a deployment group while a deployment is in process?)

I think this is asking for trouble. Raising errors from custom resources during delete events is the quickest way to get a stack into a state in which it can't be updated or rolled back cleanly and the only resort is to terminate and start over.

corymhall · 2022-10-21T13:24:57Z

packages/@aws-cdk/aws-codedeploy/lambda-packages/ecs-deployment-provider/lib/on-event.ts

+        logger.warn('Ignoring error', e as Error);
+      }
+
+      return {};


I think we need to return some data here in order for it to be processed by the IsComplete handler.

Works fine without it, but I'll add it anyway.

corymhall · 2022-10-21T13:32:08Z

packages/@aws-cdk/aws-codedeploy/lambda-packages/ecs-deployment-provider/lib/on-event.ts

+ * The properties of the CodeDeploy Deployment to create
+ */
+interface DeploymentProperties {
+  description: string;


Can you add docstrings for all the properties in this file.

Pull request has been modified.

cplee · 2022-10-24T04:57:45Z

@corymhall - I've addressed a few of the comments and also replied to a couple that could use more discussion. I've completed the redo of the unit tests. I'd be interested in hearing your feedback on some of the comments I made.

Thanks!

Currently, the `prlint` tool lists the files on a PR to enforce new integration tests are created for `feat` changes. The tool is not using pagination for list files and is using the default of `30` files per page. This means a PR could be incorrectly flagged for missing integration tests if the test files occur after the first 30 files in the PR. Example of this can be seen in aws#22455 This PR enables pagination on the `listFiles` call to ensure the validation rules are looking at all files in the PR, not just the first 30. ---- ### 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*

…Deploy

cplee · 2022-10-28T21:53:22Z

@corymhall - this PR has been rebased since #22295 was merged to main. What needs to be done with this PR?

aws-cdk-automation · 2022-11-01T15:01:50Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2022-11-01T15:01:52Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2022-11-17T00:10:01Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2022-11-18T00:10:22Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2022-11-19T07:09:21Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: b5002f2
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2022-12-10T00:08:33Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2022-12-11T00:09:37Z

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2022-12-19T00:08:30Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

cplee · 2022-12-27T18:02:50Z

This PR was abandoned in favor of creating a new CDK construct on Construct Hub to provide this functionality:

@cdklabs/cdk-ecs-codedeploy

declare const deploymentGroup: codeDeploy.IEcsDeploymentGroup;
declare const taskDefinition: ecs.ITaskDefinition;

new EcsDeployment({
  deploymentGroup,
  targetService: {
    taskDefinition,
    containerName: 'mycontainer',
    containerPort: 80,
  },
});

github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Oct 11, 2022

aws-cdk-automation requested a review from a team October 11, 2022 07:26

mergify bot mentioned this pull request Oct 11, 2022

feat(codedeploy): CodeDeploy deployment construct for ECS - SUPERSEDED by #22455 #22454

Closed

4 tasks

aws-cdk-automation previously requested changes Oct 11, 2022

View reviewed changes

cplee mentioned this pull request Oct 11, 2022

feat(codedeploy): CodeDeploy deployment group construct for ECS #22295

Merged

cplee force-pushed the ecs-deployment branch 4 times, most recently from fa58bc5 to 7269071 Compare October 18, 2022 22:12

cplee mentioned this pull request Oct 19, 2022

chore(prlint): add pagination to listFiles in PR #22555

Merged

4 tasks

cplee force-pushed the ecs-deployment branch 2 times, most recently from e2bc0d7 to 894f36d Compare October 19, 2022 16:56

cplee marked this pull request as ready for review October 19, 2022 18:01

corymhall previously requested changes Oct 21, 2022

View reviewed changes

corymhall self-assigned this Oct 21, 2022

cplee added a commit to cplee/aws-cdk that referenced this pull request Oct 24, 2022

fix: address feedback from PR aws#22455

b19111c

cplee added a commit to cplee/aws-cdk that referenced this pull request Oct 24, 2022

fix: address feedback from PR aws#22455

c4a3528

cplee force-pushed the ecs-deployment branch 2 times, most recently from e1b3979 to 32575ad Compare October 25, 2022 05:30

cplee mentioned this pull request Oct 27, 2022

Blue Green ECS Deployment #1559

Closed

feat(codedeploy): add support for deploying to ECS services with Code…

d10a22f

…Deploy

cplee force-pushed the ecs-deployment branch from 32575ad to d10a22f Compare October 28, 2022 21:04

cplee added 2 commits November 1, 2022 08:26

Merge branch 'main' into ecs-deployment

2ddce93

chore: rename integration test directory

b699b98

corymhall removed their assignment Nov 2, 2022

Merge branch 'main' into ecs-deployment

b5002f2

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Dec 19, 2022

aws-cdk-automation closed this Dec 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(codedeploy): CodeDeploy deployment construct for ECS #22455

feat(codedeploy): CodeDeploy deployment construct for ECS #22455

cplee commented Oct 11, 2022 •

edited

Loading

gitpod-io bot commented Oct 11, 2022

aws-cdk-automation left a comment

cplee commented Oct 19, 2022

Naumel commented Oct 19, 2022

cplee commented Oct 19, 2022

corymhall left a comment

corymhall Oct 21, 2022

cplee Oct 24, 2022

cplee Oct 25, 2022

corymhall Oct 21, 2022

cplee Oct 24, 2022

corymhall Oct 21, 2022

cplee Oct 24, 2022

corymhall Oct 21, 2022

cplee Oct 24, 2022

corymhall Oct 21, 2022

cplee Oct 24, 2022

corymhall Oct 21, 2022

cplee Oct 24, 2022

corymhall Oct 21, 2022

cplee Oct 24, 2022

corymhall Oct 21, 2022

cplee Oct 24, 2022

cplee commented Oct 24, 2022 •

edited

Loading

cplee commented Oct 28, 2022

aws-cdk-automation commented Nov 1, 2022

aws-cdk-automation commented Nov 1, 2022

aws-cdk-automation commented Nov 17, 2022

aws-cdk-automation commented Nov 18, 2022

aws-cdk-automation commented Nov 19, 2022

aws-cdk-automation commented Dec 10, 2022

aws-cdk-automation commented Dec 11, 2022

aws-cdk-automation commented Dec 19, 2022

cplee commented Dec 27, 2022 •

edited

Loading

		@@ -0,0 +1,116 @@
		import { Logger } from '@aws-lambda-powertools/logger';
		import { CodeDeploy } from '@aws-sdk/client-codedeploy';

	* True iff the deployment is in a final state.
	* True if the deployment is in a final state.

feat(codedeploy): CodeDeploy deployment construct for ECS #22455

feat(codedeploy): CodeDeploy deployment construct for ECS #22455

Conversation

cplee commented Oct 11, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Oct 11, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

cplee commented Oct 19, 2022

Naumel commented Oct 19, 2022

cplee commented Oct 19, 2022

corymhall 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

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

Choose a reason for hiding this comment

cplee commented Oct 24, 2022 • edited Loading

cplee commented Oct 28, 2022

aws-cdk-automation commented Nov 1, 2022

aws-cdk-automation commented Nov 1, 2022

aws-cdk-automation commented Nov 17, 2022

aws-cdk-automation commented Nov 18, 2022

aws-cdk-automation commented Nov 19, 2022

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 10, 2022

aws-cdk-automation commented Dec 11, 2022

aws-cdk-automation commented Dec 19, 2022

cplee commented Dec 27, 2022 • edited Loading

cplee commented Oct 11, 2022 •

edited

Loading

cplee commented Oct 24, 2022 •

edited

Loading

cplee commented Dec 27, 2022 •

edited

Loading