fix(codepipeline-actions): use IBaseService instead of BaseService in EcsDeployActionProps #6412

atsushi-ishibashi · 2020-02-23T16:35:44Z

refer: #6408

Replace BaseService with IBaseService in EcsDeployActionProps

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

aws-cdk-automation · 2020-02-23T16:59:32Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 8be4b51
Result: FAILED
Build Logs (available for 30 days)

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

…-> fromFargateServiceAttributes

aws-cdk-automation · 2020-02-24T09:20:18Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: f38e6f4
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-02-24T09:46:37Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 2333da2
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-02-24T10:59:51Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 441f276
Result: FAILED
Build Logs (available for 30 days)

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

atsushi-ishibashi · 2020-02-24T11:18:30Z

This PR got the below errors.
But ECS Service cannot be defined uniquely by ARN, for example arn:aws:ecs:us-west-2:123456789012:service/my-http-service, which contains only service name and you can create the same name service in different cluster.
So far I have no idea to avoid this incompatible changes🤔
Let me know if there is a better way👍

@aws-cdk/aws-ecs... CHANGES.
Original assembly: @aws-cdk/[email protected]
Updated assembly:  @aws-cdk/[email protected]
API elements with incompatible changes:
err  - METHOD @aws-cdk/aws-ecs.Ec2Service.fromEc2ServiceArn: has been removed [removed:@aws-cdk/aws-ecs.Ec2Service.fromEc2ServiceArn]
err  - METHOD @aws-cdk/aws-ecs.FargateService.fromFargateServiceArn: has been removed [removed:@aws-cdk/aws-ecs.FargateService.fromFargateServiceArn]
err  - PROP @aws-cdk/aws-ecs.IEc2Service.serviceArn: has been removed [removed:@aws-cdk/aws-ecs.IEc2Service.serviceArn]
err  - PROP @aws-cdk/aws-ecs.IFargateService.serviceArn: has been removed [removed:@aws-cdk/aws-ecs.IFargateService.serviceArn]
err  - PROP @aws-cdk/aws-ecs.IService.serviceArn: has been removed [removed:@aws-cdk/aws-ecs.IService.serviceArn]

aws-cdk-automation · 2020-02-24T11:33:00Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 51fc085
Result: FAILED
Build Logs (available for 30 days)

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

skinny85 · 2020-02-24T16:38:35Z

This PR got the below errors.
But ECS Service cannot be defined uniquely by ARN, for example arn:aws:ecs:us-west-2:123456789012:service/my-http-service, which contains only service name and you can create the same name service in different cluster.
So far I have no idea to avoid this incompatible changes🤔
Let me know if there is a better way👍

@aws-cdk/aws-ecs... CHANGES.
Original assembly: @aws-cdk/[email protected]
Updated assembly:  @aws-cdk/[email protected]
API elements with incompatible changes:
err  - METHOD @aws-cdk/aws-ecs.Ec2Service.fromEc2ServiceArn: has been removed [removed:@aws-cdk/aws-ecs.Ec2Service.fromEc2ServiceArn]
err  - METHOD @aws-cdk/aws-ecs.FargateService.fromFargateServiceArn: has been removed [removed:@aws-cdk/aws-ecs.FargateService.fromFargateServiceArn]
err  - PROP @aws-cdk/aws-ecs.IEc2Service.serviceArn: has been removed [removed:@aws-cdk/aws-ecs.IEc2Service.serviceArn]
err  - PROP @aws-cdk/aws-ecs.IFargateService.serviceArn: has been removed [removed:@aws-cdk/aws-ecs.IFargateService.serviceArn]
err  - PROP @aws-cdk/aws-ecs.IService.serviceArn: has been removed [removed:@aws-cdk/aws-ecs.IService.serviceArn]

See this comment on how to achieve this.

You need to remove all of your breaking changes, these are stables modules, so no breaking changes are allowed.

There is already a PR attempting these changes: #6203 , but it's not building ATM, and there's hasn't been much activity on it lately.

skinny85

See: #6412 (comment)

