Repository navigation
fix(ci): resolve deploy environment from inputs, not github.event_name - #1809
Conversation
cristim
added
triaged
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDeployment workflows now preserve explicit environment inputs, map supported events to environments, validate ChangesDeployment environment handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The workflows now resolve deployment environments from explicit inputs and fail instead of guessing when no valid target is supplied, preventing incorrect environment selection; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CallerWorkflow
participant DetermineDeployment
participant CloudWorkflow
CallerWorkflow->>DetermineDeployment: resolve environment from input or event
DetermineDeployment->>DetermineDeployment: validate dev, staging, or prod
DetermineDeployment->>CloudWorkflow: pass validated environment
DetermineDeployment-->>CallerWorkflow: report failure when determination is unsuccessful
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/deploy-all.yml:
- Around line 70-84: Update the ENVIRONMENT selection in the deploy workflow so
push events map to dev before falling back to INPUT_ENVIRONMENT; preserve
release-to-prod mapping and validation for other events.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c4293f0-470b-4126-aa38-b81c3473fc6a
📒 Files selected for processing (5)
.github/workflows/deploy-all.yml.github/workflows/deploy-aws-fargate.yml.github/workflows/deploy-aws-lambda.yml.github/workflows/deploy-azure.yml.github/workflows/deploy-gcp.yml
7ba01a6 to
c80634e
Compare
Pre-merge adversarial review round (independent reviewer, findings folded into
|
| # | Sev | Finding | Fix |
|---|---|---|---|
| F1 | med | notify / Check for failures only tested the four deploy jobs for == "failure". The new allowlist makes determine-deployment the first job that can deliberately fail, and in that case the deploy jobs are skipped, not failed, so the summary would print "✅ All deployments completed successfully!" for a run that deployed nothing. Same shape as #1805 itself. |
Condition now includes needs.determine-deployment.result != "success"; the failure line reworded to cover a run that stopped before deploying |
| F2 | med | The Azure/GCP comment claimed "only a direct push is allowed to imply one". False along exactly the axis this PR fixes: EVENT_NAME is push for a direct push and for a workflow_call from a push-triggered caller. Also required: true on a workflow_call input requires the key, not a non-empty value, so "therefore means none was requested" did not follow. |
Both comments rewritten to state the rule and name the inherited-push residual explicitly |
| F2b | med | deploy-aws-lambda.yml still carried the un-softened version of the same claim |
Same treatment |
| F4 | low | deploy-aws-fargate.yml's comment described an event that cannot have occurred: deploy-all.yml:105 hardcodes deploy-aws-fargate=false, so it has never called Fargate |
Hedged to "would have mis-resolved"; notes the case was latent there and live only for Azure and GCP |
| F6 | nit | Rejected-value quoting applied to only 2 of 5 allowlists | All five now quote |
| F7 | nit | The updated deploy-azure: comment documented its own revision history |
Dropped; git does that |
Accepted as documented residuals, not fixed (F3, F5). Because a caller can deliver an empty value to a required: true input, two paths remain where a future caller could get a wrong environment: an inherited push resolving dev on Azure/GCP, and an inherited release resolving prod on Lambda. Both are unreachable today (deploy-all.yml is the only caller, has no push trigger, and now allowlists what it passes). Separating a direct trigger from an inherited one would need github.job_workflow_ref; that is disproportionate machinery for an unreachable path, so the residuals are written into the comments at the relevant branch instead of denied. deploy-all.yml's own allowlist is likewise unreachable today (its workflow_dispatch input is a server-validated type: choice) and is kept deliberately as the thing that makes the above unreachable.
F8, tense. gh run list --workflow deploy-all.yml returns []. deploy-all.yml has never executed, so the release path has never fired and nothing has been mis-deployed. The commit message and PR body were corrected from past to conditional.
Independently confirmed, no action
${{ inputs.environment }}renders empty, not the literal"null", when the workflow is triggered bypush/release. Theinputscontext is available insteps.envwith no event qualifier, so this is the documented "dereferencing a nonexistent property evaluates to an empty string" case, and Null casts to String as''. The in-repo${{ inputs.deploy_to || 'all' }}idiom atdeploy-all.yml:92is only correct under the same rule. Had this been wrong the failure would have been loud and immediate (allowlist rejection on the next push to main), not silent.deploy-aws-lambda.yml'sSet image tagis not the same bug class; all six trigger combinations enumerated and correct. Reasoning is in the PR body.- No regressions: every declared trigger of all five workflows walked against
origin/main; each path gives the same or a strictly better answer. - Nothing missed: the
workflow_call-callable set is exactly {azure, gcp, fargate, lambda, database-migration};database-migration.ymlalready readsinputs.*directly and re-validates against the same allowlist. - A property worth noting: the new code is correct even if its own premise were wrong. If
github.event_namesomehow wereworkflow_callinside a called workflow, the non-empty input still wins first. The old code was load-bearing on that premise; this is not.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/deploy-all.yml:
- Around line 294-298: Update the deployment-result condition in the summary job
to compare each selected deployment’s result against success rather than
checking only for failure. Ensure cancelled selected jobs are treated as
unsuccessful while skipped, unselected jobs do not trigger the condition; apply
this to deploy-aws-lambda, deploy-aws-fargate, deploy-gcp, and deploy-azure
alongside determine-deployment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23c7149f-0979-457d-b460-63fa705fbebd
📒 Files selected for processing (5)
.github/workflows/deploy-all.yml.github/workflows/deploy-aws-fargate.yml.github/workflows/deploy-aws-lambda.yml.github/workflows/deploy-azure.yml.github/workflows/deploy-gcp.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/deploy-azure.yml
- .github/workflows/deploy-aws-fargate.yml
- .github/workflows/deploy-gcp.yml
- .github/workflows/deploy-aws-lambda.yml
| if [[ "${{ needs.determine-deployment.result }}" != "success" ]] || \ | ||
| [[ "${{ needs.deploy-aws-lambda.result }}" == "failure" ]] || \ | ||
| [[ "${{ needs.deploy-aws-fargate.result }}" == "failure" ]] || \ | ||
| [[ "${{ needs.deploy-gcp.result }}" == "failure" ]] || \ | ||
| [[ "${{ needs.deploy-azure.result }}" == "failure" ]]; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '286,309p' .github/workflows/deploy-all.yml
# Validate in GitHub Actions with a selected reusable deployment job that exits
# as cancelled. Expected: the `notify` job fails and does not emit the success
# summary.Repository: LeanerCloud/CUDly
Length of output: 1342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- deployment job declarations and result aggregation ---'
sed -n '1,120p' .github/workflows/deploy-all.yml
sed -n '120,220p' .github/workflows/deploy-all.yml
sed -n '250,315p' .github/workflows/deploy-all.yml
printf '%s\n' '--- all references to deployment selectors and job conditions ---'
rg -n -C 3 'deploy-(aws-lambda|aws-fargate|gcp|azure)|determine-deployment|needs\..*\.result|if:' .github/workflows/deploy-all.ymlRepository: LeanerCloud/CUDly
Length of output: 20448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from itertools import product
selectors = {
"aws-lambda": {"all", "aws-only", "aws-gcp", "aws-azure"},
"aws-fargate": set(),
"gcp": {"all", "gcp-only", "aws-gcp", "gcp-azure"},
"azure": {"all", "azure-only", "aws-azure", "gcp-azure"},
}
strategies = ["all", "aws-only", "gcp-only", "azure-only", "aws-gcp", "aws-azure", "gcp-azure"]
results = ["success", "failure", "cancelled", "skipped"]
for strategy in strategies:
selected = [name for name, enabled in selectors.items() if strategy in enabled]
false_positive = []
for statuses in product(results, repeat=len(selected)):
current = all(status != "failure" for status in statuses)
proposed = all(status == "success" for status in statuses)
if current and not proposed:
false_positive.append(dict(zip(selected, statuses)))
print(strategy, "selected=", selected, "current-success-but-not-proposed=", false_positive[:8],
"count=", len(false_positive))
PY
printf '%s\n' '--- reusable workflow job-level conditions and cancellation-sensitive constructs ---'
for file in $(fd -i 'deploy-(aws-lambda|aws-fargate|gcp|azure)\.yml$' .github/workflows); do
echo "### $file"
rg -n -C 2 '^[[:space:]]+(if:|needs:|concurrency:)|cancel-in-progress|always\(\)|failure\(\)|cancelled\(\)' "$file" || true
doneRepository: LeanerCloud/CUDly
Length of output: 7179
🌐 Web query:
GitHub Actions documentation needs context result values cancelled skipped reusable workflow caller job result
💡 Result:
In GitHub Actions, you can determine the result of a job in a reusable workflow using the jobs context, which is available within the reusable workflow itself [1][2]. The jobs.<job_id>.result property provides the status of a specific job and can return one of the following string values: success, failure, cancelled, or skipped [1][2]. Regarding the context in a caller workflow: 1. Caller Workflow Limitations: When a reusable workflow is triggered, the caller workflow treats the entire job that calls the reusable workflow as a single unit [3]. You can access the outputs of the reusable workflow, but the individual internal job statuses (like whether a specific internal job was cancelled or skipped) are not automatically exposed as properties in the caller workflow's jobs context [1][4]. 2. Handling Skipped or Cancelled States: If you need to trigger logic in a caller workflow based on the specific outcome of an internal job within a reusable workflow, you must explicitly define outputs in the reusable workflow [3][4]. You can set these outputs to reflect the result of internal jobs using expressions like ${{ jobs.<job_id>.result }} [1][4]. 3. Concurrency and Cancellation: If you use concurrency with cancel-in-progress: true in both the caller and the called reusable workflow, ensure you are not using the same concurrency group name, as this may lead to the caller workflow being prematurely cancelled [5][6]. To cancel sub-workflows, you can apply concurrency settings directly within the reusable workflow definition [7]. For debugging purposes, you can inspect the available information by printing the context to the logs using ${{ toJSON(jobs) }} within the reusable workflow [4].
Citations:
- 1: https://docs.github.com/en/actions/reference/workflows-and-actions/contexts
- 2: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/contexts.md
- 3: https://docs.github.com/en/actions/how-tos/reuse-automations/reuse-workflows
- 4: https://stackoverflow.com/questions/76900821/need-to-skip-a-job-based-on-the-result-of-reusable-workflow-in-github-action
- 5: https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations
- 6: https://docs.github.com/en/enterprise-server@3.13/actions/reference/reusable-workflows-reference
- 7: https://stackoverflow.com/questions/77021903/how-can-i-stop-reusable-workflows-called-from-a-caller-workflow-in-github-action
Treat cancelled selected deployments as unsuccessful.
The current check flags only failure, so a selected job with result cancelled can produce a success summary. Compare each selected deployment result with success; keep unselected jobs as skipped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/deploy-all.yml around lines 294 - 298, Update the
deployment-result condition in the summary job to compare each selected
deployment’s result against success rather than checking only for failure.
Ensure cancelled selected jobs are treated as unsuccessful while skipped,
unselected jobs do not trigger the condition; apply this to deploy-aws-lambda,
deploy-aws-fargate, deploy-gcp, and deploy-azure alongside determine-deployment.
There was a problem hiding this comment.
Valid finding, and already tracked as #1810. Deliberately out of scope for this PR.
You are right about the substance: the Check for failures step tests each deploy job for == "failure", and a cancelled job is not failure, so a run killed mid-apply prints "All deployments completed successfully!". Converting to an allowlist on == "success" is the correct shape.
It is excluded here for one reason: this PR did not cause it. The four == "failure" tests are byte-identical to origin/main. The related change that was folded into this PR is the determine-deployment case, and that one qualified precisely because the new environment allowlist made that job able to fail for the first time, turning previously-impossible skipped states into reachable ones.
Some context recorded on #1810 that is worth having before anyone picks it up, since two of these were wrong when first written:
- It is one site, not a class.
== "failure"appears 4 times indeploy-all.ymland zero times in any other workflow. Enumerated across all 16 workflow files, every other.resultcomparison allowlists on success, including all four per-cloudsummaryjobs.deploy-all.ymlis the outlier, not the pattern. - In-repo precedent exists for the fix shape:
ci.yml:740-744already testscontains(needs.*.result, 'failure')andcontains(needs.*.result, 'cancelled')as separate states. - The four per-cloud summary jobs do print "Deployment failed" on a cancelled run. Wrong wording, safe direction, tracked separately on #1810 so it does not get conflated with this fix.
Leaving the code unchanged here; the gap is documented in-place with a pointer to #1810.
There was a problem hiding this comment.
@cristim, thanks for the detailed clarification.
The cancellation gap is valid, but it predates this PR and is tracked in #1810. The added determine-deployment.result != "success" check addresses the new failure path introduced by this PR. Keeping the four existing deployment-result checks unchanged is appropriate for this PR’s scope.
✏️ Learnings added
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 1809
File: .github/workflows/deploy-all.yml:313-317
Timestamp: 2026-08-12T21:59:41.967Z
Learning: In `.github/workflows/deploy-all.yml`, the `notify` job checks the four deployment job results with `== "failure"`. A selected deployment with result `cancelled` can therefore reach the success summary. This pre-existing gap is tracked in GitHub issue `#1810` and is deliberately out of scope for PR `#1809`. The PR-specific fix is the `needs.determine-deployment.result != "success"` check because PR `#1809` introduces environment validation that can cause `determine-deployment` to fail and downstream jobs to be skipped.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 1809
File: .github/workflows/deploy-all.yml:79-93
Timestamp: 2026-08-12T21:59:15.382Z
Learning: In `.github/workflows/deploy-all.yml`, only `workflow_dispatch` and `release` trigger the workflow. `release` resolves to `prod`, and `workflow_dispatch` requires an allowlisted `dev`, `staging`, or `prod` environment. The `Set environment` allowlist must fail for an empty or unsupported environment rather than map a potential future `push` event to `dev`, so that a new trigger cannot silently choose a deployment environment.
You are interacting with an AI system.
c80634e to
dd19342
Compare
Round 2 adversarial review (head
|
| # | Sev | Finding | Fix |
|---|---|---|---|
| R2-F2 | low | The Azure/GCP comments described the deleted *) default arm in the present tense ("lands on the default arm and resolves to dev"), sitting directly above code that has no default arm. An artefact of the round-1 tense correction, and precisely the failure mode this PR exists to prevent |
Conditional, matching the Fargate hedge: "would have landed on a dev default arm and resolved to dev while the summary said prod" |
| R2-F4 | nit | deploy-all.yml: "prepare takes the input it is passed and fails the run rather than substituting a default" is false unqualified. Azure's prepare does substitute dev on the push+empty arm. True only on the path the comment is about |
One word: "takes the non-empty input it is passed" |
| R2-F3 | low | Three files' comments now rest on three facts about deploy-all.yml (only caller, no push trigger, allowlists what it passes). Adding a push: trigger here would silently falsify all three and restore #1805's failure mode, but the warning lived only in the files that rely on the fact, not where the change would be made |
Warning added at deploy-all.yml's on: block, where that edit would happen. In scope: this coupling did not exist before this PR, since the callees previously ignored inputs entirely |
Declined and split out: R2-F1 → LeanerCloud/cloud-commitments-platform#196
The finding is real. notify runs if: always() and tests the four deploy jobs for == "failure". On a cancelled run, determine-deployment.result is success and the deploy jobs are cancelled, so no clause matches and the summary prints "✅ All deployments completed successfully!" for a run killed mid-apply. That is worse than cosmetic given deploy-azure.yml already names that scenario as the worst outcome available.
It is not this PR's to fix. The four == "failure" tests are unchanged from origin/main, and the cancellation path behaved identically before this change. Contrast the adjacent determine-deployment case, which this PR did fold in: that one only became reachable because adding the allowlist made determine-deployment able to fail for the first time. Caused-by-this-PR is the line, and this falls the other side of it.
It is also a class issue across four workflows — deploy-all.yml plus deploy-azure.yml:453, deploy-aws-lambda.yml:441, deploy-aws-fargate.yml:293 all share the pattern. Fixing one instance inside a PR about environment resolution would half-fix a class and leave three live.
I did implement if: ${{ !cancelled() }}, then reverted it. Filed as #1810 with the full analysis, so the follow-up is short rather than re-derived: the documented rule chain proving !cancelled() bypasses the skipped-need auto-skip (load-bearing, because deploy-aws-fargate is permanently skipped and getting this wrong means notify silently never runs again), the YAML tag-indicator trap that makes ${{ }} mandatory, and a completeness check confirming run-level cancellation is the only producer of cancelled in this chain. Check for failures carries a comment pointing at LeanerCloud/cloud-commitments-platform#196 rather than staying silent about the gap.
Declined outright
R2-NIT — deploy-aws-lambda.yml names release and push as the two legitimate causes of an empty input, then documents the inheritance residual for release only. Adding the push case would dilute the emphasis on the genuinely dangerous one: an inherited release resolves prod, an inherited push resolves dev, and the identical dev residual is already spelled out in full in Azure and GCP. The reviewer ranked it ignorable; I agree, and I was not otherwise touching that paragraph.
Re-verified unchanged this round
dev unreachable from any workflow_call; no regressions on any declared trigger of any of the five workflows; Fargate's EVENT_NAME removal orphans nothing; no callable workflow missed; no newline-injection surface into $GITHUB_OUTPUT. actionlint 113 → 111 versus origin/main, no new findings, none on a touched line. pre-commit clean on all five files.
Also confirmed for the record, since the reviewer chased it independently: the Fargate hedge is exactly right and non-obviously so. Under the old Lambda code a release-triggered deploy-all.yml hit release) TARGET_ENVIRONMENT=prod and got prod, which is what deploy-all.yml computed anyway — so Lambda coincidentally resolved correctly and the bug was never live there either. "Live only for Azure and GCP" holds.
dd19342 to
eb49e5a
Compare
Round 3: a false claim I introduced, now correctedReviewer caught that the comment I added at The false claim. I wrote that the cancelled-run gap was "a class issue across all four deploy workflows' summary jobs". I verified all three siblings and none of them has this bug:
All three are allowlists on success. Why this needed correcting rather than shrugging off. Whoever picked up LeanerCloud/cloud-commitments-platform#196 would have opened three files hunting a bug that is not there, and the plausible outcome is "fixing" three correct summary jobs - churn on the deploy path for no gain. It also inverted the effort estimate: it read as a four-workflow sweep, it is one Corrected in three places:
Also folded into LeanerCloud/cloud-commitments-platform#196 as an explicitly separate, lower-priority task: those three siblings do print "Deployment failed" on a cancelled run, which is wrong in wording but safe in direction. Its fix is a distinct third The environment-resolution change itself is unaffected - no logic changed in this round, comment only. |
Inside a reusable workflow `github.event_name` is the caller's event and is never the literal `workflow_call`, so the `workflow_dispatch|workflow_call` arm in the called deploy workflows is dead code. A release-triggered deploy-all.yml computes environment=prod and passes it to every cloud; each callee then falls through to the `*)` arm and returns dev. Azure and GCP would apply Terraform against dev while the run summary reported prod. Nothing has been mis-deployed yet: deploy-all.yml has never run. The workflow_dispatch path survives only by coincidence, the caller's event there genuinely being workflow_dispatch. Branch on whether an environment was requested instead. `inputs` is populated on workflow_dispatch and workflow_call, the only two triggers that carry an environment, so a non-empty input identifies a workflow_call invocation that no event name can. Where an empty input has no defensible default the step now fails rather than guessing a deployment target. deploy-aws-fargate.yml declares no trigger besides workflow_dispatch and workflow_call, so it loses its fallback outright. deploy-aws-lambda.yml already produced the right answer but hardcoded it in a `release) prod` arm instead of honouring the input; the input now wins and that arm serves only its own direct release trigger. deploy-all.yml is always the entry point of its own run, so its use of github.event_name is sound and stays. It gains the dev|staging|prod allowlist the callees already had, so an empty or unknown environment stops the fan-out rather than reaching four deployments, and it no longer interpolates inputs.environment straight into a shell command. Because that allowlist makes determine-deployment the first job that can deliberately fail, the notify job now treats any non-success there as a failure: the four deploy jobs are skipped rather than failed in that case, so the summary would otherwise announce that all deployments completed successfully for a run that deployed nothing. The comment on the deploy-azure job, which documented that the callee's prepare output need not agree with the environment computed above, was true only because of this bug and is corrected here. Closes #1805
eb49e5a to
3b73caa
Compare
Round 4: sibling count was three, is fourCorrecting my own round-3 comment above, which listed a three-row table.
The three-row table in round 3 said "None of the three has this bug. Do not 'fix' them" while leaving GCP unlisted - which made it the one file a reader might then have gone and "fixed". Same direction as the defect it was correcting. Fixed in the workflow comment ("all four per-cloud Substantive claims re-verified by enumeration, not sampling:
A note on the pattern, now added to the PR body since LeanerCloud/cloud-commitments-platform#196's actionable content is a count: there were four count errors in this changeset - "class across four workflows" (one), "three siblings" (four), an independent check that reproduced the second, and "17 workflows" (16, the 17th entry is Every substantive claim survived all four corrections unchanged. Only the numbers were wrong, which is exactly why they were easy to miss. Comment-only again - no logic moved. |
CI status correction:
|
| Head | Failing jobs | Actual error |
|---|---|---|
7ba01a67 |
Build Docker Image | curl exit code 56 - failure in receiving network data |
c80634ed |
Build Docker Image | same step |
dd19342d |
Build Docker Image, Lint Code | truncated download -> sha256sum: 1 of 1 computed checksums did NOT match; Lint: Error: socket hang up |
CI Success is a gate job and is red only because those are.
The two manifestations are one root cause. curl 56 is an outright receive failure; the checksum mismatch is the same fetch failing after writing a partial file, so the hash of a truncated tarball naturally does not match. The same builds also log WARNING: opening from cache https://dl-cdn.alpinelinux.org/... APKINDEX.tar.gz: No such file or directory, and the Lint failure is socket hang up. Every failure in every run is a network read.
Evidence it is not this PR
- The pin is not stale. I downloaded
migrate.linux-amd64.tar.gzv4.19.1 and computed2ac648fbd1b127b69ab5a7b33cf96212178f71e22379fc50573630c6f4c7ce18- byte-exact match to the Dockerfile's pinned amd64 SHA. Upstream has not republished; nothing is actually wrong with the checksum. - Diff scope.
git diff origin/main --name-onlyfiltered for anything outside.github/workflows/deploy-*returns empty. Five files, all deploy workflows.CI - Build & Testdoes not consume any of them - there is no mechanism by which this change reaches a Go lint run or a Docker build. mainis green, including this branch's base8b16ed4c2.
The honest caveat
Three consecutive failures is more than I would expect from independent flake, so this may be a sustained upstream/runner incident rather than random noise. I cannot fully separate the two: no other branch has run since main at 08:01Z, ~13 hours before my first run, so there is no contemporaneous control. What I can say is that the failure mode is environmental in kind and branch-independent in mechanism.
I am not touching CI to make this pass - an environmental fetch failure is a re-run situation, not debt to paper over. Two runs (eb49e5a, 3b73caa) are in flight; a watcher is armed on the head and will report every terminal conclusion. If they fail the same way this needs a re-run or an infra look, not a code change here.
Resolved:
|
1 participant
Closes #1805
The defect
Inside a reusable workflow
github.event_nameis the caller's event, never the literal stringworkflow_call. Four deploy workflows branched on it:deploy-all.ymltriggers onrelease: types: [created], computesenvironment=prod, and passes it to every callee. On that path the callee seesevent_name=release, matches neither arm of the first case, discards theprodit was handed, and returnsdev. Theworkflow_dispatchpath worked only by coincidence: there the caller's event genuinely isworkflow_dispatch, which the arm happens to match. That is why the bug is invisible on the path anyone would test with.The signal, and why this is not "add
releaseto the case arm"Adding
releasetreats the symptom and stays fragile: the next trigger added todeploy-all.ymlreintroduces it silently.github.event_nameis the wrong signal entirely, because it always describes the triggering event of the whole run and can never answer "was I called by another workflow".The fix branches on whether an environment was requested -
inputs.environment- which answers exactly that question:inputsis populated on precisely the two triggers that carry an environment (workflow_dispatch,workflow_call) and is empty on every other trigger, whatever the caller's event was.environmentis alreadyrequired: trueon both of those triggers in all four workflows, so an empty value means none was requested.In-repo precedent:
database-migration.ymlalready resolves its environment straight frominputs.environment, and its comment at:107-110documents that an input absent on a given path "renders empty".Which workflows shared the pattern
deploy-azure.ymldevdevonly for its ownpushtrigger; everything else failsdeploy-gcp.ymldevdeploy-aws-fargate.ymldevworkflow_dispatch/workflow_call, so the input is the only source of truth and the fallback is removed outrightdeploy-aws-lambda.ymlrelease) prodarm, so it hardcoded the answer instead of honouring the input. The input now wins; the arm serves only its own directreleasetriggerdeploy-all.ymlgithub.event_nameis sound here (this workflow is always the entry point of its own run) and stays. Gains thedev|staging|prodallowlist the callees already haddatabase-migration.ymlinputs.environmentdirectlydeploy-all.ymlgaining the allowlist is what closes the last way to reach a wrong environment through aworkflow_call: it is the only caller of these workflows, and an empty or unknown environment now stops the fan-out instead of reaching four deployments. It also stops interpolating${{ inputs.environment }}directly into a shell command.That allowlist makes
determine-deploymentthe first job in this workflow that can deliberately fail, which surfaced a second, related defect.notifyrunsif: always()and itsCheck for failuresstep only tested the four deploy jobs for== "failure". Whendetermine-deploymentstops the run those jobs are skipped, not failed, so the step summary would print "✅ All deployments completed successfully!" for a run that deployed nothing. That is the same shape as #1805 itself, a summary asserting something the deployment did not do, and this PR made it reachable, so it is fixed here: the step now treats any non-success ofdetermine-deploymentas a failure.A sibling of that gap - a cancelled run also reaching the success line - was deliberately not fixed here and is split out to #1810. It is unchanged from
origin/main, so this PR did not cause it, and it is a class issue across all four deploy workflows' summary jobs; fixing one instance here would leave three live.Check for failurescarries a comment pointing at LeanerCloud/cloud-commitments-platform#196 rather than staying silent about it. The full analysis, including the documented proof that the obvious fix is safe, is on that issue.Blast radius correction
Issue #1805 originally said a release-triggered run "silently targets dev on three clouds". It is two:
deploy-all.yml:105hardcodesdeploy-aws-fargate=false, so a release fans out to Lambda, GCP and Azure only, and Lambda was already resolving toprod. The consequence would be Azure and GCP applying Terraform againstdevwhile the run summary reportedprod. Fargate was latently broken and is fixed here too.Tense matters here: nothing has actually been mis-deployed.
gh run list --workflow deploy-all.ymlreturns[]- the workflow has never executed, so the release path has never fired. The bug is real by inspection and would fire on the first release; it has no history. The impact claim on #1805 has since been retracted by its author on the strength of this, and the correction is posted there.The comment this PR makes false, and CodeRabbit's stale learning
The
deploy-azure:job indeploy-all.ymldocumented that the called workflow'sprepareoutput "is not necessarily theenvironmentcomputed above, so do not assume the two agree". That was correct, and correct precisely because of this bug. CodeRabbit challenged it on #1803, the challenge was declined with the trace, and CodeRabbit agreed the finding was invalid and stored a learning to that effect.This PR inverts that.
preparenow takes the passed input verbatim or fails the run, so the two do agree, and the comment is updated in the same commit. CodeRabbit therefore holds a now-stale learning from #1803 about this exact line. Reviewers should not treat it as authoritative here.Does #1803's concurrency reasoning still hold
Yes, and it was re-checked rather than assumed.
deploy-azure.yml's job-level groupazure-tfstate-${{ needs.prepare.outputs.environment }}and the state keygithub-${ENVIRONMENT}.terraform.tfstateboth derive from the sameprepareoutput, so they stay in lockstep whateverpreparedecides. This change alters only what that output evaluates to, never the fact that both sides read it. The comment atdeploy-azure.yml:130-132("preparerejects anything outside dev|staging|prod and fails the run, so this expression cannot evaluate to an empty suffix") remains true and is now reinforced, since an empty input fails earlier as well.Verification
What was proved. The
Determine environment/Set environmentstep body was extracted from the YAML for bothorigin/mainand this branch (parsed, not retyped), the${{ }}expressions substituted with the values each scenario would present, and executed underbashwith a scratch$GITHUB_OUTPUT.Release path,
event_name=release+inputs.environment=prod:deploy-azure.ymlenvironment=devenvironment=proddeploy-gcp.ymlenvironment=devenvironment=proddeploy-aws-fargate.ymlenvironment=devenvironment=proddeploy-aws-lambda.ymltarget_environment=prodtarget_environment=prodThe negative, a
workflow_callthat passes no environment (event_name=release+inputs.environment=''):deploy-azure.ymlenvironment=dev::error::No environment supplied on a 'release'-triggered run; refusing to guess a deployment target.deploy-gcp.ymlenvironment=devdeploy-aws-fargate.ymlenvironment=dev::error::Refusing unknown environment: ''Unchanged where it should be: direct
pushto main still resolvesdevon Azure, GCP and Lambda; directreleaseon Lambda still resolvesprod;workflow_dispatchstill honours its choice on all four.What was NOT proved. No release was created and no run was executed.
gh run list --workflow deploy-all.ymlis empty, so the release path has never run at all. This is a standalone simulation of the resolution logic, not an end-to-end deploy: it does not prove that the resolvedprodreachesterraform applyagainst the prod state file, only thatpreparenow emitsprodwhere it previously emitteddev. Everything downstream already readsneeds.prepare.outputs.*and is unchanged by this PR.Two residuals, stated rather than hidden. Note that
required: trueon aworkflow_callinput requires the key to be present, not the value to be non-empty, so a caller can deliver an empty input:pushpassing an empty environment would resolvedevon Azure/GCP, becausegithub.event_nameis inherited and reads aspushthere too.releasepassing an empty environment would resolveprodon Lambda, because Lambda's ownreleasetrigger legitimately means prod and is indistinguishable from an inherited one.Neither is reachable:
deploy-all.ymlis the only caller, it has neither apushtrigger nor a path to an empty environment now that it allowlists the value it passes. Telling a direct trigger from an inherited one would needgithub.job_workflow_ref, which is not worth the machinery for an unreachable path. Both residuals are documented in the comments at the relevant branch rather than papered over.actionlint(v1.7.12) is not wired into CI, so the same command was run againstorigin/mainfor comparison:Exit
1on bothorigin/mainand this branch - entirely pre-existing shellcheckSC2086/SC2129debt in untouched steps. Findings go 113 -> 111: no new findings, none on any line this PR touches, and the two that disappear are pre-existing unquoted$GITHUB_OUTPUTuses in thedeploy-all.ymlstep that was rewritten.A note on counts, for whoever picks up LeanerCloud/cloud-commitments-platform#196
Worth recording, because LeanerCloud/cloud-commitments-platform#196's actionable content is a count and this PR is the cautionary tale.
Four count errors occurred in this one changeset, in a PR whose entire subject is code asserting something the codebase does not do:
deploy-gcp.yml:307was missing..ymlfiles; the 17th directory entry isREADME.md.Every one came from counting the shape that was salient - files containing
if: always(), files someone happened to enumerate, entries in a directory listing - rather than enumerating the candidate set and then measuring the defining property on each member. Note that error 3 survived a review that was correct on every substantive point, which is the dangerous part: re-deriving one side of a comparison while accepting the other side as given is not verification.The generalisable rule: a count in a code comment or a filed issue is a claim about the whole repository, and the only thing that makes it true is enumerating every candidate and reading what each one does. Never carry a count forward from a previous message, a previous revision, or someone else's list, including a reviewer's.
The substantive claims survived all four corrections unchanged:
deploy-all.ymlis the only success-by-default check in the repo, and the cancelled-run gap is a single site. Only the numbers were ever wrong, which is precisely why they were easy to miss.Two smaller things
All five allowlist errors now quote the rejected value (
'$ENVIRONMENT'). Unquoted, an empty or whitespace value renders asRefusing unknown environment:with nothing after it. This matches the existing style indatabase-migration.yml:127.deploy-aws-lambda.yml'sSet image tagstep still branches ongithub.event_nameand was deliberately left alone. It is not the same bug class: the environment is a deployment intent the caller communicates out-of-band throughinputs, and the caller's event is a bad proxy for it, whereas the image tag is a property of the code being deployed, which a called workflow genuinely inherits along with the ref it checks out. On a release-triggered caller,github.event.release.tag_nameis the release actually being deployed, which is the correct answer. All six trigger combinations were enumerated and it is right in each; its empty-tag path already fails loudly.