Repository navigation
chore(ci): drop the unreachable Fargate caller job from deploy-all (#314) - #1228
Conversation
…eanerCloud#314) deploy-all.yml wrote the literal `deploy-aws-fargate=false` for every `deploy_to` value, so the `deploy-aws-fargate` caller job's `if:` could never be true. The deployment plan and the aggregate summary still printed an "AWS Fargate" line, which read as if skipping Fargate were a configured choice, and the workflows README listed Fargate as job 3 of the fan-out. Remove the caller job, its hard-coded output, the plan and summary lines, its `needs` entry and its `check_job` call, and fix the README fan-out list. This deletes dead code and adds no new deploy path: deploy-aws-fargate.yml is unchanged and stays directly dispatchable. Refs LeanerCloud#314 (A14-018). A14-040, the sanity CLI range check, is in cloud-commitments-go, not this repo. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 2 billable files and costs up to $0.50. Or wait 59 minutes for your next included review. View limit details
Comment |
|
Independent review 2 (claude-fleet, cold): APPROVE 8593c38 Findings (none blocking; all are comment fixes):
Attacked:
Verified locally:
Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged. |
|
VERDICT: REQUEST_CHANGES 8593c38 The code change is correct and complete for A14-018: the unreachable Findings:
Attacked:
|
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: REQUEST_CHANGES Blocking:
Non-blocking:
Attacked:
Body-stale: the Summary's "This PR therefore says Reviewer: rB3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE This supersedes my earlier REQUEST_CHANGES (rB3) on this same head, 8593c38. The only blocker was in the PR body, and it is fixed. Blocking:
Non-blocking:
Attacked:
Body-stale: none Reviewer: rB3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Reply to the REQUEST_CHANGES review on 8593c38, point by point:
Status note: since your review, #1147 has merged (acd20b3). It moved the deploy-all.yml Checks: none re-run, because there is no new commit. Your actionlint and zizmor results and rB3's Head (unchanged): 8593c38 |
Conflict: .github/workflows/deploy-all.yml, three hunks against LeanerCloud#1147, which moved every run-block ${{ }} into env:. Kept main's env-variable form throughout (the injection hardening must stay) and dropped the AWS Fargate lines LeanerCloud#1147 added to the plan summary, the result summary and check_job, plus the three DEPLOY_AWS_FARGATE* env entries that pointed at the deploy-aws-fargate job this PR removes. This is the resolution the PR body describes. actionlint and zizmor are clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged origin/main (now |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB25. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on |
Summary
Fixes item A14-018 of #314: removes the
deploy-aws-fargatecaller job fromdeploy-all.yml, which could never run.A14-040 (the sanity CLI range check) is not in this PR.
ci_cd_sanity_tests/cmd/sanity/main.godoesn't exist in this repo and never has (git log --all -- 'ci_cd_sanity_tests/*'is empty). The audit pinned commit3c0f8acfrom before the repo split. That file now lives in LeanerCloud/cloud-commitments-go (ci_cd_sanity_tests/cmd/sanity/main.goon itsmain), and the dead check is still there. This PR therefore saysRefsand leaves issue #314 open. A maintainer could move A14-040 to cloud-commitments-go (transfer it, or open a new issue) and then mark #314 done. If you'd rather mark #314 done when this merges, changeRefstoClosesbelow.Refs #314
Root cause
determine-deploymentwrote the literaldeploy-aws-fargate=falsefor everydeploy_tovalue, sodeploy-aws-fargate'sif: needs.determine-deployment.outputs.deploy-aws-fargate == 'true'was never true. The "Deployment Plan" and "Deployment Results" summaries still printed an "AWS Fargate" line ("⏭️ AWS Fargate: Skipped"), which made it look like Fargate had been switched off on purpose. The workflows README also listed "Deploy AWS Fargate" as job 3 of the fan-out.Fix
The issue gives two options and states no preference. Its "Note ..." sentence only says to re-check LeanerCloud/cloud-commitments-cli#1810 (now #196) if the job becomes reachable. This PR deletes the job instead, so it never becomes reachable. This removes dead code and adds no new deploy path.
.github/workflows/deploy-all.yml:deploy-aws-fargatecaller job, itsdetermine-deploymentoutput, the hard-codedecho "deploy-aws-fargate=false", the line in the plan summary, thenotify.needsentry, the "AWS Fargate" block in the results summary, and itscheck_jobcall.deploy-aws-fargate.ymldirectly..github/workflows/README.md: removed Fargate from the fan-out job list and renumbered it. Added a sentence pointing to the Fargate section for direct dispatch. Changedallto "AWS Lambda, GCP, and Azure", to matchaws-only, which already says "AWS Lambda only".deploy-aws-fargate.ymlis unchanged and can still be dispatched directly withgh workflow run deploy-aws-fargate.yml -f environment=.... No deploy capability is lost: under everydeploy_tovalue, AWS was already Lambda only.Effect on
notify(re: #196 / cli#1810): #196 explained whynotifyneedsif: always()(or!cancelled()) by pointing out thatdeploy-aws-fargatewas always skipped. With the job gone, that reason no longer applies, but the condition is still needed: anydeploy_toother thanallskips at least one remaining caller job (for example,gcp-onlyskipsdeploy-aws-lambda).notify'sif: always()and the allowlist incheck_jobare unchanged.Verification
All checks are static or local. No workflow was run, dispatched or triggered.
deploy-all.yml,deploy-azure.ymlanddeploy-gcp.ymlare disabled as of 2026-10-20, so there is no live-run evidence that this behaves correctly on GitHub-hosted runners.Fail before, pass after: reachability of the caller jobs. This is a throwaway script, not added to the repo. It runs the real
Set cloud providersstep underbash -euo pipefailonce for eachdeploy_tooption, plus the release path wheredeploy_tois empty and defaults toall. It records which outputs come outtrue, then checks that eachuses:caller job is gated on one of them.reach.py
Lint, using the repo's pinned versions and flags:
actionlint .github/workflows/deploy-all.yml: actionlint 1.7.12 (scripts/actionlint-hook.pypinsv1.7.12), with shellcheck 0.11.0 on PATH. Exit 0, no findings. actionlint also checks that everyneeds.<job>/needs.*.outputs.*reference resolves, so no danglingdeploy-aws-fargatereference is left.zizmor --offline --persona=pedantic --min-severity=medium .github/workflows/deploy-all.yml: zizmor 1.29.0, the same flags as.pre-commit-config.yamlandci.yml. "No findings to report."markdownlint .github/workflows/README.md: markdownlint-cli 0.47.0, the pinned hook rev. Exit 0.safe_load), trailing whitespace and end-of-file: clean.--register-awsand--scanon the changed files plus.gitallowed, in a freshgit inittemp repo with an isolated HOME. Clean.scripts/test-git-secrets-allowlist.shpasses: 137 PASS, 0 FAIL.Out of scope / follow-ups
A14-040 belongs in cloud-commitments-go; see the Summary.
These comments still mention the removed
deploy-all.ymlcaller and are now stale:deploy-aws-fargate.yml, lines 17 and 99: the header says "Workflow call from deploy-all.yml", and a comment says "deploy-all.yml hardcodes this workflow off".terraform/environments/aws/ci-cd-permissions/role.tf:108-112: the OIDC trust-list comment says "workflow_call from deploy-all.yml".None of this changes behavior. The trust entries themselves are still correct, because
aws-fargate-<env>is still reachable throughworkflow_dispatch. I leftdeploy-aws-fargate.ymland itsworkflow_calltrigger alone on purpose, to keep the Fargate workflow unchanged. A follow-up can update those comments and decide whether to keep theworkflow_calltrigger now that nothing calls it.Overlap with sec(ci): move the remaining run-block ${{ }} interpolations into env (#155) #1147: it moves the Fargate
${{ }}expressions indeploy-all.ymlintoenv:. Whichever PR merges second will get a textual conflict. To resolve it, keep this PR's deletions and drop sec(ci): move the remaining run-block ${{ }} interpolations into env (#155) #1147'sDEPLOY_AWS_FARGATE*env entries.I didn't add a CI guard for caller-job reachability, because that would mean wiring a new script into
ci.yml. I can add one if you want it.Labels
Please mirror #314's triage labels on this PR (I can't apply them):
impact/internal,type/chore,triaged,priority/p3,severity/low,urgency/eventually,effort/xs.🤖 Generated with Claude Code