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(aws-ecs-builder): RFC 219 - An extendable service class for AWS ECS #10129
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial comments from an App Mesh / general application networking perspective.
Generally, love the approach and excited to build on this concept further!
packages/@aws-containers/aws-ecs-builder/lib/extensions/appmesh.ts
Outdated
Show resolved
Hide resolved
packages/@aws-containers/aws-ecs-builder/lib/extensions/appmesh.ts
Outdated
Show resolved
Hide resolved
packages/@aws-containers/aws-ecs-builder/lib/extensions/appmesh.ts
Outdated
Show resolved
Hide resolved
packages/@aws-containers/aws-ecs-builder/lib/extensions/appmesh.ts
Outdated
Show resolved
Hide resolved
packages/@aws-containers/aws-ecs-builder/lib/extensions/appmesh.ts
Outdated
Show resolved
Hide resolved
packages/@aws-containers/aws-ecs-builder/lib/extensions/appmesh.ts
Outdated
Show resolved
Hide resolved
mesh: this.mesh, | ||
virtualNodeName: this.parentService.id, | ||
cloudMapService: service.cloudMapService, | ||
listener: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an assumption that every service has a listener? For example, services which read from message queues often don't.
packages/@aws-containers/aws-ecs-builder/lib/extensions/appmesh.ts
Outdated
Show resolved
Hide resolved
packages/@aws-containers/aws-ecs-builder/lib/extensions/appmesh.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly copy-edit nits, and some naming discrepencies. Did not get through all of it but wanted to publish this before we talked!
packages/@aws-containers/aws-ecs-builder/lib/extensions/extension-interfaces.ts
Outdated
Show resolved
Hide resolved
packages/@aws-containers/aws-ecs-builder/lib/extensions/extension-interfaces.ts
Outdated
Show resolved
Hide resolved
packages/@aws-containers/aws-ecs-builder/lib/extensions/extension-interfaces.ts
Outdated
Show resolved
Hide resolved
packages/@aws-containers/aws-ecs-builder/lib/extensions/extension-interfaces.ts
Outdated
Show resolved
Hide resolved
packages/@aws-containers/aws-ecs-builder/lib/extensions/extension-interfaces.ts
Outdated
Show resolved
Hide resolved
I tried to proactively put in suggestions for things like punctuation at the end of docstrings: see #9069 (comment) |
Co-authored-by: Hsing-Hui Hsu <hsinghui@amazon.com>
Co-authored-by: Hsing-Hui Hsu <hsinghui@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments.
|
||
// A map of AWS account ID's which hold the App Mesh image in various | ||
// regions | ||
enum APPMESH_ECR_ACCOUNTS { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this info belongs in the region-info
package. At least in the appmesh
package - I don't think this is the greatest place for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have some work on the underlying L2 appmesh package scheduled. I'll discuss with the team when that second pass begins. I agree this is something we should probably implement as part of that pass to catch the L2's up to the current CFN features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, region-info
package can own this configuration and we can tackle it as part of L2 work (either myself or @dfezzie). We're considering an SSM parameter for this longer term (aws/aws-app-mesh-roadmap#257).
packages/@aws-containers/aws-ecs-builder/lib/extensions/appmesh.ts
Outdated
Show resolved
Hide resolved
packages/@aws-containers/aws-ecs-builder/lib/extensions/appmesh.ts
Outdated
Show resolved
Hide resolved
packages/@aws-containers/aws-ecs-builder/lib/extensions/cloudwatch-agent.ts
Outdated
Show resolved
Hide resolved
packages/@aws-containers/aws-ecs-builder/lib/extensions/http-load-balancer.ts
Outdated
Show resolved
Hide resolved
Also adding a CPU scaling extension, for testing purposes.
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
This PR implements RFC 219
It adds a new module called "@aws-containers/aws-ecs-builder" (Name proposed but not final) with following new constructs:
Environment
- A deploy environment for a service which by default supplies its own VPC, and ECS cluster with Fargate capacityService
- An ECS serviceAnd supporting classes:
ServiceDescription
- Defines the application to run as a service, and any features it needsServiceExtension
- Defines an optional extension that may be added to a service to enhance it with new capabilities or connect it to other ECS adjacent featuresThis PR comes with a
ServiceExtension
for each of the following ECS adjacent features:Developers can use this new extendable
Service
class to build aServiceDescription
that defines their application and as many optional service extensions as they want. TheServiceDescription
is used to build out aService
inside anEnvironment
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license