feat(aws/ecs): environmentFiles on the shared task-definition surface - #904
Open
sam-goodwin wants to merge 8 commits into
Open
feat(aws/ecs): environmentFiles on the shared task-definition surface#904sam-goodwin wants to merge 8 commits into
sam-goodwin wants to merge 8 commits into
Conversation
… + both roles' inline policies on delete Every reconcile of AWS.ECS.Task and the image-owning AWS.ECS.Service registers a NEW task-definition revision, but delete only deregistered the single revision recorded in output.taskDefinitionArn — superseded revisions accumulated ACTIVE forever. This is exactly how the "legacy inline ingress is reaped" migration test leaked one task-definition family per run (deploy → rev 1, redeploy → rev 2, destroy removed rev 2 only). - reconcile: after registering a new revision, deregister + hard-delete the superseded one (same-family guarded so a BYO `task:` reference from a prior Service config is never touched); wired in Task and both Service reconcile paths - delete: deleteTaskDefinitionInfrastructure now sweeps EVERY remaining ACTIVE revision of the family, not just the recorded one - delete: inline role policies are deleted for BOTH the task role and the execution role — the Service secrets-read policy lives on the execution role, so destroying a secrets-using Service failed with DeleteConflictException and stranded the role - live tests: Task deploy → redeploy(env change) → superseded revision reaped, destroy → zero ACTIVE revisions; migration test now asserts the reap and an empty family after destroy The 4 leaked families were deregistered + deleted from the testing account. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SST-parity leftover: `environmentFiles` on the primary container for both AWS.ECS.Task and the image-owning AWS.ECS.Service (shared TaskDefinitionConfig surface). The execution role is automatically granted s3:GetObject on the referenced objects and s3:GetBucketLocation on their buckets via the `alchemy-environment-files` inline policy (removed when the prop is cleared; swept on delete by the both-roles inline-policy sweep). Live-tested: Task deploy asserts the registered revision carries the file and the execution role policy grants read; the image-owning Service platform test asserts the same through the Service synthesis path. The `containers` full-shape escape hatch is intentionally NOT added: the synthesized primary container owns the built/mirrored image, and `container` (full ContainerDefinition overrides) + `sidecars` (full ContainerDefinition entries) already span the multi-container surface — a verbatim `containers` list would conflict with both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Install the packages built from this commit: alchemy bun add alchemy@https://pkg.ing/alchemy/f02b8ee@alchemy.run/better-auth bun add @alchemy.run/better-auth@https://pkg.ing/@alchemy.run/better-auth/f02b8ee@alchemy.run/pr-package bun add @alchemy.run/pr-package@https://pkg.ing/@alchemy.run/pr-package/f02b8ee |
Review feedback: single-shot list calls miss pages beyond the first. listTaskDefinitions/listRolePolicies/listAttachedRolePolicies now go through .items(...) streams (Stream.filter/mapEffect/runDrain), which auto-paginate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review feedback: a nonexistent family is an empty list, not an error — the catch defended nothing and turned real client errors into a silent empty sweep, leaking revisions. List failures now propagate so the engine retains state and the delete retries. The deregister/delete catches stay: ClientException is how ECS reports already-inactive / not-found there, which is genuine delete idempotency. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rity # Conflicts: # packages/alchemy/test/AWS/ECS/Task.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on
claude/ecs-taskdef-hygiene-1(#902); merge that first.SST-parity leftover:
environmentFileson the primary container, on the sharedTaskDefinitionConfigsurface so it works for bothAWS.ECS.Taskand the image-owningAWS.ECS.Service.s3:GetObjecton the referenced objects ands3:GetBucketLocationon their buckets (alchemy-environment-filesinline policy, removed when the prop is cleared and swept on delete).The
containers?: ecs.ContainerDefinition[]full-shape escape hatch from the audit is intentionally NOT added: the synthesized primary container owns the built/mirrored image, andcontainer(fullContainerDefinitionoverrides for the primary) +sidecars(fullContainerDefinitionentries) already span the multi-container surface — a verbatimcontainerslist would conflict with both.dev modeis out of scope per the audit follow-up.Known pre-existing CI failures unrelated to this change: the check job's
mysql2/promisetsc errors from Planetscale MySQLMigrations, and the docs build's/kubernetesbroken link (until #892).🤖 Generated with Claude Code