Skip to content

fix(ecs): make Cluster and task definitions assignable to their interfaces under exactOptionalPropertyTypes#38097

Open
laazyj wants to merge 2 commits into
aws:mainfrom
laazyj:ecs-exactoptional
Open

fix(ecs): make Cluster and task definitions assignable to their interfaces under exactOptionalPropertyTypes#38097
laazyj wants to merge 2 commits into
aws:mainfrom
laazyj:ecs-exactoptional

Conversation

@laazyj
Copy link
Copy Markdown

@laazyj laazyj commented Jun 5, 2026

Issue # (if applicable)

Refs #37996

Reason for this change

ICluster and ITaskDefinition declare several members optional (?: T), but the concrete classes exposed them through getters typed T | undefined. Under TypeScript's exactOptionalPropertyTypes, a member typed T | undefined is not assignable to an optional ?: T, so consumers who enable that flag hit TS2420 errors and type-checking aws-cdk-lib fails for them outright, e.g.:

error TS2420: Class 'Ec2TaskDefinition' incorrectly implements interface 'IEc2TaskDefinition'.
  Types of property 'executionRole' are incompatible.
    Type 'IRole | undefined' is not assignable to type 'IRole'.
error TS2420: Class 'Cluster' incorrectly implements interface 'ICluster'.
  Types of property 'defaultCloudMapNamespace' are incompatible.

aws-cdk-lib does not build with the flag, so the break is invisible to the library's own compile and only surfaces downstream. This is one package in the sweep tracked by #37996.

Affected members:

  • TaskDefinition.executionRole — inherited by Ec2TaskDefinition, ExternalTaskDefinition, and FargateTaskDefinition;
  • Cluster.defaultCloudMapNamespace, Cluster.autoscalingGroup, and Cluster.executeCommandConfiguration.

Description of changes

Convert each offending getter to a public readonly x?: T field. Two population mechanisms are used, depending on where the value is written:

  • Written after construction (executionRole via obtainExecutionRole, defaultCloudMapNamespace via addDefaultCloudMapNamespace): populated through a small module-private helper that assigns via a Writeable cast. TypeScript forbids assigning a readonly field outside the constructor, so the cast is the minimal escape hatch; the live view is preserved (the property still reflects the post-construction population).
  • Written only in the constructor (autoscalingGroup, executeCommandConfiguration): a direct guarded readonly assignment — no cast needed.

The fields stay readonly, matching the original getters (no setter), so the jsii assembly is unchanged — every affected property remains optional: true, immutable: true, i.e. jsii-diff reports no change for this package. The internal ImportedCluster (the fromClusterAttributes path) is updated the same way so the module is fully clean under the flag.

A new per-package aws-ecs/lib/private/writeable.ts holds the shared Writeable<T> type (the package now has more than one set* helper).

Description of how you validated your changes

  • Behavior tests: each property is undefined by default, populated from props, and — for executionRole and defaultCloudMapNamespace — correctly reflects the live post-construction population (obtainExecutionRole, addDefaultCloudMapNamespace), including obtainExecutionRole idempotency.
  • A package-local guard (aws-ecs/test/exact-optional-property-types.test.ts) that type-checks the package's concrete classes (Cluster, TaskDefinition, Ec2TaskDefinition, ExternalTaskDefinition, FargateTaskDefinition) against their interfaces with exactOptionalPropertyTypes on, asserting the package stays clean. Verified it goes red (TS2375) if the fix is reverted.
  • jsii-diff (yarn compat) reports no change for this package — every affected property stays optional: true, immutable: true. (Locally the command also surfaces a few unrelated cloud_assembly_schema removals from the in-flight 2.258.0 release vs the last published baseline; those are not part of this change.) Lint clean; 192 tests pass.

Note on the shared test harness and PR ordering

The guard uses a small shared helper, exactOptionalAssignabilityDiagnosticsForPackage, added to @aws-cdk/cdk-build-tools. Several sweep PRs (one per package) each carry a byte-identical copy of this helper and import it, so they are ordering-agnostic and can land in any order. Whichever lands first establishes the helper; the others can be rebased to drop the duplicate before merge.

Checklist


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

