-
Notifications
You must be signed in to change notification settings - Fork 4k
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(ecs): make cluster and vpc optional for higher level constructs #2773
Changes from 23 commits
7b6dece
10708f4
f7896ee
da87dd9
deee9cc
bbeb602
b61209d
8d73f37
c6fdd60
66ee656
a213221
02f9dd9
ea2b992
0f538c5
493fdec
23282d0
d11fd6c
50510ec
4c6855b
d60c06c
d0bd65a
e5ef6de
a892832
ce23026
0498958
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,48 @@ export = { | |
test.done(); | ||
}, | ||
|
||
'set vpc instead of cluster'(test: Test) { | ||
// GIVEN | ||
const stack = new cdk.Stack(); | ||
const vpc = new ec2.Vpc(stack, 'VPC'); | ||
|
||
// WHEN | ||
new ecsPatterns.LoadBalancedEc2Service(stack, 'Service', { | ||
vpc, | ||
memoryLimitMiB: 1024, | ||
image: ecs.ContainerImage.fromRegistry('test'), | ||
desiredCount: 2, | ||
environment: { | ||
TEST_ENVIRONMENT_VARIABLE1: "test environment variable 1 value", | ||
TEST_ENVIRONMENT_VARIABLE2: "test environment variable 2 value" | ||
} | ||
}); | ||
|
||
// THEN - stack does not contain a LaunchConfiguration | ||
expect(stack, true).notTo(haveResource("AWS::AutoScaling::LaunchConfiguration")); | ||
|
||
test.throws(() => expect(stack)); | ||
|
||
test.done(); | ||
}, | ||
|
||
'setting vpc and cluster throws error'(test: Test) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also add a test here for providing a vpc directly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In this case, you want cluster to be required and ensure that capacity is added, fail if vpc is provided without capacity. |
||
// GIVEN | ||
const stack = new cdk.Stack(); | ||
const vpc = new ec2.Vpc(stack, 'VPC'); | ||
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc }); | ||
|
||
// WHEN | ||
test.throws(() => new ecsPatterns.LoadBalancedEc2Service(stack, 'Service', { | ||
cluster, | ||
vpc, | ||
loadBalancerType: ecsPatterns.LoadBalancerType.NETWORK, | ||
image: ecs.ContainerImage.fromRegistry("/aws/aws-example-app") | ||
})); | ||
|
||
test.done(); | ||
}, | ||
|
||
'test ECS loadbalanced construct with memoryReservationMiB'(test: Test) { | ||
// GIVEN | ||
const stack = new cdk.Stack(); | ||
|
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.
Please move this logic to
LoadBalancedFargateService
as discussed.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 am not sure how this should work, as the constructor needs the cluster and I cannot do stuff before calling super...
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.
you are right. OK, let's leave this here, but make sure that for EC2 cluster is required.
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.
And how? The base class does not know about EC2 or Fargate, does it?
Why shouldn't it be possible to do:
I am open for any hint.
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.
The base class is abstract, isn't 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.
This is the same when you provide a cluster but do not add capacity to 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.
@hoegertn is correct. We have other similar cases for “late validation”, like codepipeline (you can create a pipeline with no stages and then add them, but at least one is required). This essentially the only reason we have the “validate” hook.
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.
@eladb the intent behind the constructs in ecs-patterns is to provide an opinionated approach to creating ecs services following what is considered best practices. I definitely agree that this works, but by relying on late validation, and not requiring capacity to be an input prop, we're not providing an opinion (allowing the user to decide if they want to add capacity when we know that they need to provide capacity), we're straying away from the intentions of the ecs-patterns module. @SoManyHs thoughts?
I would suggest that AddCapacityOptions (if cluster is not provided) or cluster with capacity already added to it should be required.
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.
While I agree with this opinion I do not see how this PR changes the current behavior as it currently possible to add clusters without capacity. So I think we could have this PR as is and create an extra issue about late vs. instant validation of capacity whether providing a cluster or not. Any thoughts?
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'm okay with this, can you create an issue and add it?