fix(ecs): fix serviceArn token to have region/account in serviceArn by tobytipton · Pull Request #18382 · aws/aws-cdk

tobytipton · 2022-01-12T15:19:07Z

This fixes #18246 by changing the serviceArn to token each part vs creating one token which allows for using the serviceArn to get the environment.

Fixes #18246

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2022-01-12T15:19:10Z

madeline-k

See my comment on #18140 (comment) as well

madeline-k · 2022-01-25T00:44:41Z

packages/@aws-cdk/aws-ecs/lib/base/from-service-attributes.ts

-    name = stack.splitArn(arn, ArnFormat.SLASH_RESOURCE_NAME).resourceName as string;
+    const resourceName = stack.splitArn(arn, ArnFormat.SLASH_RESOURCE_NAME).resourceName as string;
+    const resourceNameSplit = resourceName.split('/');
+    name = resourceNameSplit.length === 1 ? resourceName : resourceNameSplit[1];


This part is overlapping with the changes that @jumic is making in this PR https://github.com/aws/aws-cdk/pull/18140/files.

Let's keep this PR to only changing serviceArn public property for Ec2 and Fargate Service that are created with the BaseService constructor. And we can leave the necessary changes for the serviceArn and serviceName public props for fromXXX() services to @jumic 's PR.

Same comment for the changes in ec2-service.ts and fargate-service.ts

madeline-k · 2022-01-25T00:46:10Z

packages/@aws-cdk/aws-ecs/test/ec2/ec2-service.test.ts


  describe('When import an EC2 Service', () => {
-    test('with serviceArn', () => {
+    test('with old serviceArn', () => {


Testing the new vs old service ARN format should be a part of #18140. It is out of scope for this one.

madeline-k · 2022-01-25T00:47:11Z

packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts


  describe('When import a Fargate Service', () => {
-    test('with serviceArn', () => {
+    test('with old serviceArn', () => {


These tests are also out of scope

madeline-k · 2022-01-25T00:49:05Z

packages/@aws-cdk/aws-ecs/test/ec2/ec2-service.test.ts

+      const importService = ecs.Ec2Service.fromEc2ServiceAttributes(stack, 'importService', {
+        cluster: cluster,
+        serviceArn: service.serviceArn,
+      });
+      expect(importService.serviceArn).toContain(`service/${cluster.clusterName}/${service.serviceName}`);
+      expect(importService.serviceName).toEqual(service.serviceName);


this should be it's own test once the fromXXX() issues are fixed in #18140

skinny85 · 2022-01-25T01:01:43Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts


-    this.serviceArn = this.getResourceArnAttribute(this.resource.ref, {
+    this.serviceName = this.getResourceNameAttribute(this.resource.attrName);
+    this.serviceArn = Stack.of(this).formatArn({


Note: this needs to keep using the getResourceArnAttribute() helper - otherwise, things will break.

Is there a better approach? The getResourceArnAttribute() generates a token which can't be parsed for region, account, clusterName or serviceName. Which means when creating the new service to get the ARN which can be leveraged to be imported you have to do this to generate the code.

The problem here is most likely the ${cluster.clusterName} part we are referencing here. We are investigating right now to confirm that 🙂.

@madeline-k did you ever conclude your investigation about that suspicious ${cluster.clusterName} expression causing problems?

Pull request has been modified.

TheRealAmazonKendra · 2022-07-29T00:02:50Z

@Mergifyio update

mergify · 2022-07-29T00:03:26Z

update

✅ Branch has been successfully updated

aws-cdk-automation · 2022-07-29T00:25:15Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 3d82774
Result: FAILED
Build Logs (available for 30 days)

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

TheRealAmazonKendra

@tobytipton We'll need the build to be passing before we can provide another review of this. It looks like your update broke the tests in another package so please take a look at those.

Additionally, please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests). In a fix, the title should state the problem that is being fixed, now what the fix itself is.

aws-cdk-automation · 2022-08-20T00:08:05Z

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2022-08-27T00:08:54Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

fix serviceArn token to have region/account in serviceArn

ee6f038

github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Jan 12, 2022

github-actions bot assigned madeline-k Jan 12, 2022

updated expected response based on serviceArn change

7476b26

madeline-k mentioned this pull request Jan 25, 2022

fix(ecs): new arn format not supported (under feature flag) #18140

Merged

madeline-k previously requested changes Jan 25, 2022

View reviewed changes

skinny85 reviewed Jan 25, 2022

View reviewed changes

remove tests related to parsing new ARN format

7a4bad9

rix0rrr added bug This issue is a bug. p1 and removed @aws-cdk/aws-ecs Related to Amazon Elastic Container labels Mar 4, 2022

TheRealAmazonKendra changed the base branch from v1-main to main July 6, 2022 22:26

github-actions bot added the effort/small Small work item – less than a day of effort label Jul 6, 2022

Merge branch 'main' into ecsServiceArnToken

129971e

Merge branch 'main' into ecsServiceArnToken

3d82774

TheRealAmazonKendra requested changes Aug 2, 2022

View reviewed changes

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Aug 27, 2022

aws-cdk-automation closed this Aug 27, 2022

Conversation

tobytipton commented Jan 12, 2022

Fixes #18246

Uh oh!

gitpod-io bot commented Jan 12, 2022

Uh oh!

madeline-k left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra commented Jul 29, 2022

Uh oh!

mergify bot commented Jul 29, 2022

✅ Branch has been successfully updated

Uh oh!

aws-cdk-automation commented Jul 29, 2022

AWS CodeBuild CI Report

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Aug 20, 2022

Uh oh!

aws-cdk-automation commented Aug 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants