Skip to content

fix(ci): fail Azure deploys on unreadable terraform outputs and show logs after a failed health check - #1304

Draft
IAmJSD wants to merge 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-438
Draft

IAmJSD wants to merge 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-438

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Root cause

Follow-ups from the review of #437, each re-checked against current main (a14dc8f2):

  1. Log step skipped on failure (still present). In deploy-azure.yml, test-deployment / "Check Container App logs" had no if:. So it was skipped exactly when "Test readiness endpoint" or "Run smoke tests" failed, which is when the logs are needed.
  2. Terraform output errors swallowed (still present). "Terraform Apply" read its three outputs with terraform output -raw X 2>/dev/null | grep -v '::' || echo "". That threw away terraform's error and published an empty step output. The failure then surfaced a job later as "Failed to get app URL" or "Terraform outputs missing", far from the cause.
  3. Tests for the health check (mostly done already). fix(server/azure): exclude uninitialized replicas from readiness #488/fix(ci/azure): wait for healthy streak in smoke test instead of failing on first unhealthy response #516 moved the smoke gate into scripts/wait-for-healthy.sh, and scripts/test-wait-for-healthy.sh (run in CI) already covers healthy, degraded/503, connection errors and 301. The empty-body and non-JSON shapes that fix(ci): fail Azure deployment verification on degraded health and log lookup errors #217 named were still missing.
  4. Warm-up curl --max-time 60 nit (already fixed). fix(server/azure): exclude uninitialized replicas from readiness #488 removed the warm-up request. The gate's only curl uses --max-time 10.

Fix

  • New scripts/read-tf-outputs.sh KEY=OUTPUT_NAME..., called from the Apply step in place of the three inline lines. It:
    • runs terraform output -raw NAME and leaves terraform's stderr in the log;
    • fails with ::error:: (exit 1) if terraform fails, or if the value isn't exactly one non-empty line after dropping lines that contain :: (the setup-terraform wrapper's ::debug:: noise, the same filter as 3d0d9de);
    • prints the raw stdout on failure instead of guessing;
    • validates its arguments (exit 2);
    • writes to $GITHUB_OUTPUT only after every output has been read, so a failure never leaves a half-written set.
  • "Check Container App logs" now has if: always() && steps.azure-login.outcome == 'success', and the Azure Login step in that job gets id: azure-login. It runs after a failed gate but not when there is no Azure session to query.
  • New scripts/test-read-tf-outputs.sh, added to CI's azure-kv-access-policy job next to the existing smoke-gate self-test. It:
    • unit-tests the script with a stub terraform;
    • extracts the workflow's own "Terraform Apply" and "Check Container App logs" run: blocks and executes them with stub terraform/az on PATH (the same extraction helpers as test-migration-down-gate.sh);
    • checks the log step's always() condition and the azure-login id it refers to;
    • fails if any terraform output call in deploy-azure.yml still pipes to 2>/dev/null or || echo.
  • scripts/test-wait-for-healthy.sh gains four cases: 503 with an empty body, 502 with an HTML (non-JSON) body, that 502 resetting a streak, and 200 with an empty body counting as ready, because the status code is the contract.

Closes #438

Deploy pause: when this matters, and why merging is safe

Azure deploys have been paused since 2026-10-20 (deploy-azure/gcp/all workflows disabled). Until they are re-enabled this changes nothing at runtime; it takes effect on the first Azure deploy after the pause lifts. Merging is safe while paused: the only change that runs on every PR is one new offline step in ci.yml, a shell test with stub binaries and no cloud or network access.

There is one behavior change to know about for that first deploy. If terraform apply succeeds but an output can't be read, the Apply step now fails with terraform's error, instead of passing and failing later in test-deployment. That is the intent of this issue.

Verification

All of this is offline evidence from stubs and fixtures, run on the branch head. No workflow was dispatched and no terraform or az command reached a real backend.

export PATH=$HOME/.local/go/bin:$HOME/go/bin:$HOME/.local/bin:$PATH
bash scripts/test-read-tf-outputs.sh          # passed=27 failed=0
bash scripts/test-wait-for-healthy.sh         # 0 failed (4 new cases PASS, real-curl cases included)
shellcheck scripts/read-tf-outputs.sh scripts/test-read-tf-outputs.sh scripts/test-wait-for-healthy.sh   # clean
actionlint .github/workflows/deploy-azure.yml .github/workflows/ci.yml                                   # clean (shellcheck on PATH)
pre-commit run {trailing-whitespace,end-of-file-fixer,actionlint,zizmor} --files <changed files>         # all Passed

