feat(ecs): allow ECS to be used declaratively #1745
Conversation
Minor changes to the ECS APIs so that they can be instantiated via a declarative template (see deCDK #1618). Merge `IContainerImage` and the base `ContainerImage` into an abstract class so it's now an "enum-like" class with static methods. It also improves discoverability for all other users and more aligned with how other constructs expose union types (e.g. `lambda.Code`). Normalize the ctor of `ContainerDefinition` to "scope, id, props" so that it can be instantiated as a deCDK resource.
| this.memoryLimitSpecified = props.memoryLimitMiB !== undefined || props.memoryReservationMiB !== undefined; | ||
|
|
||
| props.image.bind(this); | ||
| if (props.logging) { props.logging.bind(this); } | ||
| props.task._linkContainer(this); |
There was a problem hiding this comment.
Couldn't you devise syntax to call the add()er in deCDK?
|
|
||
| /** | ||
| * Constructs for types of container images | ||
| */ | ||
| export class ContainerImage { | ||
| export abstract class ContainerImage { |
There was a problem hiding this comment.
I'm not sure I agree with this change, nor that it is strictly necessary for deCDK. Surely you could implement some heuristics around this.
There was a problem hiding this comment.
I have an idea how we can do this (convert "add"s to prop arrays) but that's not implemented. Also, this approach doesn't rely on the fact that users call add to associate a container to the task definition, they can either use add or "new" the ContainerDefinition themselves, and it will "register" itself on the task definition.
There was a problem hiding this comment.
Regardless of deCDK, I think this is a better API because it's easier to discover the various options from the type of the options. It is also more aligned with other APIs like lambda.Code and lambda.Runtime. Any reason to insist on using an interface instead of an abstract class?
There was a problem hiding this comment.
Interfaces give us more flexibility to make implementation choices, classes lock us into a single-inheritance model that at some point is not going to fit whatever we want to do.
I know this is wishy-washy, I don't have a hard example to convince you right now. So fine, I guess.
I also question the necessity of making this change right this instant. When are we productizing deCDK?
There was a problem hiding this comment.
The reason I need this is in order for the deCDKs prototype which I would like to release to include an ECS example.
I think standardizing on how we implement these “unions” pattern will be useful because it’s reoccurring. I understand the benefit of using interfaces and maybe that should be the pattern but I also find the discoverability when using these a bit problematic. Using a class with a bunch of static methods/properties is closer to enums in discoverability so that’s where the value is in my mind.
Perhaps we can enforce via awslint that the jsdoc will include some information about possible implementations?
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
Minor changes to the ECS APIs so that they can be instantiated via a declarative
template (see deCDK #1618).
Merge
IContainerImageand the baseContainerImageinto an abstract classso it's now an "enum-like" class with static methods. It also improves discoverability
for all other users and more aligned with how other constructs expose union types
(e.g.
lambda.Code).Normalize the ctor of
ContainerDefinitionto "scope, id, props" so that it canbe instantiated as a deCDK resource.
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.