fix(ecs): setting updatePolicy results in error due to updateType having default value #21025
fix(ecs): setting updatePolicy results in error due to updateType having default value #21025mergify[bot] merged 2 commits intoaws:mainfrom richarddotcodes:main
Conversation
|
|
rix0rrr
left a comment
There was a problem hiding this comment.
PRs with changes that are not statically guaranteed by the type system must have tests to make sure that (a) the new behavior works as intended and (b) it is not accidentally undone by future changes (See Contributing Guide, Work Your Magic).
| vpc: this.vpc, | ||
| machineImage, | ||
| updateType: options.updateType || autoscaling.UpdateType.REPLACING_UPDATE, | ||
| updateType: !!options.updatePolicy ? undefined : options.updateType || autoscaling.UpdateType.REPLACING_UPDATE, |
There was a problem hiding this comment.
This behavior change should also be reflected in the @default docstring of the updateType field.
There was a problem hiding this comment.
Is this really changing the default for the field, or am I just changing what gets passed in?
There was a problem hiding this comment.
`updatePolicy` takes precedence over `updateType`.
@default UpdateType.REPLACING_UPDATE, unless updatePolicy has been set
There was a problem hiding this comment.
Thanks. I've updated this PR to reflect the requested changes.
|
@rix0rrr Can I get a review? Thanks! |
|
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). |
|
@richarddotcodes In order for Mergify to merge in your change you'll need to allow changes to your branch. |
|
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). |
Pull request has been modified.
…ing default value Currently, providing an updatePolicy always results in an error since updateType is always set. This change fixes that.
|
@TheRealAmazonKendra Thanks for the info. I've followed the instructions in the link you'd sent. I also merged in changes from main; can I have another review? |
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
https://github.com/Mergifyio update |
☑️ Nothing to doDetails
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
Currently, providing an updatePolicy always results in an error
since updateType is always set. This change fixes that.
closes #11581
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license