Skip to content

Conversation

@ryyakobe
Copy link
Contributor

@ryyakobe ryyakobe commented Jan 7, 2021

I deployed the all in AWS infrastructure basic app to confirm that everything is working.

Besides the CDK version bump, this includes:

  • A change in usage-based-licensing.ts caused by the change in CDK fix(aws-ecs): update desired count to be optional aws-cdk#12223 . Our UsageBasedLicensing construct uses desiredCount not only for Ec2Service, but also for ASG. As we want these values to be the same, we won't be removing the DesiredCount similar to CDK, but will keep the same behavior as before, defaulting to 1. We do this, because if we don't set DesiredCount explicitly for the Service, it will use the current task number, but we won't be able to get this number for ASG from the service.

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

@ryyakobe ryyakobe requested review from ddneilson and jusiskin January 7, 2021 22:42
cluster: this.cluster,
taskDefinition,
desiredCount: props.desiredCount,
desiredCount: props.desiredCount ?? 1,
Copy link
Contributor

@ddneilson ddneilson Jan 8, 2021

Choose a reason for hiding this comment

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

Remind me... what was the CDK change that required this change?

edit: n/m, I found it -- aws/aws-cdk#12223

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The link and short description is in the description of this PR. There were 2 options available:

  1. In RFDK we just default to size 1 if desiredCount is not set explicitly. (what I did in this PR)
  2. Do not set any default in RFDK for Ec2Service. In this case CDK will not add DesiredCount property to the CloudFormation template. But then it's not clear what to use for minCapacity and maxCapacity in ASG here https://github.com/aws/aws-rfdk/blob/mainline/packages/aws-rfdk/lib/deadline/lib/usage-based-licensing.ts#L508, that's why I decided to go with a simpler option 1.

@ddneilson ddneilson merged commit 05941d1 into aws:mainline Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants