Skip to content

Conversation

@iamhopaul123
Copy link
Contributor

@iamhopaul123 iamhopaul123 commented May 5, 2020

Removes unnecessary validation for "minHealthyPercent must be 0 when daemon strategy is set". Additionally, add validation for "minHealthyPercent must less than maxHealthyPercent".

Previously, the Ec2Service will throw an exception when the daemon strategy is used and a non-zero minHealthyPercent is defined. Users will now be allowed to define a minHealthyPercent of greater than zero.

Fixes #7396


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

@iamhopaul123 iamhopaul123 requested review from a team, SoManyHs and piradeepk May 5, 2020 22:22
Copy link
Contributor

@piradeepk piradeepk left a comment

Choose a reason for hiding this comment

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

Thanks @iamhopaul123! This looks great overall, but had a couple minor comments.

@mergify mergify bot dismissed piradeepk’s stale review May 5, 2020 22:43

Pull request has been modified.

@SomayaB SomayaB added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label May 5, 2020
Copy link
Contributor

@piradeepk piradeepk left a comment

Choose a reason for hiding this comment

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

Ship it! 💯 Looks great! Thanks @iamhopaul123!

@piradeepk piradeepk added the pr/do-not-merge This PR should not be merged at this time. label May 5, 2020
@piradeepk
Copy link
Contributor

@iamhopaul123 can you update the description and describe hte previous behaviour as well as the new behaviour.

@iamhopaul123 iamhopaul123 removed the pr/do-not-merge This PR should not be merged at this time. label May 5, 2020
@piradeepk piradeepk added pr/do-not-merge This PR should not be merged at this time. and removed pr/do-not-merge This PR should not be merged at this time. labels May 5, 2020
@piradeepk piradeepk changed the title fix(ecs): update minHealthyPercent constrain for ec2service with daemon fix(ecs): update minHealthyPercent validation for ec2service May 5, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 5b05962
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

},

'errors if daemon and minimum not 0'(test: Test) {
'errors if minimum not less than maximum'(test: Test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "greater than"?

@iamhopaul123 iamhopaul123 added pr/do-not-merge This PR should not be merged at this time. and removed pr/do-not-merge This PR should not be merged at this time. labels May 5, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 44124b3
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented May 5, 2020

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

@mergify mergify bot merged commit 19e3fd8 into aws:master May 5, 2020
karupanerura pushed a commit to karupanerura/aws-cdk that referenced this pull request May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

@aws-cdk/aws-ecs Related to Amazon Elastic Container

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update minHealthyPercent for Ec2Service using Daemon strategy.

6 participants