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.

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

fix(ecs): update minHealthyPercent validation for ec2service #7814

Merged
merged 1 commit into from
May 5, 2020

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 SoManyHs, piradeepk and a team 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.

packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts Outdated Show resolved Hide resolved
@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

packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts Outdated Show resolved Hide resolved
@@ -351,7 +351,7 @@ export = {
test.done();
},

'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