feat(appmesh): remove fromName() methods and replace with fromAttributes() #11266

dfezzie · 2020-11-03T04:36:28Z

Follow Up from #10879

BREAKING CHANGE: all fromResourceName() methods in the AppMesh module have been replaced with fromResourceAttributes()

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

…ttributes()

gitpod-io · 2020-11-03T04:36:31Z

eladb · 2020-11-03T10:11:10Z

packages/@aws-cdk/aws-appmesh/lib/gateway-route.ts

   */
  public virtualGateway: IVirtualGateway;

-  constructor(scope: Construct, id: string, props: GatewayRouteAttributes) {
+  constructor(scope: Construct, id: string, attrs?: GatewayRouteAttributes, gatewayRouteArn?: string) {


change to props?

I can do that. I changed it since the fromAttributes methods asked for the parameter to be named attrs

Is there a general rule of thumb when it should be props vs attrs?

I think what Elad is saying is to add an ImportedGatewayRouteProps interface that extends GatewayRouteAttributes and contains the gatewayRouteArn property, instead of having 4 arguments to this constructor.

Got it. Makes sense

eladb · 2020-11-03T10:11:44Z

packages/@aws-cdk/aws-appmesh/lib/gateway-route.ts

-   * The name of the Virtual Gateway this GatewayRoute is associated with
-   */
-  readonly virtualGateway?: IVirtualGateway;
+  readonly virtualGateway: IVirtualGateway;


In EKS, for example we actually only take ARNs here and not strongly-typed objects.

@skinny85 asked for these to be strongly typed. An ARN would work for this, but this using the constructs is more explicit.

Yes, we always use the strongly-typed properties for these, surprised EKS does something different.

skinny85

Good first attempt, but I'd like us to use a different structure that doesn't have these weird ifs inside of the imported classes.

skinny85 · 2020-11-03T18:53:48Z

packages/@aws-cdk/aws-appmesh/lib/gateway-route.ts

-      this.gatewayRouteArn = props.gatewayRouteArn;
-      this.gatewayRouteName = cdk.Fn.select(4, cdk.Fn.split('/', cdk.Stack.of(scope).parseArn(props.gatewayRouteArn).resourceName!));
-      this.virtualGateway = VirtualGateway.fromVirtualGatewayArn(this, 'virtualGateway', props.gatewayRouteArn);
+    if (gatewayRouteArn) {


I really dislike this structure. If there is an if like this in the code, that suggests to me this class is not needed. I also really hate the "dead code" that throws the validation exception.

How about we do this instead?

public static fromGatewayRouteArn(scope: Construct, id: string, gatewayRouteArn: string): IGatewayRoute { return new class extends cdk.Resource implements IGatewayRoute { readonly gatewayRouteArn = gatewayRouteArn; readonly gatewayRouteName = cdk.Fn.select(4, cdk.Fn.split('/', cdk.Stack.of(scope).parseArn(gatewayRouteArn).resourceName!)); readonly virtualGateway = VirtualGateway.fromVirtualGatewayArn(this, 'virtualGateway', gatewayRouteArn); }(scope, id); } public static fromGatewayRouteAttributes(scope: Construct, id: string, attributes: GatewayRouteAttributes): IGatewayRoute { return new class extends cdk.Resource implements IGatewayRoute { readonly gatewayRouteName = attributes.gatewayRouteName; readonly gatewayRouteArn = cdk.Stack.of(scope).formatArn({ service: 'appmesh', resource: `mesh/${attributes.virtualGateway.mesh.meshName}/virtualGateway/${attributes.virtualGateway.virtualGatewayName}/gatewayRoute`, resourceName: this.gatewayRouteName, }); readonly virtualGateway = attributes.virtualGateway; }(scope, id); }

Same comment for all of the other resources.

Got it. I'll make this change

skinny85 · 2020-11-03T19:07:19Z

packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

-  /**
-   * Utility method to add a new GatewayRoute to the VirtualGateway
-   */
-  addGatewayRoute(id: string, route: GatewayRouteBaseProps): GatewayRoute;


Why was this removed?

My original thought was that this does not make sense to have in the IVirtualGateway resource. If we were to implement the inline class you suggested, we would need to also implement the addGatewayRoute method for that class, right?

Thinking about it more, calling addGatewayRoute would be a valid call for imported virtual gateways, but you could accomplish the same thing by passing the imported gateway to the Gateway Route constructor. What are your thoughts on this?

The way we deal with this usually in the CDK is to have an abstract module-private class called <Resource>Base (so in this case, VirtualGatewayBase) that implements the resource interface (so IVirtualGateway in this case), and that class contains the implementation of the methods like addGatewayRoute. In that case, the inline class should extend from VirtualGatewayBase instead of cdk.Resource:

public static fromVirtualGatewayAttributes(scope: Construct, id: string, attrs: VirtualGatewayAttributes): IVirtualGateway { return new class extends VirtualGatewayBase { // ...

Apparently there's already VirtualGatewayBase, so your work is easier:

aws-cdk/packages/@aws-cdk/aws-appmesh/lib/virtual-gateway.ts

Line 75 in 8c9321b

abstract class VirtualGatewayBase extends cdk.Resource implements IVirtualGateway {

Looking at other classes, it looks like they implement a ResourceBase class in instances like this. I will do the same here

skinny85 · 2020-11-03T19:14:18Z

packages/@aws-cdk/aws-appmesh/test/test.gateway-route.ts

@@ -133,20 +133,36 @@ export = {
    },
  },

-  'Can export and import GatewayRoutes and perform actions'(test: Test) {
+  'Can import Gateway Routes using ARN and attributes'(test: Test) {


Can you leave this test unchanged, and add a new one for fromGatewayRouteAttributes please?

packages/@aws-cdk/aws-appmesh/test/test.route.ts

skinny85 · 2020-11-03T19:17:19Z

packages/@aws-cdk/aws-appmesh/test/test.virtual-gateway.ts

-    test.equal(virtualGateway2.mesh.meshName, 'testMesh');
-    test.equal(virtualGateway2.virtualGatewayName, 'test-gateway');
+    test.equal(virtualGateway2.mesh.meshName, meshName);
+    test.equal(virtualGateway2.virtualGatewayName, virtualGatewayName);


Same comment here - leave the old test as-is, and create a new one for fromVirtualGatewayAttributes.

skinny85 · 2020-11-03T19:17:39Z

packages/@aws-cdk/aws-appmesh/test/test.virtual-node.ts


+    // WHEN
+    const virtualNode2 = appmesh.VirtualNode.fromVirtualNodeArn(
+      stack, 'importedVirtualNode2', arn);


Same comment here - leave the old test as-is, and create a new one for fromVirtualNodeAttributes .

The old test here does not make much sense. It adds a listener to node2 but never does any sort of validation on it. Adding the listener to the imported class does not actually make much sense, as it is modeled outside of the CDK and making a mutating call on it would not be valid.

I can revert it, but it does not provide much value.

skinny85 · 2020-11-03T19:17:59Z

packages/@aws-cdk/aws-appmesh/test/test.virtual-router.ts

@@ -304,15 +304,29 @@ export = {
    },
  },

-  'can import a virtual router'(test: Test) {
+  'Can import Virtual Routers using ARN and attributes'(test: Test) {


Same comment here - leave the old test as-is, and create a new one for fromVirtualRouterAttributes .

skinny85 · 2020-11-03T19:18:29Z

packages/@aws-cdk/aws-appmesh/test/test.virtual-service.ts

+    const virtualRouter2 = appmesh.VirtualRouter.fromVirtualRouterArn(stack, 'importedVirtualRouter2', arn);
+    // THEN
+    test.equal(virtualRouter2.mesh.meshName, meshName);
+    test.equal(virtualRouter2.virtualRouterName, virtualServiceName);


Same comment here - leave the old test as-is, and create a new one for fromVirtualServiceAttributes.

Pull request has been modified.

skinny85

Looks great, thanks @dfezzie !

mergify · 2020-11-03T22:48:54Z

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).

Pull request has been modified.

skinny85

Snuck in 2 minor last comments since Mergify dismissed my review for some reason 😜

skinny85 · 2020-11-05T02:00:55Z

packages/@aws-cdk/aws-appmesh/README.md

+## Importing Resources
+
+Each resource comes with two static methods for importing a reference to an existing App Mesh resource.
+There are two static methods, `from<Resource>Arn` and `from<Resource>Attributes` where the `<Resource>` is replaced with the resource name.


I guess Mesh is an exception to the rule...?

hm. Fair point..

Updated to include info about importing meshes

packages/@aws-cdk/aws-appmesh/lib/route.ts

dfezzie · 2020-11-05T03:52:36Z

Snuck in 2 minor last comments since Mergify dismissed my review for some reason 😜

It was due to the README not being updated with the feat(...) tag. Figure it would be good to get those documented. 😁

mergify · 2020-11-05T17:24:15Z

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).

Pull request has been modified.

mergify · 2020-11-06T20:12:11Z

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-11-06T20:40:26Z

AWS CodeBuild CI Report

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

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

mergify · 2020-11-06T20:40:47Z

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).

refactor(appmesh): remove from*Name() methods and replace with from*A…

b83a8ee

…ttributes()

eladb reviewed Nov 3, 2020

View reviewed changes

skinny85 previously requested changes Nov 3, 2020

View reviewed changes

Dominic Fezzie added 2 commits November 3, 2020 12:51

remove the import classes and replace with inline class declarations

f53354c

add addGatewayRoute back to IVirtualGateway

2b60aab

dfezzie requested a review from skinny85 November 3, 2020 20:56

Update comment on GatewayRouteAttributes.virtualGateway

0d225a8

skinny85 previously approved these changes Nov 3, 2020

View reviewed changes

skinny85 changed the title ~~refactor(appmesh): remove from*Name() methods and replace with from*Attributes()~~ feat(appmesh): remove from*Name() methods and replace with from*Attributes() Nov 3, 2020

mergify bot and others added 2 commits November 3, 2020 22:48

Merge branch 'master' into fix/fromResourceAttr

dbcbc08

update README to contain information about importing resources

f84669b

Merge branch 'master' into fix/fromResourceAttr

bc5028c

dfezzie requested a review from skinny85 November 5, 2020 00:17

skinny85 reviewed Nov 5, 2020

View reviewed changes

addresses feedback

30f2704

skinny85 previously approved these changes Nov 5, 2020

View reviewed changes

Merge branch 'master' into fix/fromResourceAttr

be6be66

SomayaB assigned skinny85 Nov 5, 2020

Merge branch 'master' into fix/fromResourceAttr

c28dacd

skinny85 approved these changes Nov 6, 2020

View reviewed changes

Merge branch 'master' into fix/fromResourceAttr

f3c0646

mergify bot merged commit 13d713e into aws:master Nov 6, 2020

dfezzie deleted the fix/fromResourceAttr branch November 6, 2020 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appmesh): remove fromName() methods and replace with fromAttributes() #11266

feat(appmesh): remove fromName() methods and replace with fromAttributes() #11266

dfezzie commented Nov 3, 2020 •

edited by skinny85

Loading

gitpod-io bot commented Nov 3, 2020 •

edited

Loading

eladb Nov 3, 2020

dfezzie Nov 3, 2020

dfezzie Nov 3, 2020

skinny85 Nov 3, 2020

dfezzie Nov 3, 2020

eladb Nov 3, 2020

dfezzie Nov 3, 2020

skinny85 Nov 3, 2020

skinny85 left a comment

skinny85 Nov 3, 2020

dfezzie Nov 3, 2020

skinny85 Nov 3, 2020

dfezzie Nov 3, 2020

skinny85 Nov 3, 2020

skinny85 Nov 3, 2020

dfezzie Nov 3, 2020

skinny85 Nov 3, 2020

skinny85 Nov 3, 2020

skinny85 Nov 3, 2020

dfezzie Nov 3, 2020

skinny85 Nov 3, 2020

skinny85 Nov 3, 2020

skinny85 left a comment

mergify bot commented Nov 3, 2020

skinny85 left a comment

skinny85 Nov 5, 2020

dfezzie Nov 5, 2020

dfezzie Nov 5, 2020 •

edited

Loading

dfezzie commented Nov 5, 2020

mergify bot commented Nov 5, 2020

mergify bot commented Nov 6, 2020

aws-cdk-automation commented Nov 6, 2020

mergify bot commented Nov 6, 2020

feat(appmesh): remove from*Name() methods and replace with from*Attributes() #11266

feat(appmesh): remove from*Name() methods and replace with from*Attributes() #11266

Conversation

dfezzie commented Nov 3, 2020 • edited by skinny85 Loading

gitpod-io bot commented Nov 3, 2020 • edited Loading

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

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

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 3, 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

dfezzie Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

dfezzie commented Nov 5, 2020

mergify bot commented Nov 5, 2020

mergify bot commented Nov 6, 2020

aws-cdk-automation commented Nov 6, 2020

AWS CodeBuild CI Report

mergify bot commented Nov 6, 2020

feat(appmesh): remove fromName() methods and replace with fromAttributes() #11266

feat(appmesh): remove fromName() methods and replace with fromAttributes() #11266

dfezzie commented Nov 3, 2020 •

edited by skinny85

Loading

gitpod-io bot commented Nov 3, 2020 •

edited

Loading

dfezzie Nov 5, 2020 •

edited

Loading