where are these defaults defined ?

@shikha372

I used this default value as defined in FargateTaskDefinitionProps.

See:

Should I write the code as follows?

  private validateContainerCpu(containerCpu?: number, cpu: number = 256) { // default value for FargateTaskDefinitionProps.cpu is 256

Thanks @ren-yamanashi , I see the default values are in fargate task definition and not being set under props.cpu of ApplicationLoadBalancer class, I would prefer if we keep the check without default value as we might miss the update in case it changes in future as well.

@shikha372

Thank you for your comment.

In the current validateContainerCpu method, it compares the values of containerCpu and cpu and returns an error if containerCpu exceeds cpu.

As you pointed out, referencing the default value of FargateServiceBaseProps.cpu in ApplicationLoadBalancedFargateService’s validateContainerCpu could indeed lead to an issue where we might miss the update in case it changes in future as well.

However, avoiding the use of the default value of FargateServiceBaseProps.cpu within ApplicationLoadBalancedFargateService to address this concern could lead to the following problem:

validateContainerCpu method might not validate properly

If we don’t reference the default value of FargateServiceBaseProps.cpu, the cpu param in validateContainerCpu would need to be optional.
(because props.cpu is optional)

  private validateContainerCpu(containerCpu?: number, cpu?: number) {
    /**
     * ... Implementation of the method
     */
  }

In this scenario, it becomes challenging to validate cases where:

• containerCpu is set, but cpu is not.

In this cases, an error should be returned if containerCpu exceeds the default value of FargateServiceBaseProps.cpu (256), but without referencing the default value within the method, determining if an error should be returned becomes difficult.

Possible Approaches for Resolution

Taking these considerations into account, I suggest the following approach. What could you think?
Also, if there are any other good ways, could you please let me know?

Note: this approaches would result in referencing the default value of FargateServiceBaseProps.cpu in ApplicationLoadBalancedFargateService, but I thought it could be more appropriate than the current implementation, so I am suggesting them.

Override props.cpu in ApplicationLoadBalancedFargateServiceProps

override props.cpu in ApplicationLoadBalancedFargateServiceProps and pass it to the FargateTaskDefinition argument within ApplicationLoadBalancedFargateService, as shown below:

export interface ApplicationLoadBalancedFargateServiceProps extends ApplicationLoadBalancedServiceBaseProps, FargateServiceBaseProps {
  /**
   * ... some comments
   *
   * @default 256
   */
  readonly cpu?: number;

 /**
  * ... some props
  */
}

export class ApplicationLoadBalancedFargateService extends ApplicationLoadBalancedServiceBase {
  // ... some implementation
  constructor(scope: Construct, id: string, props: ApplicationLoadBalancedFargateServiceProps = {}) {
    // ... some implementation

      this.taskDefinition = new FargateTaskDefinition(this, 'TaskDef', {
        cpu: props.cpu ?? 256,
        // ... some props
      });
  }
}   

Thank you @ren-yamanashi for defining the alternate approach in detail, I would not modify the current behaviour of the construct by setting the default for props.cpu, I believe in case, value of containerCpu and cpu doesn't align with each other, this would result in a failed deployment (please let me know if that is not the case), given that if intention by adding this validation is to fail fast before the deployment itself, can we add a warning instead of an error in the function so that in case these values change in near future it will not block customer deployments.

@shikha372

I apologize for the delay in responding to you.

If the values of containerCpu and cpu do not match, no error occurs, and the error occurs if the value specified for containerCpu is greater than cpu.

given that if intention by adding this validation is to fail fast before the deployment itself, can we add a warning instead of an error in the function so that in case these values change in near future it will not block customer deployments.

Perhaps the assumed behavior is,

• When the default value of cpu is changed and the containerCpu value becomes an incorrect value, a warning is issued without changing the implementation.

I think it, is this correct?

Currently, if the default value of cpu is changed, it is unclear what the cpu value actually specified when this validation is executed.

Therefore, I think that such an implementation would be difficult (I just haven't thought of it, but there may be a better idea...)

As an alternative, Considering the concerns you mentioned, I thought it would be better to allow containerCpu to be specified only if cpu is specified. What are your thoughts on this?

Hi @ren-yamanashi , discussed with @shikha372 and the suggestion is to validate using this.taskDefinition.cpu since FargateTaskDefinition sets a default for cpu. i.e.

this.validateContainerCpu(props.containerCpu, this.taskDefinition.cpu);

It looks like we will need to surface cpu in the FargateTaskDefinition class as a public readonly member in order to do this. Let us know if you're still planning to work on this, otherwise we can take over to get it merged (we will make sure to credit you as a co-author :) ). Thanks!

@gracelu0

Thank you for review and suggestions on a clear implementation policy!
I will modify the implementation on my part!

@gracelu0

I have fixed it, so please confirm!

40fb61f

Hi @ren-yamanashi , discussed with @shikha372 and the suggestion is to validate using this.taskDefinition.cpu since FargateTaskDefinition sets a default for cpu. i.e.

this.validateContainerCpu(props.containerCpu, this.taskDefinition.cpu);

It looks like we will need to surface cpu in the FargateTaskDefinition class as a public readonly member in order to do this. Let us know if you're still planning to work on this, otherwise we can take over to get it merged (we will make sure to credit you as a co-author :) ). Thanks!