-
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
fix(ecs): imported services don't have account & region set correctly #15944
Conversation
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
Is there anything I can do that might further this along? |
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 PR @robert-lilleker, but this is not a good solution - the region the EcsDeployAction
is taken from the service that is passed to it, it can't be set separately from that.
The correct fix to take the region (and account) of the service from the ARN when it's imported using the fromEc2ServiceAttributes()
/ fromFargateServiceAttributes()
methods. This needs a change here, basically changing this line:
return new Import(scope, id);
to:
return new Import(scope, id, {
environmentFromArn: arn,
});
All of the other changes in this PR should be reverted.
Thanks,
Adam
yarn.lock
Outdated
@@ -1628,7 +1628,7 @@ | |||
dependencies: | |||
"@types/istanbul-lib-report" "*" | |||
|
|||
"@types/jest@^26.0.24": | |||
"@types/jest@^26.0.22", "@types/jest@^26.0.24": |
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.
Changes to this file should be reverted.
d40572c
to
f3b76d9
Compare
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.
Awesome work @robert-lilleker, but we also need a unit test 🙂. Take some inspiration from these.
924c117
to
ebfd5f2
Compare
…aws#16997) This is a fix for the region issue raised by aws#11199 allowing multi regional ecs deployments fixes aws#11199 supersedes aws#15944, merged master and added tests ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This issue was fixed by #16997. |
This is a fix for the region issue raised by #11199 allowing multi regional ecs deployments
fixes #11199
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license