Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(ecs): add
BaseService.fromServiceArnWithCluster()
for use in CodePipeline #18530feat(ecs): add
BaseService.fromServiceArnWithCluster()
for use in CodePipeline #18530Changes from 7 commits
fb55002
1721144
aa35e12
6dfaf31
fc96ff4
9160bc0
d1a1fd5
25a0e6b
d7a7ef7
359f1ef
f84c6ae
3b7afce
e83ae42
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We might want to add an note in here that for cross accounts you will want to provide the role, because if not a role name will be generated for you which doesn't actually get created, what do you think?
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.
Hmm, not sure I understand what you mean...?
If the Action is cross-account, a new Role, with a well-known name, will be generated for you in the correct account (if you don't specify the
role
property).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.
If you don't provide role on the ECSDeployAction a roleArn will be provided in the code pipeline actions for you.
If you are deploying across accounts, that role will most likely not exist in the other account.
From my ecs deploy action test.
In order for the deployment to work I would need to create an IAM Role with the name
pipelinestack-support-serloyecsactionrole49867f847238c85af7c0
in the service account, in order for the pipeline to be able to deploy.In a self-mutating CDK pipeline the self mutation will actually fail because the account doesn't exist so it can't update the Policy for the KMS key, bucket and Pipeline Role's Policy. I would guess an normal pipeline might have the same issue as well.
Providing an IAM role which is already created in the other account resolves that, typically we have been leveraging the CDK bootstrap deploy role and adding permissions to deploy ECS matching this is typically how we have been working around this. Write up from other PR on role.
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 mean, that's not just some random Role name that we just make up 🙂. There is another Stack in the application that gets generated, in the target account, that contains that Role, and the Pipeline Stack depends on that Stack, so
cdk deploy PipelineStack
should correctly include it, deploy it first, and then everything should work.If it doesn't, that's a bug we need to fix.
However, it's fine if you want to mention the Role in the ReadMe.
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.
Interesting, I guess I never noticed that other support stack being created for the service account, to create that role. I have see the other stack which is in the other region from the pipeline which gets generated in the pipeline account. I added a little blurb about the role, I know I have seen issues with the CDK pipeline when adding a new region having an issue if the role referenced hasn't been created.
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.
Let's add a note in the docs that the service ARN must be in the "new" format, with maybe a link to the AWS docs about it.
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.
Let's add a note in these docs that the Cluster returned does not allow accessing the
vpc
property.