-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(EcsDeployAction): define service attributes instead of service itself #17917
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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 the contribution @tobytipton, but this is not a change we will make. You can have the same effect by using the FargateService.fromFargateServiceAttributes()
and Ec2Service.fromEc2ServiceAttributes()
, and passing the name and region (using the ARN) there, and using the existing service
property of EcsDeployAction
. So, this doesn't add anything to the expressivity of the CDK, while making the API more complex.
So looking further at Levaraging
generates the region, configuration correctly.
However if I create the ECS service in the region stack and then "export" out the clusterName and serviceArn, it doesn't set the region correctly, I added the role here because with out it, it was causing
Generates something like this which has
It appears in the working example where the import is using names to import the resource serviceArn is To workaround this had to do define the serviceArn on the stack which creates the service like so rather than trying to use the
that generates the serviceArn to |
Thanks for the detailed explanation @tobytipton! Do you think it's worth it to update our documentation with your findings? |
I think it would be helpful to provide some additional documentation around this. Part of the reason I provided this detail was to make it easier for later reference for others which might be running into a similar scenario. I created my self some test with the above examples for testing them out to make sure I had it working correctly. |
@tobytipton I see you opened #18042, I assume that's the example you're referencing here? Anyway, thanks for all your work on this! |
Sorry I was referring to the example I did in #17917 which is for just the aws-codepipelines Pipeline API #18042 is the example/test for modern CDK PIpelines If you would like I can do test/example for the aws-codepipelines Pipeline API as well. |
If you have the time, it would of course be awesome to get an example for the |
I added the example/test to #18042 if that is ok, since both are leveraging ECS Deploy Action. |
Actually, can you split that out to a separate PR? Those two modules are owned by different owners, so splitting them up will make them easier to review and then merge. |
Separate PR created #18059 |
Thank you! |
This implements feature request #17911 by providing a service attributes to the Deploy Action.
Also increase the tests to cover the the attribute, as well as provides tests where using service fails and how serviceAttributes can resolve the issue.
closes #17911
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license