…faces under exactOptionalPropertyTypes

`ICluster` and `ITaskDefinition` declare several members optional (`?:`), but
the concrete classes exposed them through getters typed `T | undefined`. Under
TypeScript's `exactOptionalPropertyTypes`, a member typed `T | undefined` is not
assignable to an optional `?: T`, so consumers who enable the flag get TS2420
errors and type-checking `aws-cdk-lib` fails for them outright. The library does
not build with the flag, so the break is invisible to its own compile.

Affected members:
- `TaskDefinition.executionRole` (inherited by Ec2/External/Fargate task
  definitions), and
- `Cluster.defaultCloudMapNamespace`, `Cluster.autoscalingGroup`,
  `Cluster.executeCommandConfiguration`.

Convert each getter to a `public readonly x?: T` field. `executionRole` and
`defaultCloudMapNamespace` are populated after construction (by
`obtainExecutionRole` and `addDefaultCloudMapNamespace`), so they are set through
a module-private helper that assigns through a `Writeable` cast; the live view is
preserved. `autoscalingGroup` and `executeCommandConfiguration` are only set in
the constructor, so they use a direct guarded `readonly` assignment. The fields
stay `readonly`, so the jsii assembly is unchanged (the properties remain
optional and immutable) and `yarn compat` passes with no new entry. The internal
`ImportedCluster` is updated the same way so the module is fully clean.

Add behavior tests (undefined by default, populated from props, and the live
population via `obtainExecutionRole` / `addDefaultCloudMapNamespace`) and a
package-local guard that type-checks the package's concrete classes against
their interfaces under the flag, asserting the package stays clean.

Refs aws#37996
@laazyj
Copy link
Copy Markdown
Author

laazyj commented Jun 5, 2026

Exemption Request: This is a type-only change — it converts existing read-only getters on Cluster and TaskDefinition (and the internal imported variants) to equivalent readonly fields so the classes are assignable to their interfaces under exactOptionalPropertyTypes. There is no runtime behavior change and no change to synthesized output or the jsii assembly (every affected property stays optional: true, immutable: true; yarn compat passes with no new entry), so an integration test would add nothing. The change is fully covered by unit tests (behavior tests for each property, including the live post-construction population paths, plus a package-local type-assignability guard). Requesting the pr-linter/exempt-integ-test label.

@github-actions github-actions Bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jun 5, 2026
@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Jun 5, 2026
Copy link
Copy Markdown
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

✅ A exemption request has been requested. Please wait for a maintainer's review.

@laazyj
Copy link
Copy Markdown
Author

laazyj commented Jun 5, 2026

Correction to my exemption note above: to be precise about compatibility — jsii-diff reports no change for this package. Every property affected by this PR stays optional: true, immutable: true (the getters and the equivalent readonly fields model identically in jsii), so there is no API/assembly change. Run locally, the yarn compat command currently also surfaces a few unrelated cloud_assembly_schema symbol removals (from the in-flight 2.258.0 release vs the last published aws-cdk-lib baseline); those are not part of this change and reproduce on a clean main.

@laazyj
Copy link
Copy Markdown
Author

laazyj commented Jun 5, 2026

Correction / resolution on the compat note: those cloud_assembly_schema diff entries I mentioned were a local stale-node_modules artifact, not a real change. After a yarn install to resync dependencies to current main and a clean rebuild, yarn compat passes cleanly (exit 0, no entries) and yarn build is green. So this PR is fully compat-clean — every affected property remains optional: true, immutable: true, with no API/assembly change. Apologies for the noise in the earlier comment.

Replace the hand-written assignability snippet with the shared
`exactOptionalAssignabilityDiagnosticsForPackage` helper, which enumerates every
exported, non-abstract class in the package and type-checks its assignability to
each interface it implements (own and inherited) under the flag.

Enumerating — rather than listing classes by hand — keeps the guard complete by
construction: a newly-added offending class, or a new optional getter on an
existing one, is caught automatically. Re-confirmed `Cluster` and the task
definitions (executionRole / defaultCloudMapNamespace / autoscalingGroup /
executeCommandConfiguration) are the only offenders in aws-ecs.

Refs aws#37996
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants