feat(autoscaling): health check configuration

[nmussy]

Implements missing Auto Scaling Group properties HealthCheckType and HealthCheckGracePeriod
They are grouped in a healthCheck object property.
The constructor checks that the following requirement is fulfilled:

If you are adding an ELB health check, you must specify this property.

Fixes #3381

---

Please read the contribution guidelines and follow the pull-request checklist.

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

[nmussy]

I couldn't find a current default value in the CloudFormation documentation. The API Reference mentions a default of 0. Should I use Duration.seconds(0) in the JSDoc?

[eladb]

This will lends itself nicely to a "union-like" API:

export interface Ec2HealthCheckOptions {
  readonly grace?: Duration;
}

export interface ElbHealthCheckOptions {
  readonly grace: Duration; // <-- required here
}

export class HealthCheck {
  public static ec2(options: Ec2HealthCheckOptions = {}): HealthCheck {
    return new HealthCheck('EC2', options.grace && options.grace.toSeconds());
  }

  public static elb(options: ElbHealthCheckOptions): HealthCheck {
    return new HealthCheck('ELB', options.grace.toSeconds());
  }

  private constructor(public readonly type: string, public readonly gradePeriod?: number) { }
}

And then the usage is:

healthCheck: HealthCheck.ec2();
healthCheck: HealthCheck.ec2({ grace: Duration.minutes(5) });
healthCheck: HealthCheck.elb({ grace: Duration.minutes(5) }); // <-- required grace enforced by compiler

[nmussy]

That's a much cleaner API, thanks.

How would you feel about using a Partial<HealthCheckOptions> instead of repeating the Options Interface?

export interface HealthCheckOptions {
  readonly grace: Duration;
}

export class HealthCheck {
  public static ec2(options: Partial<HealthCheckOptions> = {}): HealthCheck {
    return new HealthCheck('EC2', options.grace && options.grace.toSeconds());
  }

  public static elb(options: HealthCheckOptions): HealthCheck {
    return new HealthCheck('ELB', options.grace.toSeconds());
  }

  private constructor(public readonly type: string, public readonly gradePeriod?: number) { }
}

[nmussy]

Never mind, it seems like JSII doesn't support the Partial keyword:

09:46:31 - File change detected. Starting incremental compilation...
@aws-cdk/aws-autoscaling: lib/auto-scaling-group.ts:712:21 - error TS9999: JSII: Only string index maps are supported
@aws-cdk/aws-autoscaling: 712   public static ec2(options: Partial<HealthCheckOptions> = {}): HealthCheck {
@aws-cdk/aws-autoscaling:                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[nmussy]

Done, thanks

[eladb]

If healthCheck is not specified, then there are no health checks, right? Fix the default description.

[nmussy]

I think the health check is mandatory. The CloudFormation documentation says EC2 is the default, and the API reference says the value is required.

[eladb]

Got it, so we should say that EC2 with no grace period is the default.

[nmussy]

Done

[eladb]

No need to export this.

[nmussy]

Done

[eladb]

This could be a Duration actually and converted at the usage site

[nmussy]

Done

[eladb]

add a test for ec2

[nmussy]

Done

[eladb]

Got it, so we should say that EC2 with no grace period is the default.

[eladb]

I am assuming no grace is the default?

[nmussy]

Yeah, like I said in the original PR, the CloudFormation doc isn't explicit, but the API reference says the duration is 0 seconds.
I updated the JSDoc accordingly