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

(aws-ecs-patterns): QueueProcessingServiceBase does not respect a minScalingCapacity: 0 #14336

Closed
wittekm opened this issue Apr 22, 2021 · 2 comments · Fixed by #16961
Closed
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@wittekm
Copy link

wittekm commented Apr 22, 2021

In the following code:

I am in a situation where:

  • I have turned on this.node.setContext(cxapi.ECS_REMOVE_DEFAULT_DESIRED_COUNT, true); (putting me in the else-else clause on line 312)
  • I am creating a new ecs_patterns.QueueProcessingFargateService
  • I'm feeding in a props.minScalingCapacity of 0 - which appears to be a completely valid number when using the ECS UI - but that is being overridden by the || 1

Reproduction Steps

Described reasonably well above. Create a new ecs_patterns.QueueProcessingFargateService with minScalingCapacity: 0 and inspect the generated Cfn template; it'll show a non-zero for MinCapacity. In my case:

    "apr23sysmongeneratorserviceQueueProcessingFargateServiceTaskCountTargetAE4E3513": {
      "Type": "AWS::ApplicationAutoScaling::ScalableTarget",
      "Properties": {
        "MaxCapacity": 5,
        "MinCapacity": 1,
        "ResourceId": {

What did you expect to happen?

Create a Cfn AWS::ApplicationAutoScaling::ScalableTarget with MinCapacity: 0

What actually happened?

Created a Cfn AWS::ApplicationAutoScaling::ScalableTarget with MinCapacity: 1

Environment

  • Framework Version: 1.97.0

Other

Suggested solution:
Consider using = props.minScalingCapacity ?? 1 - this is the new Typescript Nullish Coalescing Operator, which slightly differs from || in that it will accept values like 0 and false, instead of falling through to the backup.

In general, you'd probably want to do a serious audit of all uses of x = x || y in cases where, say, 0 is a valid prop.


This is 🐛 Bug Report

@wimax-grapl
Copy link

wimax-grapl commented Apr 26, 2021

As a workaround, it looks like I can piggyback on the || props.desiredTaskCount on 309, and set that to zero as well.

@SoManyHs SoManyHs added effort/small Small work item – less than a day of effort p2 and removed needs-triage This issue or PR still needs to be triaged. labels May 5, 2021
@mergify mergify bot closed this as completed in #16961 Oct 13, 2021
mergify bot pushed a commit that referenced this issue Oct 13, 2021
fixes: #15632
fixes: #14336

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
fixes: aws#15632
fixes: aws#14336

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
6 participants