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

feat(appmesh): change VirtualRouter's Listener to a union-like class #11277

Merged
merged 17 commits into from
Nov 7, 2020

Conversation

dfezzie
Copy link
Contributor

@dfezzie dfezzie commented Nov 3, 2020

Follows the pattern set in #10879 and transitions the VirtualRouter listener to use protocol specific listeners.

BREAKING CHANGE: VirtualRouter's Listeners are no longer a struct; use the static factory methods of the VirtualNodeListener class to obtain instances of them

  • appmesh: VirtualRouter accepts a list of listeners instead of a single listener

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 Nov 3, 2020

@dfezzie
Copy link
Contributor Author

dfezzie commented Nov 3, 2020

Noticed a failure with the ECS patterns library. Will get a fix for this soon

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.

Gotta say, I don't see the point of this abstraction currently. Unless it's in preparation for more listener properties in the future? But if that's the case, we should have a separate interface per static factory method right now (it's fine if they're empty and just extend RouterListenerProps today).

Also, you've missed updating the ReadMe, and noting the breaking change to VirtualRouter (going from listener to listeners).

/**
* Returns a TCP Listener for a VirtualRouter
*/
public static tcpGatewayListener(props: RouterListenerProps = {}): VirtualRouterListener {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any chance any of these will have protocol-specific properties later? Because frankly, in its current form, I don't think this abstraction is buying us anything...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Virtual Routers specifically do not currently have any protocol specific properties. The main reason I think we should do this change is for consistency. If we have listeners on Virtual Nodes and Virtual Gateways, having listeners for your virtual routers that are defined in a different way seems like poor UX. If you don't think it is worth the change, we can scrap this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency is not a bad argument.

How about this. Let's make port?: number an optional positional argument to each of the static factory methods in this class, and get rid of RouterListenerProps completely. Then, creating a Listener will be as simple as calling VirtualRouterListener.http(8081). I think that's a reasonable compromise between consistency and ease of use.

Thoughts on this idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. Simpler for sure. Made the update

/**
* Protocol the listener implements
*/
protected protocol: Protocol = Protocol.HTTP;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you instead make the base class abstract with a public abstract readonly protocol: Protocol property, and then each of the subclasses will implement protocol?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we can further simplify this by only having one subclass for all 4 protocols.

packages/@aws-cdk/aws-appmesh/lib/virtual-router.ts Outdated Show resolved Hide resolved
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.

My replies to the questions. I think it makes sense to do this PR, just with some slight tweaks.

/**
* Represents the properties needed to define Listeners for a VirtualRouter
*/
export interface RouterListenerProps {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be *Options, so RouterListenerOptions. *Props are reserved for construction properties of Constructs. Apologies I did not notice this before (we'll probably have to go back and change a few of these in other places in the AppMesh module).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. Should updating other places be done in this in this PR or separate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it separate.

/**
* Returns a TCP Listener for a VirtualRouter
*/
public static tcpGatewayListener(props: RouterListenerProps = {}): VirtualRouterListener {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency is not a bad argument.

How about this. Let's make port?: number an optional positional argument to each of the static factory methods in this class, and get rid of RouterListenerProps completely. Then, creating a Listener will be as simple as calling VirtualRouterListener.http(8081). I think that's a reasonable compromise between consistency and ease of use.

Thoughts on this idea?

@mergify mergify bot dismissed skinny85’s stale review November 5, 2020 06:12

Pull request has been modified.

@dfezzie dfezzie requested a review from skinny85 November 5, 2020 17:30
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.

Looks great! We're almost there, a few comments.

Also, missing ReadMe changes.

@mergify mergify bot dismissed skinny85’s stale review November 5, 2020 21:15

Pull request has been modified.

@dfezzie dfezzie requested a review from skinny85 November 5, 2020 21:46
@skinny85 skinny85 changed the title refactor(appmesh): changes VirtualRouter to have static listener methods feat(appmesh): changes VirtualRouter to have static listener methods Nov 5, 2020
@skinny85 skinny85 changed the title feat(appmesh): changes VirtualRouter to have static listener methods feat(appmesh): change VirtualRouter's Listener to a union-like class Nov 5, 2020
skinny85
skinny85 previously approved these changes Nov 5, 2020
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.

LGTM!

@mergify mergify bot dismissed skinny85’s stale review November 6, 2020 21:21

Pull request has been modified.

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.

One small nitpick.

@mergify mergify bot dismissed skinny85’s stale review November 7, 2020 00:03

Pull request has been modified.

@dfezzie dfezzie requested a review from skinny85 November 7, 2020 00:04
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.

Can the PortMapping interface be removed with this change?

@skinny85
Copy link
Contributor

skinny85 commented Nov 7, 2020

Can the PortMapping interface be removed with this change?

I guess no, because it's still used in the current version of VirtualNodeListener.

@mergify
Copy link
Contributor

mergify bot commented Nov 7, 2020

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Nov 7, 2020

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

@mergify mergify bot merged commit 0a3e980 into aws:master Nov 7, 2020
@dfezzie dfezzie deleted the refactor/virtualRouterListener branch November 7, 2020 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants