fix(appmesh): routes with weight 0 are assigned a weight of 1#21400
fix(appmesh): routes with weight 0 are assigned a weight of 1#21400mergify[bot] merged 2 commits intoaws:mainfrom
Conversation
|
|
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests).
Additionally, please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process
| renderedTargets.push({ | ||
| virtualNode: t.virtualNode.virtualNodeName, | ||
| weight: t.weight || 1, | ||
| weight: t.weight == null ? 1 : t.weight, |
There was a problem hiding this comment.
You probably want to compare against undefined here instead of null.
Pull request has been modified.
|
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 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). |
| renderedTargets.push({ | ||
| virtualNode: t.virtualNode.virtualNodeName, | ||
| weight: t.weight || 1, | ||
| weight: t.weight == undefined ? 1 : t.weight, |
There was a problem hiding this comment.
I agree with @ytsssun This should be weight: t.weight || 0. Is there a reason we are defaulting to 1?
There was a problem hiding this comment.
0 would be a better default because The total weight for all targets combined must be less than or equal to 100. . But I wanted to keep this PR as small as possible without changing the established defaults.
It might be that changing this default could break something for some users. @TheRealAmazonKendra what do you think?
All Submissions:
fixes #21365
This PR fixes an unexpected behaivor when creating an appmesh.
Expected Behavior
Routes with targets of weight 0 should not change to 1 automatically.
Current Behavior (without fix)
Routes with targets of weight 0 are being changed to 1 automatically.
Solution
t.weight || 1will result in0 || 1if theweightis set to0. This will result in a route that can never have a weight of 0, since0is afaultyvalue.To fix this, I compare the weight with
undefined& return1only if the weight is undefined.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