-
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 group construct for ECS #22295
Conversation
@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: I ran into a few issues:
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. |
Yeah that is something that I'm working on. Once #22238 I have another PR ready to add waiting for a certain result.
This should get fixed in #22238.
I don't have a fix yet for this, but I have a couple of ideas.
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. |
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'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.
@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? |
This feature was released in 2018, but the CloudFormation release notes show that CloudFormation support for the new attributes in
@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: |
Pull request has been modified.
Co-authored-by: Cory Hall <43035978+corymhall@users.noreply.github.com>
Yes that would be great! I think that would help me see the full picture |
Co-authored-by: Casey Lee <cplee@nektos.com>
Pull request has been modified.
…t, fix samples in README
@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) |
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.
@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:
- Validation that the service is configured correctly. I think this would just be that the
deploymentController
andtaskDefinition
are configured correctly. - Configuration of the service (done in the
BaseService
construct). I think this would just be
that when the user configuresdeploymentController: DeploymentControllerType.CodeDeploy
that
the revision is stripped from the task definition.
Pull request has been modified.
packages/@aws-cdk/aws-elasticloadbalancingv2/lib/nlb/network-listener.ts
Show resolved
Hide resolved
@corymhall these are both added now |
Ping @corymhall - hopefully we're good to merge now 😄 |
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.
@clareliguori thanks for the updates. Just one last conversation to resolve!
Pull request has been modified.
@corymhall ah somehow I missed that one, thanks. Should be resolved now |
Pull request has been modified.
@corymhall Whoops I forgot to update the samples in the readme - can you approve again? |
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 |
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). |
closes #1559
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license