cdk8s-team / cdk8s

Define Kubernetes native apps and abstractions using object-oriented programming
https://cdk8s.io
Apache License 2.0
4.31k stars 291 forks source link

Improve Typescript code generation: fromString, fromNumber #1324

Closed shinebayar-g closed 2 months ago

shinebayar-g commented 1 year ago

Description of the feature or enhancement:

When we define container resources KubeDeployment class is expecting the custom classes instead of just string or number type.

resources: {
    limits: {
        cpu: Quantity.fromString('500m'),
        memory: Quantity.fromString('2Gi'),
    },

export interface ResourceRequirements {
  /**
   * Limits describes the maximum amount of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
   *
   * @schema io.k8s.api.core.v1.ResourceRequirements#limits
   */
  readonly limits?: { [key: string]: Quantity };

  /**
   * Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
   *
   * @schema io.k8s.api.core.v1.ResourceRequirements#requests
   */
  readonly requests?: { [key: string]: Quantity };

}

/**
 * @schema io.k8s.apimachinery.pkg.api.resource.Quantity
 */
export class Quantity {
  public static fromString(value: string): Quantity {
    return new Quantity(value);
  }
  public static fromNumber(value: number): Quantity {
    return new Quantity(value);
  }
  private constructor(public readonly value: string | number) {
  }
}

There are other cases like

endpoints: [
    {
        path: '/prometheus',
        scrapeTimeout: '2s',
        targetPort: ServiceMonitorSpecEndpointsTargetPort.fromNumber(8080),
    },
],

/**
 * Name or number of the target port of the Pod behind the Service, the port must be specified with container port property. Mutually exclusive with port.
 *
 * @schema ServiceMonitorSpecEndpointsTargetPort
 */
export class ServiceMonitorSpecEndpointsTargetPort {
  public static fromNumber(value: number): ServiceMonitorSpecEndpointsTargetPort {
    return new ServiceMonitorSpecEndpointsTargetPort(value);
  }
  public static fromString(value: string): ServiceMonitorSpecEndpointsTargetPort {
    return new ServiceMonitorSpecEndpointsTargetPort(value);
  }
  private constructor(public readonly value: number | string) {
  }
}

Use Case:

UX could be improved a lot if cdk8s generates union types for such use cases. string | number

Proposed Solution:

So it may could look like:

// Usage
resources: {
    requests: {
        cpu: 1,
        memory: '1Gi',
    }
}

// Interface
interface ResourceRequirements {
    readonly cpu: string | number;
    readonly memory: string | number;
}

interface Resources {
    readonly requests: ResourceRequirements;
    readonly limits?: ResourceRequirements;
}

Other:


This is a :rocket: Feature Request

shinebayar-g commented 1 year ago

Another example.

/**
 * Name or number of the port to access on the container. Number must be in the range 1 to 65535. Name must be an IANA_SVC_NAME.
 *
 * @schema ThanosRulerSpecInitContainersStartupProbeHttpGetPort
 */
export class ThanosRulerSpecInitContainersStartupProbeHttpGetPort {
  public static fromNumber(value: number): ThanosRulerSpecInitContainersStartupProbeHttpGetPort {
    return new ThanosRulerSpecInitContainersStartupProbeHttpGetPort(value);
  }
  public static fromString(value: string): ThanosRulerSpecInitContainersStartupProbeHttpGetPort {
    return new ThanosRulerSpecInitContainersStartupProbeHttpGetPort(value);
  }
  private constructor(public readonly value: number | string) {
  }
}

/**
 * Number or name of the port to access on the container. Number must be in the range 1 to 65535. Name must be an IANA_SVC_NAME.
 *
 * @schema ThanosRulerSpecInitContainersStartupProbeTcpSocketPort
 */
export class ThanosRulerSpecInitContainersStartupProbeTcpSocketPort {
  public static fromNumber(value: number): ThanosRulerSpecInitContainersStartupProbeTcpSocketPort {
    return new ThanosRulerSpecInitContainersStartupProbeTcpSocketPort(value);
  }
  public static fromString(value: string): ThanosRulerSpecInitContainersStartupProbeTcpSocketPort {
    return new ThanosRulerSpecInitContainersStartupProbeTcpSocketPort(value);
  }
  private constructor(public readonly value: number | string) {
  }
}
iliapolo commented 4 months ago

I agree that having TS union types would provide a better ergonomic. The problem is that union types are not natively supported in all jsii languages. For example, in Java a union would be translated to an Object, and that essentially breaks down type checking.

The fromNumber and fromString is the way union types are handled in all jsii based libraries (aws-cdk, cdk8s, cdktf, ...)

Unfortunately there's not much we can do here without a major overhaul of jsii code generation.

github-actions[bot] commented 3 months ago

This issue has not received a response in a while and will be closed soon. If you want to keep it open, please leave a comment below @mentioning a maintainer.