Build the container images in CI, dispatch-only for now #121
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/ci-build-container-images"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Task 187. Adds a
container-imagesjob that builds both images, probes the running API container's non-root user (viadocker exec ... id, not the Dockerfile'sUSERline), and asserts the WebUI'sApi__BaseAddresssubstitution plain and gzipped. ADR-0109.This job does not gate pull requests or pushes to
main. Two independent runs on two different commits (#536, #540) both failed well inside 30 seconds of the job's own job-level time — the only timeout this job carries, on eitherdocker versionordocker info; no shorter bound exists anywhere in the file — ruling out a hang, consistent with this runner having no usable Docker daemon reachable for this job. The exact cause (missing binary, refused socket, permissions) could not be confirmed: no endpoint on this Forgejo instance exposes step-level log text to an agent (task detail 404s, the run page is client-rendered, artifacts are empty), only job-level status and timing. That limitation, and a cheap diagnostic suggestion for whoever picks up task 190, are recorded in ADR-0109 so neither is rediscovered.A job that cannot currently pass does not belong in the gate — it would sit permanently red and train reviewers to ignore it. The job stays in the workflow, fully built, triggered only by
workflow_dispatch, so it can be run by hand and will exercise everything immediately once the runner gains Docker (task 190, infrastructure-owned). Turning it back into a gate at that point is a one-line change (drop the job-levelif:).The path-based relevance filter (skip the build on a pull request touching neither Dockerfile) and the bounded daemon-probe timeouts are unchanged from the original design and verified correct in both directions, ready for the day this job runs automatically again.
Full suite, architecture tests,
dotnet formatand the coverage ratchet all clean on the pinned SDK.3c4c304f7d28f459f612Verdict: changes needed
CI is genuinely red, not silently green — run #536's job list (fetched via
/actions/tasks, notget_workflow_run) showscontainer-images(id 611) actually executed and failed at 14:19:29–14:19:53, alongsidebuild(609, success) ande2e(610, success). That's a real failure to fix, which blocks merge on its own.Verified locally, in both directions, against
origin/main: a scratch commit touchingsrc/PlaceMark.Api/Dockerfileyieldsrelevant=true; a scratch commit touching onlyREADME.mdyieldsrelevant=false. The predicate discriminates correctly now.The job's 24-second duration is too short for a real two-image build and shorter than the 30s daemon-probe timeout, so the failure most likely happened at or before "Confirm the runner can build and run containers" — a fast failure, not a hang. That's an inference from timing only (log access is off-limits for this review); whether the runner has a usable Docker daemon at all is still open, and leans toward no. Worth confirming before assuming this narrowing/probe design is otherwise done.
timeout-minutes: 15and thetimeout 30 docker version/docker infoprobe are both present as described.No
TEMPORARY:commit anywhere in the branch history or diff. No numeric cost figure (the debunked ~30-minute one or any other) anywhere in the workflow comments or in any ADR.The narrowing is stated explicitly in the workflow comments, including the correction of the earlier #532 misattribution — good. It is not yet stated in the PR body, which is still the draft placeholder ("Draft: pushing to get a real CI run before writing the final PR body"). Needs the real body before merge, per your own requirement that it's disclosed in both places.
On the "passed vs never ran" question: the unconditional
$GITHUB_STEP_SUMMARYwrite plus the gated "built and probed" step make this observable to anyone who opens the run's summary, which is the right fix given this Forgejo instance exposes no per-step API. One residual gap worth naming: the job's bare status/conclusion — what any external consumer polling job state sees, including this review's own investigation — still can't tell the two apart; only the summary can. Not a blocker, but worth a line in the PR body alongside the narrowing disclosure.79178869c6e9b75e7bdaBuild and probe both container images in CIto Build the container images in CI, dispatch-only for nowVerdict: changes needed
CI green on
45fdde0:build(629) ande2e(630), both success, run #453.container-imagesdoes not appear in the task list at all for this run — not reported as skipped, simply absent — which is actually the cleaner resolution to the earlier "passed vs never ran" concern: no green check appears for a job that didn't run.1. Evidence for "no usable Docker daemon": sufficient for the actual decision made (pull the job out of the gate), not overreached. Two independent runs, 24s and 8s of job-level time, both comfortably under the daemon probe's real timeout, no hang, no intermittency — that's enough to say "this job cannot currently pass here," which is all removing it from the gate requires. It is not enough to say why, and the ADR correctly doesn't try to. Worth doing before task 190 lands, not blocking this PR: a one-line diagnostic step (
command -v docker, check the socket file,id/group membership) would cheaply distinguish missing-binary from refused-socket from permissions, sharpening that ticket's starting point rather than leaving it to rediscover the same three possibilities from scratch.2. ADR-0109 overclaim check: it doesn't overclaim. States plainly that which of the three explanations is real "could not be confirmed," and gives the log-retrieval limitation in real detail (task-detail 404s, client-rendered run page, empty artifacts). Good.
3. The
if:guard, verified against the file, not taken on trust:on:at the top of the file listspull_request,push,workflow_dispatch— workflow-wide, as claimed.container-imagescarriesif: github.event_name == 'workflow_dispatch'at the job level, and nothing aboutneeds: buildwould otherwise restrict it. The reasoning holds, and the live run confirms it:container-imagesgenuinely didn't run on this PR's own event.4. No
TEMPORARY:commit anywhere in this branch's history or diff — checked both.5.
AdrIndexTests— all 8 pass locally against this branch; the index lists 0109 once, correctly, no dropped or duplicated row.Two things to fix before merge, both factual and both cheap:
container-images, and ADR-0109's Context section, both say the failures were "never close to either the 10s or the 30s timeout the daemon probe uses." Checked the file's entire history:timeout 30 docker versionandtimeout 30 docker infoare the only bounded timeouts that have ever been in this job — there is no 10s anywhere, past or present. It doesn't change the "fails fast, not a hang" conclusion (8s and 24s are still well under the real 30s), but a fabricated number cited as part of the evidence for that conclusion, repeated in two places, needs fixing to "30s" (or whatever it's meant to say) rather than merging as-is..md/.ymlfile, no lingering 0107 reference anywhere in the tree); this is Forgejo-side PR text the renumbering commit didn't update.Verdict: mergeable
Checked directly against the file, not just the diff:
timeout 30is genuinely the only bound anywhere in.forgejo/workflows/ci.yml(grepped for10s/10-second/10 secondacross the whole file — no hits). The job-header comment and ADR-0109's Context both now say so explicitly, including naming their own earlier mistake rather than silently fixing it ("an earlier version of this comment invented one" / "no shorter bound anywhere in this file... an earlier version of this comment invented one").Independently re-derived the two failure durations from raw job data (not the summarised view, which mismatched fields on a first pass) rather than trusting the citation: job id 611 (head_sha
28f459f,2026-08-11T14:19:29Z→14:19:53Z) is 24 seconds; job id 621 (head_sha7917886,14:50:11Z→14:50:19Z) is 8 seconds. Both match ADR-0109's own figures exactly, and both are well under the real 30-second bound. No other invented number found in the workflow, the ADR, or the PR body.Conclusion hasn't crept: ADR-0109 still says the exact cause "could not be confirmed" and lists the three candidates without picking one. That holds even with the weaker margin the correction leaves (24s of 30s is a real fraction of the bound) — the total job duration includes checkout and setup steps before any
dockerinvocation runs, so "near-instant" is still the right characterisation of the probe itself, not just asserted past what the timing supports.PR body:
ADR-0109throughout, noADR-0107remnant; states 30 seconds, not 10.AdrIndexTests(8/8) pass against this branch's regenerated index.One thing to flag, not block on:
mainhas moved again sinceb2fab1f's own merge (#123 landed after). Forgejo reportsmergeable: truefor the current diff, which I didn't second-guess with a manual trial merge — but it's worth a lastgit status-equivalent check immediately before merging, given how fastmainis moving today.