Shouldn't we allow to specify tags as well?

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.

Makes sense 👍 Thanks for clarifying!

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

Thanks. I added validation and unit tests.

Makes sense 👍 Thanks for clarifying!

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

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?

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

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?

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.

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.

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.

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