Pull request has been modified.

aws-cdk-automation · 2020-02-24T18:19:57Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 61f2f5f
Result: FAILED
Build Logs (available for 30 days)

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

skinny85 · 2020-02-24T18:39:01Z

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

+  /**
+   * The name of the cluster that hosts the service.
+   */
+  readonly clusterName: string;


This needs to be ICluster. Not clusterName.

skinny85 · 2020-02-24T18:39:39Z

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

+  /**
+   * The service ARN.
+   */
+  readonly serviceArn: string;


There should also be serviceName, and one of serviceArn or serviceName is required.

I added validation to prevent from setting both or neither. If both are set, it may be confusing.

aws-cdk-automation · 2020-02-24T19:04:06Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 8c9e994
Result: SUCCEEDED
Build Logs (available for 30 days)

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

Pull request has been modified.

aws-cdk-automation · 2020-02-24T19:38:19Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b44e309
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-02-24T20:01:47Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: e786f9f
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-02-24T20:47:41Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: bee67c6
Result: SUCCEEDED
Build Logs (available for 30 days)

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

atsushi-ishibashi · 2020-02-25T23:18:09Z

I said to extract the body of fromEc2ServiceAttributes and fromFargateServiceAttributes to a module-private function to reduce duplication. I never said anything about a class.

I will add public static function to class BaseService. If I couldn't understand your intention, please correct me🙇 @skinny85

skinny85 · 2020-02-26T00:15:20Z

I said to extract the body of fromEc2ServiceAttributes and fromFargateServiceAttributes to a module-private function to reduce duplication. I never said anything about a class.

I will add public static function to class BaseService. If I couldn't understand your intention, please correct me🙇 @skinny85

No. I don't want to make this function a part of the public contract of the ECS module. Do this instead:

// file: from-service-attributes.ts
export interface ServiceAttributes {
  // serviceName, serviceArn, cluster
  ...
}

export function fromServiceAtrributes(scope: Construct, id: string, attributes: ServiceAttributes): IBaseService {
  ... 
}

Then call this function from fromEc2ServiceAttributes and fromFargateServiceAttributes. It will work because of TypeScript's structural typing.

Pull request has been modified.

aws-cdk-automation · 2020-02-26T03:49:30Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 6e1d696
Result: SUCCEEDED
Build Logs (available for 30 days)

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

atsushi-ishibashi · 2020-02-26T23:02:50Z

@skinny85 please review again. Thanks

skinny85

Thanks @atsushi-ishibashi , looking good! We're almost there. Just a few last comments before I'm comfortable shipping this.

skinny85 · 2020-02-27T18:53:09Z

packages/@aws-cdk/aws-codepipeline-actions/test/ecs/test.ecs-deploy-action.ts

 import * as codepipeline from '@aws-cdk/aws-codepipeline';
+import { FakeSourceAction } from '@aws-cdk/aws-codepipeline/test/fake-source-action';


Please just use one of the source actions from this package. FakeSourceAction is private to the @aws-codepipeline package.

skinny85 · 2020-02-27T18:53:30Z

packages/@aws-cdk/aws-codepipeline-actions/test/ecs/test.ecs-deploy-action.ts

+      });
+      const artifact = new codepipeline.Artifact('Artifact');
+
+      test.doesNotThrow(() => {


Remove the test.doesNotThrow(), it's redundant.

skinny85 · 2020-02-27T18:54:01Z

packages/@aws-cdk/aws-codepipeline-actions/test/ecs/test.ecs-deploy-action.ts

+      const stack = new cdk.Stack();
+      const service = ecs.FargateService.fromFargateServiceAttributes(stack, 'FargateService', {
+        serviceName: 'my-http-service',
+        cluster: new ecs.Cluster(stack, 'Cluster', {


Import the cluster with a well-known name as well.

You mean like this?

const service = ecs.FargateService.fromFargateServiceAttributes(stack, 'FargateService', { serviceName: 'service-name', cluster: new ecs.Cluster(stack, 'Cluster', { clusterName: 'cluster-name', }), });

I meant more Cluster.fromClusterAttributes(), but this should work as well (I want a specific name in the test assertion, not a CFN Ref expression).

skinny85 · 2020-02-27T18:55:58Z

packages/@aws-cdk/aws-codepipeline-actions/test/ecs/test.ecs-deploy-action.ts

+                Name: 'Source',
+                ActionTypeId: {
+                  Category: "Source",
+                  Provider: "Fake"


No reason for these assertions here, just leave it as:

Stages: [ {}, { "Name": "Deploy", "Actions": [ { // ... } ], } ]

skinny85 · 2020-02-27T18:57:02Z

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

+  /**
+   * The service ARN.
+   *
+   * @default - generated from serviceName


This comment is wrong (should be "either this, or {@link serviceName}, is required").

skinny85 · 2020-02-27T18:57:17Z

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

+  /**
+   * The name of the service.
+   *
+   * @default - generated from serviceArn


This comment is wrong (should be "either this, or {@link serviceArn}, is required").

Is @default - {@link serviceArn}, is required what you want?
Optional property must have @default documentation.

I want:

/** * The name of the imported service. * * @default - either this, or {@link serviceArn}, is required */ readonly serviceName?: string;

skinny85 · 2020-02-27T18:58:28Z

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

+  /**
+   * The service ARN.
+   *
+   * @default - generated from serviceName


This comment is wrong (should be "either this, or {@link serviceName}, is required").

skinny85 · 2020-02-27T18:58:35Z

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

+  /**
+   * The name of the service.
+   *
+   * @default - generated from serviceArn


This comment is wrong (should be "either this, or {@link serviceArn}, is required").

skinny85 · 2020-02-27T18:59:03Z

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

+  /**
+   * The service ARN.
+   *
+   * @default - generated from serviceName


This comment is wrong (should be "either this, or {@link serviceName}, is required").

skinny85 · 2020-02-27T19:00:17Z

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

+  /**
+   * The name of the service.
+   *
+   * @default - generated from serviceArn


This comment is wrong (should be "either this, or {@link serviceArn}, is required").

Pull request has been modified.

aws-cdk-automation · 2020-02-28T02:50:21Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 583b8af
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-02-28T09:42:15Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: fee33f2
Result: SUCCEEDED
Build Logs (available for 30 days)

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

atsushi-ishibashi · 2020-03-02T09:36:25Z

Ready for review👍 @skinny85
If you need update branch, please do it! I may fall in asleep

aws-cdk-automation · 2020-03-02T10:20:53Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 2a4e1f3
Result: SUCCEEDED
Build Logs (available for 30 days)

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

atsushi-ishibashi · 2020-03-04T01:42:25Z

@skinny85 I'd like you to include this PR in the next release:bow: In out product, we're constructing CD pipeline refering existing ECS services. So this feature is important to proceed it.
Thanks!

atsushi-ishibashi · 2020-03-05T20:33:13Z

@skinny85 @SomayaB
Please review this PR. Sorry to remind you many times🙏

jkdilunika · 2020-03-08T03:05:56Z

@skinny85 really appreciate if you could merge this fix to next release of CDK (if it has solved the blocking issue). I have to carry forward my blocked pipeline ticket for the third sprint now. I was criticized during last sprint retro for picking CDK for this project :). I will be more careful next time when picking new AWS frameworks for my work place projects ...

skinny85 · 2020-03-11T15:24:01Z

Hey all,

sorry, I was out on vacation the last week and a half. Will review soon.

Thanks,
Adam

skinny85

Thanks for your patience @atsushi-ishibashi !

mergify · 2020-03-11T21:52:48Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-03-11T22:39:05Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 5310ca4
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-03-11T22:39:30Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

use IService instead of BaseService

8be4b51

fromEc2ServiceArn -> fromEc2ServiceAttributes, fromFargateServiceArn …

f38e6f4

…-> fromFargateServiceAttributes

fix ci error

2333da2

fix build error

441f276

Merge branch 'master' into replace-iservice

51fc085

atsushi-ishibashi requested review from skinny85 and eladb February 24, 2020 11:26

skinny85 previously requested changes Feb 24, 2020

View reviewed changes

avoid breaking changes

61f2f5f

atsushi-ishibashi changed the title ~~fix(codepipeline-actions): use IService instead of BaseService in EcsDeployActionProps~~ fix(codepipeline-actions): use IBaseService instead of BaseService in EcsDeployActionProps Feb 24, 2020

add IBaseService to implements of BaseService

8c9e994

skinny85 previously requested changes Feb 24, 2020

View reviewed changes

skinny85 mentioned this pull request Feb 24, 2020

CodePipeline EcsDeployAction - unable to refer existing Fargate Service #6178

Closed

change to ICluster, add serviceName

b44e309

fix build error

e786f9f

fix build error

bee67c6

create fromServiceAtrributes, remove ImportedBaseService. Some tests

6e1d696

skinny85 previously requested changes Feb 27, 2020

View reviewed changes

update comment, tests

583b8af

update comments, tests

fee33f2

Merge branch 'master' into replace-iservice

2a4e1f3

atsushi-ishibashi requested a review from skinny85 March 5, 2020 00:45

skinny85 approved these changes Mar 11, 2020

View reviewed changes

Merge branch 'master' into replace-iservice

5310ca4

mergify bot merged commit bed5357 into aws:master Mar 11, 2020

This was referenced Mar 12, 2020

fix: Iservice for ecsdeployaction #6203

Closed

CodePipeline ClusterName & ServiceName deployment using AWS CDK #4375

Closed

skinny85 mentioned this pull request Mar 24, 2020

Use IService instead of BaseService in EcsDeployActionProps? #6408

Closed

snyk-bot mentioned this pull request Apr 10, 2020

[Snyk] Upgrade aws-cdk from 1.28.0 to 1.30.0 ammobinDOTca/ammobin-cdk#2

Closed

		import * as codepipeline from '@aws-cdk/aws-codepipeline';
		import { FakeSourceAction } from '@aws-cdk/aws-codepipeline/test/fake-source-action';

fix(codepipeline-actions): use IBaseService instead of BaseService in EcsDeployActionProps #6412

fix(codepipeline-actions): use IBaseService instead of BaseService in EcsDeployActionProps #6412

Conversation

atsushi-ishibashi commented Feb 23, 2020 • edited Loading

aws-cdk-automation commented Feb 23, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Feb 24, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Feb 24, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Feb 24, 2020

AWS CodeBuild CI Report

atsushi-ishibashi commented Feb 24, 2020

aws-cdk-automation commented Feb 24, 2020

AWS CodeBuild CI Report

skinny85 commented Feb 24, 2020

skinny85 left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 24, 2020

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 24, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Feb 24, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Feb 24, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Feb 24, 2020

AWS CodeBuild CI Report

atsushi-ishibashi commented Feb 25, 2020 • edited Loading

skinny85 commented Feb 26, 2020 • edited Loading

aws-cdk-automation commented Feb 26, 2020

AWS CodeBuild CI Report

atsushi-ishibashi commented Feb 26, 2020

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 28, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Feb 28, 2020

AWS CodeBuild CI Report

atsushi-ishibashi commented Mar 2, 2020

aws-cdk-automation commented Mar 2, 2020

AWS CodeBuild CI Report

atsushi-ishibashi commented Mar 4, 2020

atsushi-ishibashi commented Mar 5, 2020

jkdilunika commented Mar 8, 2020

skinny85 commented Mar 11, 2020

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 11, 2020

aws-cdk-automation commented Mar 11, 2020

AWS CodeBuild CI Report

mergify bot commented Mar 11, 2020

atsushi-ishibashi commented Feb 23, 2020 •

edited

Loading

atsushi-ishibashi Feb 24, 2020 •

edited

Loading

atsushi-ishibashi commented Feb 25, 2020 •

edited

Loading

skinny85 commented Feb 26, 2020 •

edited

Loading