Repository navigation
Conversation
There was a problem hiding this comment.
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.
|
Exemption Request: This fix only removes a synth-time check that blocked runtimePlatform on EC2 tasks. The rendered property is the same one Fargate and Managed Instances tasks already produce. The unit tests in task-definition.test.ts cover the new template output. |
|
Automated review A maintainer will still review this — treat the notes below as a starting point. 📌 Commit This PR fixes a bug in 🔴 0 blocking · 🟡 1 recommended · ⚪ 0 optional Files with findings (1)
Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews. |
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| ephemeralStorage: this.ephemeralStorageGiB ? { | ||
| sizeInGiB: this.ephemeralStorageGiB, | ||
| } : undefined, | ||
| runtimePlatform: (this.isFargateCompatible || this.isManagedInstancesCompatible) && this._runtimePlatform ? { | ||
| runtimePlatform: this._runtimePlatform ? { |
There was a problem hiding this comment.
🟡 Recommended — This change renders the RuntimePlatform CloudFormation property on EC2 task definitions for the first time (CDK previously rejected it), but the existing ECS integration coverage for RuntimePlatform exercises only the Fargate launch type. Enabling a previously-untested property/launch-type combination normally owes a deploy-backed integration test, since a successful synth alone does not prove ECS accepts RuntimePlatform on an EC2 task definition at register time, and without one a regression or service-side rejection would not be caught (INTEGRATION_TESTS.md#when-are-integration-tests-required). The three new unit tests already assert the synthesized template, so the synthesis behavior is locked in.
Suggested change: Add an EC2 task definition carrying a runtimePlatform to the existing framework-integ/test/aws-ecs/test/fargate/integ.runtime.ts (or a new integ.ec2-runtime-platform.ts under aws-ecs/test/ec2/), deploy it to refresh the snapshot via a real deploy (not --dry-run), and commit the updated *.snapshot/. For example:
new ecs.TaskDefinition(stack, 'Ec2RuntimePlatformTaskDef', {
compatibility: ecs.Compatibility.EC2,
runtimePlatform: {
cpuArchitecture: ecs.CpuArchitecture.ARM64,
operatingSystemFamily: ecs.OperatingSystemFamily.LINUX,
},
// ... a container definition
});
Alternatively, if the unit-level template assertions are judged sufficient for this relaxation, apply the pr-linter/exempt-integ-test label with a short justification.
There was a problem hiding this comment.
I didn't add an integ test. Refreshing the snapshot needs a real deploy, and I can't run one. ECS documents runtimePlatform for EC2 task definitions (EC2 task definition parameters), and the ARM64 guide lists EC2 as a supported launch type. I asked for an exemption in the PR comments. If a maintainer still wants a deployed integ test, I'll add one.
Issue # (if applicable)
Closes #39330.
Reason for this change
TaskDefinitionthrowsCannotSpecifyRuntimePlatformwhenruntimePlatformis set on a task that is only EC2 compatible. ECS supportsruntimePlatform(operatingSystemFamilyandcpuArchitecture) for the EC2 launch type too, see the EC2 task definition parameters. So there was no way to pin the CPU architecture or OS family of an EC2 task definition without an escape hatch.Description of changes
The check now only rejects
runtimePlatformfor External tasks, and the property is rendered for every other compatibility. The Windows cpu/memory size check is Fargate specific, so it now only runs for Fargate compatible tasks. Without that, an EC2 task with a Windows OS family and no task-level cpu would fail with a Fargate sizing error.I kept External blocked since I couldn't confirm ECS Anywhere supports it. Nothing that synthesized before changes its output: EC2 tasks with
runtimePlatformused to throw.Describe any new or updated permissions being added
None.
Description of how you validated changes
Added unit tests to
task-definition.test.tsfor EC2 with a Linux runtime platform, EC2 with a Windows OS family and no task size, and External still throwing. The two EC2 tests fail without the change.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license