Fail before / pass after. The workflow half of the new test was run against origin/main's deploy-azure.yml (WORKFLOW=<main copy> bash scripts/test-read-tf-outputs.sh) and gave passed=21 failed=5:

  • workflow apply: a missing output did not fail the step loudly (exit 0)
  • workflow apply: published 'resource_group_name='
  • workflow: a terraform output error is still swallowed (lines 286, 290, 293)
  • workflow logs: step has no always() condition ...
  • workflow logs: missing outputs (exit 1 ...)

All five pass on this branch.

Mutation probes on read-tf-outputs.sh. Each probe makes the suite fail:

Mutation Failing cases
No exit after a terraform failure or bad line count 9
:: filter removed 2
Empty value allowed 3
Multi-line value allowed 1
Write each line directly instead of after all reads 2
Argument-count check dropped 1
Output-name validation dropped 3

Merge check. git merge-tree against the open branches that touch deploy-azure.yml or ci.yml (#1147, #1222, #1227, #1119, #1142, fix/issue-1233, -177, -59): no conflicts.

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ b56afb2b = origin/main a14dc8f2 plus these heads, merged in this order:

Branch Tested head
fix/issue-204 76f85a4
fix/issue-438 78fe011
fix/issue-838 120b07f
fix/issue-143 aef1256
fix/issue-349 cfbb551
fix/issue-80 63405f9
fix/issue-199 36b9f8f
fix/issue-41 96fc237
fix/issue-299 6af14ed
fix/issue-324 3ab6729

fix/issue-324 and fix/issue-204 both edit .github/workflows/ci.yml next to the azure-kv-access-policy job. The tree kept #324's new "Run Function URL reader self-tests" step and #204's updated job comment; whichever PR merges second takes that same resolution.

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran on ed0c7174, which differs from b56afb2b only by one .gitallowed line on fix/issue-324:

  • Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
  • Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
  • Integration tests, every changed package that has them, one package at a time (-tags integration -count=1, 60-minute local timeout): internal/api, internal/purchase, internal/scheduler and internal/server all pass.
  • Secrets: the first run caught a bare placeholder account ID in fix/issue-324's new tftest, now allowlisted with an anchored .gitallowed line. Re-run at b56afb2b: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
  • Nothing was applied to any cloud.

Out of scope

  • deploy-aws-fargate.yml, deploy-aws-lambda.yml and deploy-gcp.yml use the same terraform output ... 2>/dev/null | grep -v '::' || echo "" pattern. This issue is about Azure only. read-tf-outputs.sh takes generic KEY=OUTPUT_NAME pairs, so those workflows can adopt it in a follow-up.
  • The grep -v '::' wrapper filter is kept as it was. Setting terraform_wrapper: false on setup-terraform would remove the noise at the source, but it changes plan/apply annotations for the whole job, so it is a separate decision.

Labels

A maintainer, please mirror the issue's triage labels onto this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/s.

🤖 Generated with Claude Code

…logs after a failed health check

The Azure Terraform Apply step read its outputs with
`terraform output -raw X 2>/dev/null | grep -v '::' || echo ""`, which
hid terraform's error and published an empty step output. The failure
then surfaced a job later as "Failed to get app URL" or "Terraform
outputs missing", far from its cause. The outputs are now read by
scripts/read-tf-outputs.sh, which keeps terraform's stderr, still drops
the setup-terraform wrapper's `::` lines, requires exactly one non-empty
value line per output and writes nothing to GITHUB_OUTPUT unless every
output was read.

The "Check Container App logs" step had no condition, so it was skipped
exactly when the readiness or smoke gate failed. It now runs with
`always()` once the Azure login succeeded, and warns instead of failing
when there is no app to read from; an az lookup error still fails it.

scripts/test-read-tf-outputs.sh tests the script with a stub terraform
and executes the workflow's own Apply and log-step run blocks with stub
terraform/az; it runs in CI next to the smoke-gate self-test.
test-wait-for-healthy.sh gains the empty-body and non-JSON response
cases LeanerCloud#217 asked for.

Closes LeanerCloud#438

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added impact/internal Team-internal only type/chore Maintenance / non-user-visible triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/s Hours labels Oct 23, 2026
@coderabbitai

coderabbitai Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci(deploy): Azure log step should run on failure; stop swallowing terraform output errors; add tests for health-check scripts

1 participant