feat(apprunner): add AutoScalingConfiguration for AppRunner Service

[mazyu36]

Issue # (if applicable)

Closes #30353 .

Reason for this change

At the moment, L2 Construct does not support a custom auto scaling configuration for the AppRunner Service.

Description of changes

• Add AutoScalingConfiguration Class
• Add autoScalingConfiguration property to the Service Class

Description of how you validated changes

Add unit tests and integ tests.

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[lpizzinidev]

Thanks 👍 Left some comments for minor adjustments

[lpizzinidev]

Shouldn't we allow to specify tags as well?

[mazyu36]

Looking at the other L2 Constructs, it seemed like there were few resources adding tags as properties, so I didn't add them.
Since tags can also be added through aspects, I thought they might be unnecessary like other resources. What do you think?
I would appreciate your opinion on the criteria for adding tags, as I'm not sure about it.

[lpizzinidev]

Makes sense 👍 Thanks for clarifying!

[lpizzinidev]

Can you also add validation for the autoScalingConfigurationName property (docs)?

[mazyu36]

Thanks. I added validation and unit tests.

[mazyu36]

@lpizzinidev
Thank you for the review!
I've addressed all your comments.

[lpizzinidev]

Thanks 👍 Left suggestions for minor adjustments on validation

[lpizzinidev]

Makes sense 👍 Thanks for clarifying!

[mazyu36]

@lpizzinidev
Thanks. I've addressed your comments and fix unit tests.

[lpizzinidev]

Thanks 👍

[mazyu36]

@pahud
Thank you for the review.
I've incorporated all your comments.

[pahud]

some minor suggested changes

[pahud]

I guess we probably should make it string. Just like ecs task definition.

aws-cdk/packages/aws-cdk-lib/aws-ecs/lib/base/task-definition.ts

Line 1316 in 3c36fe9

public readonly revision: string;

Also in the doc

AutoScalingConfigurationRevision
The revision of this auto scaling configuration. It's unique among all the active configurations that share the same AutoScalingConfigurationName.

It's unclear to me if it would be number. I am afraid some day it might support revision like latest just like ecs task revision and this would be a breaking change in CDK.

With that being said, I don't see the benefits using number over string. wdyt?

[mazyu36]

Thank you. Personally, I'm in favor of making the revision a string.
However, I have some concerns.

The revision in the Cfn (L1 construct) is a number, but would it be appropriate to change it to a string in the L2 construct? I'm not sure if there's a possibility of it changing, so if you know anything about it, please let me know.

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_apprunner.CfnAutoScalingConfiguration.html#attrautoscalingconfigurationrevision

Additionally, since the already implemented VpcConnector treats the revision as a number, I'm also concerned about having a different approach from that.

https://docs.aws.amazon.com/cdk/api/v2/docs/@aws-cdk_aws-apprunner-alpha.VpcConnector.html#vpcconnectorrevisionspan-classapi-icon-api-icon-experimental-titlethis-api-element-is-experimental-it-may-change-without-noticespan

[pahud]

Thank you for the callout.

I checked the cfnspec and yes the type is Integer, which is number in TS.

