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 group construct for ECS #22295

Merged
merged 16 commits into from Oct 28, 2022

Conversation

clareliguori
Copy link
Member

@clareliguori clareliguori commented Sep 29, 2022

closes #1559

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 29, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team September 29, 2022 17:12
@github-actions github-actions bot added admired-contributor [Pilot] contributed between 13-24 PRs to the CDK p2 effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p1 and removed p2 labels Sep 29, 2022
@clareliguori
Copy link
Member Author

This PR should replace #22081 (thanks @cplee!)

@clareliguori
Copy link
Member Author

@corymhall, I attempted to add an integ test that actually starts a deployment as we discussed in PR #22159, but ran into some issues. You can see the diff here showing the test:
clareliguori@70ed0ae

I ran into a few issues:

  1. I had expected the test would retry the getDeployment call for a while until receiving the expected output (status = successful). It appears that the stack only calls getDeployment once and then checks the output in a different CFN resource? So the test gives up while the deployment is still in progress.
  2. It appears that the API call custom resource calls the API on every event (create, update, and delete), so while the stack was deleting, it attempted to create yet another deployment by calling createDeployment on delete.
  3. The output of getDeployment is sometimes too big, so I ran into (integ-tests): awsApiCall Response object is too long #22059

Thoughts? Is there another pattern I should follow here? I did confirm that the deployment started by the integ test does eventually succeed, but ideally this would run every time as part of the integ test suite.

@corymhall
Copy link
Contributor

  • I had expected the test would retry the getDeployment call for a while until receiving the expected output (status = successful). It appears that the stack only calls getDeployment once and then checks the output in a different CFN resource? So the test gives up while the deployment is still in progress.

Yeah that is something that I'm working on. Once #22238 I have another PR ready to add waiting for a certain result.

  • It appears that the API call custom resource calls the API on every event (create, update, and delete), so while the stack was deleting, it attempted to create yet another deployment by calling createDeployment on delete.

This should get fixed in #22238.

  • The output of getDeployment is sometimes too big, so I ran into

I don't have a fix yet for this, but I have a couple of ideas.

Thoughts? Is there another pattern I should follow here? I did confirm that the deployment started by the integ test does eventually succeed, but ideally this would run every time as part of the integ test suite.

I think for now as long as you have run the test and manually confirmed that is the best we can do. Just make sure to add a comment block to the integration test detailing the manual steps to follow. Once some of these integ test enhancements land we can revisit.

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've done an initial pass. Is this a new feature that was recently released? I know people have been
asking for this for a while and I haven't seen any announcement or documentation that explains how
to do this.

My biggest concern right now is that while you can configure the actual deployment, you can't
actually perform the deployment? I know that is the first question that people will ask. Unless I am
missing something.

packages/@aws-cdk/aws-codedeploy/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-codedeploy/README.md Outdated Show resolved Hide resolved
@cplee
Copy link
Contributor

cplee commented Sep 29, 2022

My biggest concern right now is that while you can configure the actual deployment, you can't
actually perform the deployment? I know that is the first question that people will ask. Unless I am
missing something.

@corymhall - I have a PR that will follow right after this one to add support for ECS deployments. Would it help to draft that PR now?

@clareliguori
Copy link
Member Author

I've done an initial pass. Is this a new feature that was recently released? I know people have been asking for this for a while and I haven't seen any announcement or documentation that explains how to do this.

This feature was released in 2018, but the CloudFormation release notes show that CloudFormation support for the new attributes in AWS::CodeDeploy::DeploymentGroup was added July 8, 2021. I think no one noticed when CFN support was added? ¯\(ツ)/¯.

My biggest concern right now is that while you can configure the actual deployment, you can't
actually perform the deployment? I know that is the first question that people will ask. Unless I am
missing something.

@cplee noted that he has a follow-up PR for doing in-stack deployments. Though, based on the comments in the original issue, most folks are looking to stand up all the resources in CFN, and then deploy to the service in a pipeline outside of CFN. The current alternative is to use this feature, which is already supported by CDK:
https://aws.amazon.com/about-aws/whats-new/2020/05/aws-cloudformation-now-supports-blue-green-deployments-for-amazon-ecs/

@mergify mergify bot dismissed corymhall’s stale review September 29, 2022 21:33

Pull request has been modified.

@corymhall
Copy link
Contributor

@corymhall - I have a PR that will follow right after this one to add support for ECS deployments. Would it help to draft that PR now?

Yes that would be great! I think that would help me see the full picture

@cplee
Copy link
Contributor

cplee commented Oct 19, 2022

@rix0rrr @corymhall - it looks like all feedback and requested changes have been addressed on this PR. Is there anything else needed to get this PR approved?

(FYI - PR #22455 builds on this PR by adding a construct for creating CodeDeploy deployments. It is ready for review immediately following this one)

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.

@clareliguori sorry for the delay on this review! Thanks for removing the editing of the underlying
service. I think the only thing we need to add now is:

  1. Validation that the service is configured correctly. I think this would just be that the
    deploymentController and taskDefinition are configured correctly.
  2. Configuration of the service (done in the BaseService construct). I think this would just be
    that when the user configures deploymentController: DeploymentControllerType.CodeDeploy that
    the revision is stripped from the task definition.

@mergify mergify bot dismissed corymhall’s stale review October 20, 2022 20:18

Pull request has been modified.

@clareliguori
Copy link
Member Author

@clareliguori sorry for the delay on this review! Thanks for removing the editing of the underlying service. I think the only thing we need to add now is:

1. Validation that the service is configured correctly. I think this would just be that the
   `deploymentController` and `taskDefinition` are configured correctly.

2. Configuration of the service (done in the `BaseService` construct). I think this would just be
   that when the user configures `deploymentController: DeploymentControllerType.CodeDeploy` that
   the revision is stripped from the task definition.

@corymhall these are both added now

@clareliguori
Copy link
Member Author

Ping @corymhall - hopefully we're good to merge now 😄

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.

@clareliguori thanks for the updates. Just one last conversation to resolve!

@mergify mergify bot dismissed corymhall’s stale review October 28, 2022 15:30

Pull request has been modified.

@clareliguori
Copy link
Member Author

@corymhall ah somehow I missed that one, thanks. Should be resolved now

corymhall
corymhall previously approved these changes Oct 28, 2022
@mergify mergify bot dismissed corymhall’s stale review October 28, 2022 16:06

Pull request has been modified.

@clareliguori
Copy link
Member Author

@corymhall Whoops I forgot to update the samples in the readme - can you approve again?

@mergify
Copy link
Contributor

mergify bot commented Oct 28, 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
Copy link
Contributor

mergify bot commented Oct 28, 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: 0637ba4
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit efd24d1 into aws:main Oct 28, 2022
@mergify
Copy link
Contributor

mergify bot commented Oct 28, 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admired-contributor [Pilot] contributed between 13-24 PRs to the CDK effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blue Green ECS Deployment
5 participants