feat(apprunner): add HealthCheckConfiguration property in Service #27029

go-to-k · 2023-09-06T08:39:51Z

This PR adds HealthCheckConfiguration property in Service construct.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

lpizzinidev · 2023-09-06T15:41:19Z

packages/@aws-cdk/aws-apprunner-alpha/lib/service.ts

@@ -1074,6 +1147,10 @@ export class Service extends cdk.Resource implements iam.IGrantable {
      throw new Error('configurationValues cannot be provided if the ConfigurationSource is Repository');
    }

+    if (this.props.healthCheckConfiguration?.path && this.props.healthCheckConfiguration?.protocol !== HealthCheckProtocolType.HTTP) {


Can you please also add validation for HealthyThreshold, Interval, Timeout, and UnhealthyThreshold min/max values according to this spec?

@lpizzinidev

Thanks for your review! I missed!

I changed them, please see again.

2a11dc4

lpizzinidev

Thanks for the contribution!
Looks good overall, just a small addition on parameter validation.

lpizzinidev · 2023-09-07T06:45:25Z

packages/@aws-cdk/aws-apprunner-alpha/lib/service.ts

@@ -1074,6 +1147,35 @@ export class Service extends cdk.Resource implements iam.IGrantable {
      throw new Error('configurationValues cannot be provided if the ConfigurationSource is Repository');
    }

+    if (this.props.healthCheckConfiguration?.path !== undefined) {


The implementation is correct.
We should create a separate function to handle the logic to keep the constructor cleaner, something like:

private validateHealthCheckConfiguration(healthCheckConfiguration?: HealthCheckConfiguration) { // Add guard to avoid using ? on every successive control if (!healthCheckConfiguration) return; // Validate parameters... }

lpizzinidev · 2023-09-07T06:48:44Z

packages/@aws-cdk/aws-apprunner-alpha/lib/service.ts

+    }
+    if (this.props.healthCheckConfiguration?.healthyThreshold !== undefined) {
+      if (this.props.healthCheckConfiguration?.healthyThreshold < 1 || this.props.healthCheckConfiguration?.healthyThreshold > 20) {
+        throw new Error('healthyThreshold must be greater than or equal to 1 and less than or equal to 20');


We should add the received value in the error message as a feedback to the user and make the message more concise:

throw new Error(`healthyThreshold must be between 1 and 20, got ${healthCheckConfiguration.healthyThreshold}`);

lpizzinidev · 2023-09-07T06:57:06Z

packages/@aws-cdk/aws-apprunner-alpha/test/service.test.ts

+  }).toThrow('path length must be greater than 0');
+});
+
+test('OK if healthyThreshold is 1 in healthCheckConfiguration', () => {


I think that we should only leave the unit tests for the failing cases of validation, we are already checking that the implementation is working in the Service has healthCheckConfiguration test.
Adding test cases for checking limit values is a bit redundant in my opinion.

lpizzinidev

Thanks for the fast implementation 💪
Looks good overall!
Just a couple of changes on code cleanup and message formatting.

go-to-k · 2023-09-07T07:13:50Z

Thanks, I changed!

9e84888

lpizzinidev

Thanks!
One last change on the formatting of the durations in the validation message

lpizzinidev · 2023-09-07T07:24:40Z

packages/@aws-cdk/aws-apprunner-alpha/lib/service.ts

+    }
+    if (healthCheckConfiguration.interval !== undefined) {
+      if (healthCheckConfiguration.interval.toSeconds() < 1 || healthCheckConfiguration.interval.toSeconds() > 20) {
+        throw new Error(`interval must be between 1 and 20 seconds, got ${healthCheckConfiguration.interval}`);


interval and timeout should be converted with toSeconds()

OK, I changed!

629b131

mrgrain · 2023-09-21T10:25:11Z

packages/@aws-cdk/aws-apprunner-alpha/README.md

+new apprunner.Service(this, 'Service', {
+  source: apprunner.Source.fromEcrPublic({
+    imageConfiguration: { port: 8000 },
+    imageIdentifier: 'public.ecr.aws/aws-containers/hello-app-runner:latest',
+  }),
+  healthCheckConfiguration: {
+    healthyThreshold: 5,
+    interval: Duration.seconds(10),
+    path: '/',
+    protocol: apprunner.HealthCheckProtocolType.HTTP,
+    timeout: Duration.seconds(10),
+    unhealthyThreshold: 10,
+  },
+});


I'd like us to use an enum like class for this, because HTTP and TCP health checks have different props:

Suggested change

new apprunner.Service(this, 'Service', {

source: apprunner.Source.fromEcrPublic({

imageConfiguration: { port: 8000 },

imageIdentifier: 'public.ecr.aws/aws-containers/hello-app-runner:latest',

}),

healthCheckConfiguration: {

healthyThreshold: 5,

interval: Duration.seconds(10),

path: '/',

protocol: apprunner.HealthCheckProtocolType.HTTP,

timeout: Duration.seconds(10),

unhealthyThreshold: 10,

},

});

new apprunner.Service(this, 'Service', {

source: apprunner.Source.fromEcrPublic({

imageConfiguration: { port: 8000 },

imageIdentifier: 'public.ecr.aws/aws-containers/hello-app-runner:latest',

}),

healthCheck: apprunner.HealthCheck.http({

healthyThreshold: 5,

interval: Duration.seconds(10),

path: '/',

timeout: Duration.seconds(10),

unhealthyThreshold: 10,

}),

});

Have a look at appmesh.HealthCheck

@mrgrain

I changed to use the HealthCheck class like appmesh.HealthCheck.

d0cd255

70c125e

Pull request has been modified.

changed comments use bind

mergify · 2023-09-27T12:50:50Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2023-09-27T12:56:00Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-09-27T14:28:33Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 5938fd1
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-09-27T14:29:03Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

feat(apprunner): add HealthCheckConfiguration property

b4889f8

github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Sep 6, 2023

aws-cdk-automation requested a review from a team September 6, 2023 08:40

github-actions bot added the p2 label Sep 6, 2023

aws-cdk-automation previously requested changes Sep 6, 2023

View reviewed changes

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. labels Sep 6, 2023

go-to-k changed the title ~~feat(apprunner): add HealthCheckConfiguration property~~ feat(apprunner): add HealthCheckConfiguration property in Service Sep 6, 2023

go-to-k added 7 commits September 6, 2023 18:29

using duration

a30062d

HealthCheckProtocolType

779a580

add to props

f346240

add validation for path

1415c34

add unit tests

9f63382

change README

deb0418

add integ tests

7345fe9

change docs

877d5f2

go-to-k marked this pull request as ready for review September 6, 2023 10:57

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 6, 2023

lpizzinidev reviewed Sep 6, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 6, 2023

lpizzinidev suggested changes Sep 6, 2023

View reviewed changes

add validations and tests

2a11dc4

lpizzinidev reviewed Sep 7, 2023

View reviewed changes

lpizzinidev suggested changes Sep 7, 2023

View reviewed changes

change for review

9e84888

lpizzinidev suggested changes Sep 7, 2023

View reviewed changes

lpizzinidev reviewed Sep 7, 2023

View reviewed changes

change for re-review

629b131

lpizzinidev approved these changes Sep 7, 2023

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 7, 2023

mrgrain previously requested changes Sep 21, 2023

View reviewed changes

mrgrain removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 21, 2023

go-to-k force-pushed the feat/apprunner-custom-health-endpoint branch from cfcb72f to 6664245 Compare September 23, 2023 14:02

add HealthCheck class

d0cd255

changed comments use bind

go-to-k force-pushed the feat/apprunner-custom-health-endpoint branch from 6664245 to d0cd255 Compare September 23, 2023 14:09

change docs for path

70c125e

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 23, 2023

rix0rrr approved these changes Sep 27, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 27, 2023

Merge branch 'main' into feat/apprunner-custom-health-endpoint

5938fd1

mergify bot merged commit 4e8c9c4 into aws:main Sep 27, 2023
8 checks passed

github-actions bot mentioned this pull request Oct 1, 2023

Monthly PRs metrics report - Sep 2023 #27368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(apprunner): add HealthCheckConfiguration property in Service #27029

feat(apprunner): add HealthCheckConfiguration property in Service #27029

go-to-k commented Sep 6, 2023 •

edited

Loading

aws-cdk-automation left a comment

lpizzinidev Sep 6, 2023

go-to-k Sep 7, 2023 •

edited

Loading

lpizzinidev left a comment

lpizzinidev Sep 7, 2023 •

edited

Loading

lpizzinidev Sep 7, 2023

lpizzinidev Sep 7, 2023

lpizzinidev left a comment

go-to-k commented Sep 7, 2023

lpizzinidev left a comment

lpizzinidev Sep 7, 2023

go-to-k Sep 7, 2023

mrgrain Sep 21, 2023

go-to-k Sep 23, 2023 •

edited

Loading

mergify bot commented Sep 27, 2023

mergify bot commented Sep 27, 2023

aws-cdk-automation commented Sep 27, 2023

mergify bot commented Sep 27, 2023

feat(apprunner): add HealthCheckConfiguration property in Service #27029

feat(apprunner): add HealthCheckConfiguration property in Service #27029

Conversation

go-to-k commented Sep 6, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

lpizzinidev Sep 6, 2023

Choose a reason for hiding this comment

go-to-k Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

lpizzinidev Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

lpizzinidev Sep 7, 2023

Choose a reason for hiding this comment

lpizzinidev Sep 7, 2023

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

go-to-k commented Sep 7, 2023

lpizzinidev left a comment

Choose a reason for hiding this comment

lpizzinidev Sep 7, 2023

Choose a reason for hiding this comment

go-to-k Sep 7, 2023

Choose a reason for hiding this comment

mrgrain Sep 21, 2023

Choose a reason for hiding this comment

go-to-k Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

mergify bot commented Sep 27, 2023

mergify bot commented Sep 27, 2023

aws-cdk-automation commented Sep 27, 2023

AWS CodeBuild CI Report

mergify bot commented Sep 27, 2023

go-to-k commented Sep 6, 2023 •

edited

Loading

go-to-k Sep 7, 2023 •

edited

Loading

lpizzinidev Sep 7, 2023 •

edited

Loading

go-to-k Sep 23, 2023 •

edited

Loading