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

fix(ecs): imported services don't have account & region set correctly #15944

Closed
wants to merge 1 commit into from

Conversation

robert-lilleker
Copy link

@robert-lilleker robert-lilleker commented Aug 9, 2021

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

@gitpod-io
Copy link

gitpod-io bot commented Aug 9, 2021

@mergify
Copy link
Contributor

mergify bot commented Aug 9, 2021

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@robert-lilleker robert-lilleker changed the title Ecs deploy region feat(ecs deploy) Ecs deploy region Aug 9, 2021
@robert-lilleker robert-lilleker changed the title feat(ecs deploy) Ecs deploy region feat(aws-codepipeline-actions) Ecs deploy region Aug 9, 2021
@robert-lilleker robert-lilleker changed the title feat(aws-codepipeline-actions) Ecs deploy region feat: Deploy ecr in code pipeline across regions Aug 9, 2021
@robert-lilleker robert-lilleker changed the title feat: Deploy ecr in code pipeline across regions feat(codepipeline): Deploy ecr in code pipeline across regions Aug 9, 2021
@robert-lilleker robert-lilleker changed the title feat(codepipeline): Deploy ecr in code pipeline across regions feat(codepipeline): deploy ecr in code pipeline across regions Aug 10, 2021
@peterwoodworth peterwoodworth added @aws-cdk/aws-codepipeline Related to AWS CodePipeline p1 effort/small Small work item – less than a day of effort labels Aug 11, 2021
@robert-lilleker
Copy link
Author

Is there anything I can do that might further this along?

@skinny85 skinny85 assigned skinny85 and unassigned BenChaimberg Aug 25, 2021
@skinny85 skinny85 self-requested a review August 25, 2021 21:15
Copy link
Contributor

@skinny85 skinny85 left a 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":
Copy link
Contributor

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.

@mergify mergify bot dismissed skinny85’s stale review August 26, 2021 09:20

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: f3b76d9
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Copy link
Contributor

@skinny85 skinny85 left a 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.

@skinny85 skinny85 changed the title feat(codepipeline): deploy ecr in code pipeline across regions fix(ecs): imported services don't have account & region set correctly Sep 2, 2021
@skinny85 skinny85 removed their assignment Oct 12, 2021
mergify bot pushed a commit that referenced this pull request Oct 15, 2021
…#16997)

This is a fix for the region issue raised by #11199 allowing multi regional ecs deployments

fixes #11199

supersedes #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*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…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*
@rix0rrr rix0rrr removed the p1 label Mar 4, 2022
@rix0rrr rix0rrr added feature-request A feature should be added or improved. p1 and removed @aws-cdk/aws-codepipeline Related to AWS CodePipeline effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 labels Mar 4, 2022
@TheRealAmazonKendra
Copy link
Contributor

This issue was fixed by #16997.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[aws-codepipeline-actions] support cross account/region ecs deployment
7 participants