"Attributes": {
        "AutoScalingConfigurationRevision": {
          "PrimitiveType": "Integer"
        },

I am fine having it as number for consistency here but I kind of feel if apprunner supports latest revision someday, which would be a breaking change to CFN then CDK would have breaking change as well.

@GavinZZ @paulhcsun what do you think?

[GavinZZ]

Discussed with team internally. I think we don't need to over-optimize the type here. CFN schema type changes are not supposed to happen especially after it's already in CloudFormation resource schema registry.

[pahud]

Consider the code like this

function validateAutoScalingConfiguration(props: {
  minSize?: number;
  maxSize?: number;
  maxConcurrency?: number;
}) {
  const isMinSizeDefined = typeof props.minSize === 'number';
  const isMaxSizeDefined = typeof props.maxSize === 'number';
  const isMaxConcurrencyDefined = typeof props.maxConcurrency === 'number';

  if (isMinSizeDefined && (props.minSize < 1 || props.minSize > 25)) {
    throw new Error(`minSize must be between 1 and 25, got ${props.minSize}`);
  }

  if (isMaxSizeDefined && (props.maxSize < 1 || props.maxSize > 25)) {
    throw new Error(`maxSize must be between 1 and 25, got ${props.maxSize}`);
  }

  if (isMinSizeDefined && isMaxSizeDefined && !(props.minSize < props.maxSize)) {
    throw new Error('maxSize must be greater than minSize');
  }

  if (isMaxConcurrencyDefined && (props.maxConcurrency < 1 || props.maxConcurrency > 200)) {
    throw new Error(`maxConcurrency must be between 1 and 200, got ${props.maxConcurrency}`);
  }
}

The main changes are:

1. Extracted the validation logic into a separate function validateAutoScalingConfiguration to make the code more modular and easier to test.

2. Used typeof to check if the properties are defined and a number as well as !cdk.Token.isUnresolved because if it is unresolved it would not be number? I am not 100% sure here maybe you still need

typeof props.minSize === 'number' && !cdk.Token.isUnresolved(props.minSize);

3. Moved the isMaxConcurrencyDefined check into a separate condition to make the code more readable.

There are a few benefits to using typeof to check if the properties are defined and a number, instead of using
undefined and cdk.Token.isUnresolved:

1. Simplicity and Readability: The typeof checks are more straightforward and easier to understand at a glance. They directly check the type of the value, which makes the code more self-documenting and easier to maintain.

2. Handling Unresolved Tokens: The cdk.Token.isUnresolved check is specifically for handling unresolved tokens in the AWS CDK. However, in this case, we're not just checking for unresolved tokens, but also ensuring the property is defined and a number. Using typeof handles all of these cases in a single, simple check. [1]

3. Robustness: The typeof checks are more robust because they handle a wider range of cases. For example, if the property is set to null, the typeof check will still correctly identify it as not a number, whereas the
   undefined check would not catch this case.

4. Performance: The typeof checks are generally faster than using undefined and cdk.Token.isUnresolved
   , as they are simpler operations. This can be especially beneficial in performance-critical code.

5. Consistency: Using typeof to check the type of a value is a common and widely-accepted pattern in JavaScript/TypeScript development. It aligns with the language's standard practices and makes the code more familiar to other developers.

In summary, the typeof checks provide a more straightforward, robust, and performant way to validate the properties, while also making the code more readable and consistent with standard JavaScript/TypeScript practices. This can lead to better maintainability and easier understanding of the validation logic.

This optimized version of the code is more concise, easier to read, and easier to maintain. It also follows best practices for input validation, such as using clear error messages and keeping the validation logic separate from the main application logic.

[pahud]

I would consider to make it a construct private method and use something like this.validateAutoScalingConfiguration() for all the validations, it's very opinionated and not mandatory though.

[mazyu36]

Thanks. I like your suggested your approach.
I've created a private method including autoScalingConfigurationName validation.

[mazyu36]

@pahud
Thank you.
I have replied to the comments and refactored the code.​​​​​​​​​​​​​​​​

[pahud]

LGTM now! I'll let other maintainers to take a final review.

[GavinZZ]

Thanks Pahud for the thorough reviews and thanks @mazyu36 for the PR and quick responses.

[GavinZZ]

LGTM thanks for contributing

[mergify]

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).

[mazyu36]

@Mergifyio update

[mergify]

update

☑️ Nothing to do

• #commits-behind>0 [📌 update requirement]
• -closed [📌 update requirement]
• -conflict [📌 update requirement]
• queue-position=-1 [📌 update requirement]

[mazyu36]

@Mergifyio refresh

[mergify]

refresh

✅ Pull request refreshed

[mergify]

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-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 5a88d1d
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

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-cdk-automation]

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.