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.

Sign up for GitHub

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): set default HealthCheckGracePeriodSeconds #2942

Merged
merged 6 commits into from
Jun 21, 2019
Merged

feat(ecs): set default HealthCheckGracePeriodSeconds #2942

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3ca7720
feat(ecs): set the default `HealthCheckGracePeriodSeconds` in base-se…
hencrice Jun 19, 2019
633b527
Fixed formatting issue.
hencrice Jun 20, 2019
596e9cb
Merge branch 'master' into baseservice-fixme
hencrice Jun 20, 2019
9072363
Moved the logic that determines the value of `healthCheckGracePeriodS…
hencrice Jun 20, 2019
73e37d0
Merge remote-tracking branch 'origin/master' into pr/hencrice/2942
rix0rrr Jun 21, 2019
d0d1ec0
Simplify evaluator, update expectations
rix0rrr Jun 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions packages/@aws-cdk/aws-ecs/lib/base/base-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export interface BaseServiceProps {
/**
* Time after startup to ignore unhealthy load balancer checks.
*
* @default ??? FIXME
* @default - defaults to 60 seconds if not set and at least one load balancer is in-use
hencrice marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly healthCheckGracePeriodSeconds?: number;

Expand Down Expand Up @@ -151,7 +151,12 @@ export abstract class BaseService extends Resource
maximumPercent: props.maximumPercent || 200,
minimumHealthyPercent: props.minimumHealthyPercent === undefined ? 50 : props.minimumHealthyPercent
},
healthCheckGracePeriodSeconds: props.healthCheckGracePeriodSeconds,
// only set the grace period when load balancers are configured and
// healthCheckGracePeriodSeconds is not already set
healthCheckGracePeriodSeconds: Lazy.anyValue({ produce: () =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this to a helper private method for readability?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a Lazy.numberValue(), no?

this.loadBalancers
Copy link
Contributor

Choose a reason for hiding this comment

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

this.loadBalancers should never be undefined. Should we not make the default an empty array? Gets rid of the test here.

&& this.loadBalancers.length
&& props.healthCheckGracePeriodSeconds === undefined ? 60 : props.healthCheckGracePeriodSeconds}),
/* role: never specified, supplanted by Service Linked Role */
networkConfiguration: Lazy.anyValue({ produce: () => this.networkConfiguration }),
serviceRegistries: Lazy.anyValue({ produce: () => this.serviceRegistries }),
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-awsvpc-nw.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -844,6 +844,7 @@
"MinimumHealthyPercent": 50
},
"DesiredCount": 1,
"HealthCheckGracePeriodSeconds": 60,
"LaunchType": "EC2",
"LoadBalancers": [
{
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-bridge-nw.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,6 +866,7 @@
"MinimumHealthyPercent": 50
},
"DesiredCount": 1,
"HealthCheckGracePeriodSeconds": 60,
"LaunchType": "EC2",
"LoadBalancers": [
{
Expand Down
11 changes: 9 additions & 2 deletions packages/@aws-cdk/aws-ecs/test/ec2/test.ec2-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export = {
test.done();
},


hencrice marked this conversation as resolved.
Show resolved Hide resolved
"errors if daemon and desiredCount both specified"(test: Test) {
// GIVEN
const stack = new cdk.Stack();
Expand Down Expand Up @@ -600,11 +601,17 @@ export = {
ContainerPort: 808,
LoadBalancerName: { Ref: "LB8A12904C" }
}
],
]
}));

expect(stack).to(haveResource('AWS::ECS::Service', {
// if any load balancer is configured and healthCheckGracePeriodSeconds is not
// set, then it should default to 60 seconds.
HealthCheckGracePeriodSeconds: 60
}));

test.done();
},
}
},

'When enabling service discovery': {
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-ecs/test/fargate/integ.lb-awsvpc-nw.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@
"MinimumHealthyPercent": 50
},
"DesiredCount": 1,
"HealthCheckGracePeriodSeconds": 60,
"LaunchType": "FARGATE",
"LoadBalancers": [
{
Expand Down
6 changes: 6 additions & 0 deletions packages/@aws-cdk/aws-ecs/test/fargate/test.fargate-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,12 @@ export = {
}
}));

expect(stack).to(haveResource('AWS::ECS::Service', {
// if any load balancer is configured and healthCheckGracePeriodSeconds is not
hencrice marked this conversation as resolved.
Show resolved Hide resolved
// set, then it should default to 60 seconds.
HealthCheckGracePeriodSeconds: 60
}));

test.done();
},

Expand Down