fix(ecs-patterns): `minHealthyPercent` and `maxHealthyPercent` props validation by lpizzinidev · Pull Request #26193 · aws/aws-cdk

lpizzinidev · 2023-07-01T13:33:12Z

Setting maxHealthyPercent to a non-integer value was not raising synth-time errors, but was generating invalid CFN templates.
This fix adds validation for both maxHealthyPercent and minHealthyPercent.

Closes #26158.

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

…dation

gitpod-io · 2023-07-01T13:33:18Z

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.

otaviomacedo · 2023-07-01T13:46:08Z

packages/aws-cdk-lib/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts

    }

+    this.validateHealthyPercentage('minHealthyPercent', props.minHealthyPercent);
+    this.validateHealthyPercentage('maxHealthyPercent', props.maxHealthyPercent);


While we're here, please also validate that min < max.

lpizzinidev · 2023-07-01T14:47:45Z

Exemption request.
Let me know if an integration test is needed.

aws-cdk-automation · 2023-07-23T00:09:20Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

lpizzinidev · 2023-07-23T09:02:03Z

Exemption request.
This fix implements validation, it shouldn't require an integration test.

lpizzinidev · 2023-07-24T12:54:52Z

@otaviomacedo
This should be ready to merge

mrgrain

~~Still a couple of issues. Assuming I understand correctly how this works and min should indeed by lower than max.~~
My bad.

mrgrain · 2023-07-24T13:28:15Z

packages/aws-cdk-lib/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts

+    this.validateHealthyPercentage('minHealthyPercent', props.minHealthyPercent);
+    this.validateHealthyPercentage('maxHealthyPercent', props.maxHealthyPercent);
+
+    if (props.minHealthyPercent && props.maxHealthyPercent && props.minHealthyPercent >= props.maxHealthyPercent) {


Suggested change

if (props.minHealthyPercent && props.maxHealthyPercent && props.minHealthyPercent >= props.maxHealthyPercent) {

if (props.minHealthyPercent && !Token.isUnresolved(props.minHealthyPercent) && props.maxHealthyPercent && !Token.isUnresolved(props.maxHealthyPercent) && props.minHealthyPercent < props.maxHealthyPercent) {

Yes, in fact, I throw an exception if min >= max.
My code should be correct (unless I'm missing something).
I can add the token resolution check if you think that's necessary.

Ah yes of course 🤦🏻 Just need to add the checks for unresolved tokens than please!

Suggested change

if (props.minHealthyPercent && props.maxHealthyPercent && props.minHealthyPercent >= props.maxHealthyPercent) {

if (props.minHealthyPercent && !Token.isUnresolved(props.minHealthyPercent) && props.maxHealthyPercent && !Token.isUnresolved(props.maxHealthyPercent) props.minHealthyPercent >= props.maxHealthyPercent) {

packages/aws-cdk-lib/aws-ecs-patterns/test/fargate/load-balanced-fargate-service.test.ts

mrgrain · 2023-07-24T13:29:52Z

packages/aws-cdk-lib/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts

+    if (value === undefined) { return; }
+    if (!Number.isInteger(value) || value < 0) {


Suggested change

if (value === undefined) { return; }

if (!Number.isInteger(value) || value < 0) {

if (value === undefined || Token.isUnresolved(value)) {

return;

}

mrgrain · 2023-07-24T16:55:07Z

@lpizzinidev Sorry for that! We still need the checks for unresolved Tokens. Otherwise this is good to go.

aws-cdk-automation · 2023-07-25T07:31:21Z

AWS CodeBuild CI Report

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

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

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

mergify · 2023-07-25T10:38:28Z

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).

…validation (aws#26193) Setting `maxHealthyPercent` to a non-integer value was not raising synth-time errors, but was generating invalid CFN templates. This fix adds validation for both `maxHealthyPercent` and `minHealthyPercent`. Closes aws#26158. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

fix(ecs-patterns): minHealthyPercent and maxHealthyPercent props vali…

5aaa11f

…dation

github-actions bot added admired-contributor [Pilot] contributed between 13-24 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Jul 1, 2023

aws-cdk-automation requested a review from a team July 1, 2023 13:33

aws-cdk-automation previously requested changes Jul 1, 2023

View reviewed changes

otaviomacedo reviewed Jul 1, 2023

View reviewed changes

lpizzinidev added 2 commits July 1, 2023 16:02

Merge branch 'main' into awsgh-26158

0f801e8

added min < max validation

46b8faf

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Jul 1, 2023

mrgrain requested changes Jul 24, 2023

View reviewed changes

lpizzinidev added 2 commits July 25, 2023 08:53

Merge branch 'main' into awsgh-26158

bf947cb

added token resolution checks

d9090d5

lpizzinidev force-pushed the gh-26158 branch from cdc40d1 to d9090d5 Compare July 25, 2023 07:05

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

mrgrain added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jul 25, 2023

mrgrain approved these changes Jul 25, 2023

View reviewed changes

mergify bot merged commit bdfdc91 into aws:main Jul 25, 2023

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

Monthly PRs metrics report - July 2023 #26583

Closed

	if (props.minHealthyPercent && props.maxHealthyPercent && props.minHealthyPercent >= props.maxHealthyPercent) {
	if (props.minHealthyPercent && !Token.isUnresolved(props.minHealthyPercent) && props.maxHealthyPercent && !Token.isUnresolved(props.maxHealthyPercent) && props.minHealthyPercent < props.maxHealthyPercent) {

		if (value === undefined) { return; }
		if (!Number.isInteger(value) \|\| value < 0) {

Conversation

lpizzinidev commented Jul 1, 2023

Uh oh!

gitpod-io bot commented Jul 1, 2023

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

otaviomacedo Jul 1, 2023

Choose a reason for hiding this comment

Uh oh!

lpizzinidev commented Jul 1, 2023

Uh oh!

aws-cdk-automation commented Jul 23, 2023

Uh oh!

lpizzinidev commented Jul 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpizzinidev commented Jul 24, 2023

Uh oh!

mrgrain left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrgrain Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

lpizzinidev Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

mrgrain Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrgrain Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

mrgrain commented Jul 24, 2023

Uh oh!

aws-cdk-automation commented Jul 25, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lpizzinidev commented Jul 23, 2023 •

edited

Loading

mrgrain left a comment •

edited

Loading

mrgrain Jul 24, 2023 •

edited

Loading