test: phase 2 CI exercise - #30
Open
rfay wants to merge 20 commits into
Open
Conversation
|
Download the artifacts for this pull request:
See Testing a PR.
|
|
Pushed updated container image(s) for this PR:
|
|
Pushed updated container image(s) for this PR:
|
…ware CI/Buildkite, for ddev#8609 Phase 2 of ddev#8609: CI now detects a changed container image, waits for a maintainer's approval, and builds/pushes it automatically for both maintainer and fork PRs, without ever exposing the DockerHub push token to a job that runs fork-supplied build content. Buildkite no longer races the push. - Phase 2 of ddev#8609 Phase 1 (ddev#8612) gave every image a content-addressed tag and made `make` auto-build changed images locally. The CI/registry side was untouched: a maintainer still had to notice a container changed and manually run `push-tagged-image.yml`/`push-tagged-dbimage.yml`, and fork contributors couldn't get an image change pushed without a maintainer doing it by hand. Two new workflows split build from push so a push secret never shares a job with untrusted (fork) build content: - `image-build-push.yml` (untrusted side, no secrets anywhere): a `detect` job recomputes each image's real content hash and checks the registry directly (never trusting the tag string committed in `versionconstants.go`, so a fork can't forge it); an `approval` job gates on the new `image-push` GitHub Environment before any expensive/untrusted build work runs; a `build` job builds per-arch using the same local-build Makefile targets Phase 1's `autotag-images` already uses, then uploads a `docker save` tarball plus tag metadata as an artifact. - `image-push.yml` (trusted side): triggered by `workflow_run` once the build workflow completes, so it always runs the default-branch version of itself and never checks out or executes the PR's code. It downloads the artifact, re-validates the tag against a strict format + reserved-tag blocklist (`containers/validate-image-tag.sh`), loads and pushes each per-arch image, assembles the multi-arch manifest, and comments on the PR. New scripts: `containers/registry-tag-exists.sh` (registry existence check, the same `docker buildx imagetools inspect` idiom already used in the post-push wait-loop) and `containers/validate-image-tag.sh`, both with bash test harnesses matching `containers/autotag_test.sh`'s style. Buildkite gets a `.buildkite/wait-for-images.sh` self-guarding step (wired into `test.sh`/`perf.sh`) that polls the registry for the tags a checkout actually needs before pulling anything, instead of racing the push - implementing the "lighter" option from the issue's open Buildkite decision. The existing `push-tagged-image.yml`/`push-tagged-dbimage.yml` are untouched and remain for manual re-pushes and the 18 `ddev-dbserver` variants Phase 1 doesn't auto-build. Requires one-time setup on the test repo (see `release-management.md`'s new "One-time setup" section): create a GitHub Environment named `image-push` with required reviewers, and add `PUSH_SERVICE_ACCOUNT_TOKEN` as a secret scoped to that environment. 1. Run the new unit tests directly: `containers/registry_tag_exists_test.sh` and `containers/validate_image_tag_test.sh` (no Docker daemon or network needed). 2. On a test repo (`image-push.yml` must be on its default branch - `workflow_run` triggers only fire for the default-branch copy of the listening workflow), push a trivial change to `containers/ddev-xhgui/` on a branch and open a PR. Confirm: `detect` flags it, `approval` blocks `build` until a reviewer approves, `build` produces artifacts with no secrets referenced in that job, `image-push` fires on completion (its own approval), loads and pushes the image, and comments on the PR. 3. Negative test: hand-edit `versionconstants.go` on that branch to a bogus `latest-<fakehash>`-shaped tag and confirm `detect` ignores it (recomputes the real hash from content) and that `containers/validate-image-tag.sh` rejects a manufactured `latest`/`vX.Y.Z` tag directly. Two new bash test harnesses (`containers/registry_tag_exists_test.sh`, `containers/validate_image_tag_test.sh`) stub `docker`/use pure string checks, no daemon or network required, wired into `container-tests.yml`'s existing unit-test job alongside `autotag_test.sh`. No Go code changed. No behavior change for anyone not touching `containers/`. Requires one-time manual GitHub Environment setup (documented) before the automatic push path is live on a given repo; until then `detect`/`approval`/`build` still run harmlessly (approval job would just wait indefinitely with no reviewers configured). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
…ix, for ddev#8609 The automatic build/push flow (and Phase 1's autotag-images before it) only auto-builds the default ddev-dbserver variant (mariadb_11.8). Tests exercising other db types/versions (TestDdevAllDatabases and similar) still need a manual push. Confirmed via a live test run on ddev-test/ddev that this is working as designed, not a bug - recording it as a follow-up to revisit, likely by moving full-matrix builds later in the flow rather than the pre-approval detect/build stage, since building all ~19 variants on every PR would be expensive. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
…recation warning Caught via a live test run on ddev-test/ddev: image-push.yml's use of actions/download-artifact@v4 still targets Node 20 internally, which GitHub now flags as deprecated and force-runs on Node 24 anyway. v8 uses Node 24 natively with the same github-token/run-id/pattern/path inputs, so this is a straight version bump, not a behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
…t, for ddev#8609 Caught via a live test run on ddev-test/ddev: unlike push-tagged-image.yml and push-tagged-dbimage.yml, image-push.yml wasn't cleaning up the intermediary <tag>-amd64/<tag>-arm64 tags after assembling the multi-arch manifest, leaving them on DockerHub permanently. Adds the same JWT-token DELETE cleanup those workflows already do, right after each docker buildx imagetools create call. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
rfay
force-pushed
the
20260814_rfay_test_phase2_xhgui_change
branch
from
August 15, 2026 00:44
dab39d6 to
698a0eb
Compare
|
Pushed updated container image(s) for this PR:
|
|
Pushed updated container image(s) for this PR:
|
…sh race Buildkite has, for ddev#8609 ## Short Summary (TL;DR) test-reusable.yml and test-wsl2-reusable.yml pull pinned images the same way Buildkite does - a fresh runner never builds a changed image itself, so they can race image-push.yml's approval/build/push exactly like Buildkite could. Adds the same wait-for-images.sh guard there too, and moves the script out of .buildkite/ since it's no longer Buildkite-specific. ## The Issue Related to ddev#8609 (phase 2). Confirmed live on ddev-test/ddev PR #30: a `containers/ddev-xhgui` change passed "Test Nginx-FPM" regardless of push status only because that test never pulls ddev-xhgui (it's gated behind XHProf mode). If the changed image had been ddev-webserver (used by nearly every test), or a test that does exercise xhgui had run, it would have raced the push and failed on the pull - `autotag.sh`'s no-op fast path trusts the committed tag without verifying the image exists anywhere, and a brand-new GitHub-hosted runner has no local build to fall back on. ## How This PR Solves The Issue - Moves `wait-for-images.sh` from `.buildkite/` to `containers/`, since both Buildkite and the GitHub-hosted test workflows need it now. - Adds a "Wait for pushed images" step to `test-reusable.yml`, right after the public-variables fetch and before anything Docker-dependent. - Adds the equivalent call to `.github/workflows/wsl2-test.sh` (the script `test-wsl2-reusable.yml` runs inside the WSL2 guest), right after Docker is confirmed ready and before `make` builds the binary. - Fixes a latent bug the new test caught: `tag="$(tag_for "$tag_var")"` silently killed the whole script under `set -e -o pipefail` when a tag var was missing from `versionconstants.go`, before ever reaching the intended "could not find..." error message. Same pattern exists in Phase 1's `autotag.sh` (`EXISTING_TAG="$(grep ... | sed ...)"`) - confirmed it has the identical silent-death bug, but left it alone since it's already-shipped code and this PR's scope is the wait-for-images guard; flagging separately. - Adds `containers/wait_for_images_test.sh` (same bash-harness style as `autotag_test.sh`), covering the fast path, eventual recovery within budget, giving up after exhausting the budget, and the missing-tag-var error path. Wired into `container-tests.yml`. ## Manual Testing Instructions Run `containers/wait_for_images_test.sh` directly (stubs `docker`/`sleep`, no daemon or network). For the real path: open a PR that changes an image test-reusable.yml/test-wsl2-reusable.yml actually depend on (e.g. `containers/ddev-webserver`) before approving the corresponding `image-push` run, and confirm the "Wait for pushed images" step polls rather than failing outright, then succeeds once the push lands. ## Automated Testing Overview New `containers/wait_for_images_test.sh`, run directly and via `container-tests.yml`'s unit-test job alongside the other containers/ bash tests. ## Release/Deployment Notes No behavior change when nothing changed under `containers/` (fast path, single registry check per image, no wait). Only affects PRs where an auto-managed image's tag doesn't yet exist in the registry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
…est workflows The previous commit only staged the .buildkite -> containers rename (a mis-staged `git add` silently dropped everything else). This commit carries the actual content: the new "Wait for pushed images" steps in test-reusable.yml and wsl2-test.sh, the wait-for-images.sh silent-exit fix, the new containers/wait_for_images_test.sh, and its wiring into container-tests.yml. See the previous commit's message for the full rationale. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
… for ddev#8609 ## Short Summary (TL;DR) A push to `main` or a same-repo PR (no fork involved) no longer needs any manual approval to build/push a changed container image - only actual fork PRs still go through the two-approval fork-safe flow. Reported as awkward friction on #30, where a maintainer-pushed, non-fork PR still required two approval clicks. ## The Issue Related to ddev#8609 (phase 2). The `approval` gate (before `build`) and `image-push.yml`'s own approval exist purely because of the fork threat model: GitHub won't hand secrets to a job running a fork's Dockerfile, and even if it would, you don't want to. Neither concern applies to a `push` event (only write-access collaborators can push branches to the base repo) or a same-repo PR (head and base are the same repo, exactly as trusted as `main-build.yml`, which already uses this same secret unguarded). Gating those cases behind an approval was unjustified friction, not a security requirement. ## How This PR Solves The Issue `detect` now also computes `is_fork` using the same fork-check idiom already used in `push-tagged-image.yml`/`push-tagged-dbimage.yml`/`test-pull-push-providers.yml` (`github.event.pull_request.head.repo.owner.login != github.repository_owner`, false for any non-`pull_request` event). Jobs branch on it: - `is_fork == 'true'`: unchanged `approval` -> `build` (no secrets, artifact hand-off) -> `image-push.yml` (its own approval) flow. - `is_fork == 'false'`: new `build-and-push` job builds and pushes directly per arch, then `create-manifests` assembles the multi-arch manifest, cleans up the intermediary per-arch tags, and comments on the PR if there is one. Neither job declares `environment: image-push`, so they read `PUSH_SERVICE_ACCOUNT_TOKEN` as a plain repository secret with no approval gate - the same access level `main-build.yml` already has. Updated `building-contributing.md`/`release-management.md` to describe the fork-vs-non-fork split instead of a blanket "one approval click." ## Manual Testing Instructions Replayed the `is_fork` bash logic directly against `pull_request`-from-fork, `pull_request`-same-repo, and `push` event shapes - resolves to `true`/`false`/`false` respectively. Replayed `create-manifests`' push/imagetools-create/cleanup logic against a stubbed `docker`/`curl` - confirms both `ddev-webserver` and `ddev-webserver-prod` get manifests created and per-arch tags cleaned up. On `ddev-test/ddev`: push directly to a branch (non-fork) with a container change and confirm no approval prompt appears at all before the image is pushed. ## Automated Testing Overview No new bash test files (this is workflow-YAML branching); verified via direct bash replay of the fork-detection and manifest-creation logic (see above), plus `make staticrequired`. ## Release/Deployment Notes Reduces friction for maintainer/same-repo workflows; fork PRs are unaffected and keep the full two-approval flow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
…ovals, for ddev#8609 ## Short Summary (TL;DR) The pre-build and pre-push approvals both showed up as "image-push" in GitHub's approval prompt, since that's the environment name it surfaces, not the job name - making it hard to tell which one a reviewer was being asked about. The pre-build gate now uses its own `image-build-approval` environment. ## The Issue Related to ddev#8609 (phase 2). Reported after testing on ddev-test/ddev: "Review pending deployments" only distinguishes by environment name, and both approval points shared `environment: image-push`, so approvers had to dig into which workflow/job they were actually looking at. ## How This PR Solves The Issue The `approval` job in image-build-push.yml (fork-PR pre-build gate) now uses `environment: image-build-approval` instead of `image-push`. It needs no secret - only `image-push.yml`'s `push` job still needs the DockerHub token, so that's the only one that keeps `environment: image-push`. Sharpened both jobs' `name:` fields ("Approve: build this fork PR's Dockerfile(s)" / "Approve: push the built image(s) to DockerHub") for the same reason. Updated release-management.md's setup checklist to create both environments, and noted that referencing an environment that doesn't exist yet auto-creates it with no protection rules - so it's important to verify each one actually has a `required_reviewers` rule (e.g. via `gh api .../environments/<name>`) before relying on it. ## Manual Testing Instructions Open a fork PR that changes a container image and confirm the two approval prompts now name different environments (`image-build-approval` then `image-push`). Requires creating the new `image-build-approval` GitHub Environment (with required reviewers, no secret) on the test/target repo first - until then this gate would silently not gate at all. ## Automated Testing Overview No behavioral logic changed (only environment names/job names), verified via `yaml.safe_load` that both jobs' `environment`/`name` fields resolve as intended, plus the existing containers/*_test.sh suite and `make staticrequired`. ## Release/Deployment Notes Requires the one-time creation of the `image-build-approval` GitHub Environment before this lands, on both ddev-test/ddev and ddev/ddev, mirroring image-push's required reviewers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
…pproval, for ddev#8609 [skip ci] Fork PRs now only need one approval (before push), not two. The `build` job never holds registry credentials, so gating it before it runs wasn't protecting a secret - it was only a compute/abuse control, and that's judged not worth the extra friction here. Related to ddev#8609 (phase 2). Following on from the previous commit (separate `image-build-approval`/`image-push` environments so the two prompts were distinguishable): on reflection, the pre-build `approval` job never had anything to protect - `permissions: contents: read`, no secrets referenced at all. Its only justification was resource/abuse control on untrusted compute (mirroring GitHub's first-time-contributor hold) - a real but lower-severity concern than the actual registry-mutation approval, and one this project is choosing to accept in exchange for one fewer manual click. Removes the `approval` job and its `image-build-approval` environment entirely. `build` now depends only on `detect` and runs immediately for fork PRs (still with zero secrets). The push-side approval on `image-push`'s environment, in the separate trusted `image-push.yml` workflow, is unchanged - it's still the only real gate. Updated `building-contributing.md`/`release-management.md` to describe the single-approval flow and drop the now-unneeded `image-build-approval` environment setup step (it was never actually created on either repo, so nothing to clean up there). Open a fork PR that changes a container image and confirm `build` starts immediately (no waiting job before it), and the only approval prompt appears before `image-push.yml`'s push step. No behavioral logic changed beyond removing a job; verified via `yaml.safe_load` that `build`'s `needs`/`environment` fields are as intended, plus the existing containers/*_test.sh suite and `make staticrequired`. Simplifies the fork-PR flow to one approval. The `image-push` environment/secret setup from previous commits is unchanged and still required. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
rfay
force-pushed
the
20260814_rfay_test_phase2_xhgui_change
branch
from
August 15, 2026 02:03
dfd0689 to
bf9d6d2
Compare
…letions, for ddev#8609 [skip ci] ## Short Summary (TL;DR) A maintainer-pushed (non-fork) PR still triggered the image-push approval prompt, even though that path already builds and pushes directly with no gate. image-push.yml's `push` job had no fork check of its own, so it asked for approval on every completion of "Image build", fork or not. ## The Issue Observed live on ddev-test/ddev: a PR pushed by a maintainer (not a fork) still required approval before "Image push" could run, contradicting the intended design (docs already say non-fork changes need zero approval clicks). ## How This PR Solves The Issue image-push.yml is triggered by `workflow_run` on completion of "Image build" - which runs for every PR/push, not just fork ones. Only the fork path (`build` job) uploads artifacts for it to find; the non-fork path (`build-and-push`/`create-manifests`) already pushed directly in the other workflow. But the `push` job's `environment: image-push` gate had no condition tied to fork status, so GitHub created an approval request before the job's steps (including the "no artifacts" fallback) ever ran. Added a job-level `if:` comparing `workflow_run.head_repository.full_name` to `workflow_run.repository.full_name` - the standard fork signal for `workflow_run` events - so the job (and its environment gate) is skipped entirely for non-fork completions. ## Manual Testing Instructions Push a container change directly on ddev-test/ddev (no fork) and confirm no approval prompt appears anywhere in the run. Open a fork PR with a container change and confirm the push-side approval still appears as before. ## Automated Testing Overview YAML-only change; verified with `python3 -c "import yaml; yaml.safe_load(...)"` and `make staticrequired`. ## Release/Deployment Notes No behavior change for forks. Removes an unnecessary/unintended approval prompt for maintainer-pushed changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
…ers, for ddev#8609 [skip ci] ## Short Summary (TL;DR) wait-for-images.sh checked ddev/<image> instead of the repo's actual DOCKER_ORG, so on ddev-test/ddev it polled a registry the image was never pushed to and timed out even after the real push succeeded. ## The Issue Observed live on ddev-test/ddev (PR #30): "Wait for pushed images" polled `ddev/ddev-webserver:<tag>` for 20 attempts and gave up, even though the image had already landed at `ddevhq/ddev-webserver:<tag>`. test-reusable.yml and test-wsl2-reusable.yml never exported DOCKER_ORG, so the script's `DOCKER_ORG="${DOCKER_ORG:-ddev}"` fallback silently used the wrong org on any repo where vars.DOCKER_ORG isn't "ddev". ## How This PR Solves The Issue Added `DOCKER_ORG: ${{ vars.DOCKER_ORG }}` to test-reusable.yml's job-level env (same pattern main-build.yml already uses), and to test-wsl2-reusable.yml's job-level env plus its `wsl -u testuser` export list, since that job crosses into a WSL2 shell that doesn't inherit GitHub Actions env directly. Buildkite's test.sh/perf.sh are unchanged - they only ever target the real ddev/ddev registry and have no vars.DOCKER_ORG equivalent to read. ## Manual Testing Instructions Re-run a GitHub-hosted test job on a ddev-test/ddev PR with a pushed container change and confirm "Wait for pushed images" checks ddevhq/... and passes on the first attempt once the image is up. ## Automated Testing Overview YAML-only change; verified with `python3 -c "import yaml; yaml.safe_load(...)"` on both files. ## Release/Deployment Notes No effect on ddev/ddev, where vars.DOCKER_ORG is already "ddev". Fixes the check for any repo (like ddev-test/ddev) using a different org. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
rfay
force-pushed
the
20260814_rfay_test_phase2_xhgui_change
branch
from
August 15, 2026 02:16
bf9d6d2 to
68c919a
Compare
… [skip ci] ## Short Summary (TL;DR) The old default (20 attempts x 15s = 5 minutes) was already shorter than ddev-webserver's ~6-8 minute build time alone, before counting a fork PR's approval-click delay or the push/manifest steps afterward. Bumped to 40 x 30s (~20 minutes). ## The Issue Racing test jobs (test-reusable.yml, test-wsl2-reusable.yml, Buildkite test.sh/perf.sh) call wait-for-images.sh before pulling any DDEV image. Its old default budget was too short to reliably outlast a real ddev-webserver build, so it would likely give up before the image ever became available - not just in the slow "waiting on maintainer approval" case, but even in the ordinary same-repo build-and-push path with no human delay at all. ## How This PR Solves The Issue Raised WAIT_FOR_IMAGES_ATTEMPTS/WAIT_FOR_IMAGES_SLEEP defaults from 20/15 to 40/30 in containers/wait-for-images.sh, giving ~20 minutes of headroom. Callers can still override both via env if needed; none of the existing callers (test-reusable.yml, test-wsl2-reusable.yml, .buildkite/test.sh, .buildkite/perf.sh) set these explicitly, so they all pick up the new defaults. ## Manual Testing Instructions None beyond the existing unit test - this only changes two default numbers. ## Automated Testing Overview containers/wait_for_images_test.sh already overrides both env vars in every scenario it exercises, so it's unaffected; ran it directly to confirm. ## Release/Deployment Notes Racing GitHub-hosted/Buildkite jobs will wait longer (up to ~20 min instead of ~5) before giving up on a missing image, trading a bit of idle runner time for far fewer spurious "gave up waiting" failures on the common slow-build path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
|
Pushed updated container image(s) for this PR:
|
|
Pushed updated container image(s) for this PR:
|
…ionconstants.go, for ddev#8609 [skip ci] ## Short Summary (TL;DR) wait-for-images.sh read its expected tag straight from versionconstants.go, but that file's committed tag only has its hash kept current locally - the branch prefix can be stale from whenever that image last actually changed. It now recomputes the tag from real content the same way image-build-push.yml's detect job does, so it always checks the tag CI actually built. ## The Issue Observed live on ddev-test/ddev (PR #30): a GitHub-hosted test job waited the full retry budget and gave up on `ddevhq/ddev-traefik-router:main-c96123b524`, even though the image had been pushed - just under `ddevhq/ddev-traefik-router:<current-branch>-c96123b524`. The hash matched; only the branch prefix was wrong. autotag.sh only rewrites versionconstants.go's committed tag string when the *hash* changes (containers/autotag.sh:73-79), so an image untouched since some earlier branch keeps that branch's name in its committed tag indefinitely - a stale value the rest of this design already explicitly refuses to trust (detect never reads it either). ## How This PR Solves The Issue wait-for-images.sh now takes a required WAIT_FOR_IMAGES_BRANCH and recomputes each image's tag via hash-paths.sh, using the same repo_suffix/hash-paths list and `<branch>-<hash>` formula as image-build-push.yml's detect job - never reading versionconstants.go at all. Threaded WAIT_FOR_IMAGES_BRANCH through every caller: test-reusable.yml and test-wsl2-reusable.yml set it to `github.head_ref || github.ref_name` (matching detect exactly), and .buildkite/test.sh/perf.sh set it to `$BUILDKITE_BRANCH`. Rewrote wait_for_images_test.sh to compute expected tags via the real hash-paths.sh against this checkout's actual content instead of a fabricated versionconstants.go fixture. ## Manual Testing Instructions Re-run a GitHub-hosted or Buildkite test job on a ddev-test/ddev PR with a pushed container change and confirm "Wait for pushed images" checks `<branch>-<hash>` and passes without timing out. ## Automated Testing Overview containers/wait_for_images_test.sh rewritten and passing (fast path, delayed-recovery, give-up, and a new required-WAIT_FOR_IMAGES_BRANCH check). Ran `make staticrequired` clean. ## Release/Deployment Notes No effect on the actual build/push decision (detect's logic is unchanged) - only fixes what the downstream wait check looks for. Every caller of wait-for-images.sh must now set WAIT_FOR_IMAGES_BRANCH; all current callers do. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
…se 2 CI on ddev-test/ddev Comment-only Dockerfile edit so `containers/ddev-xhgui`'s content hash changes, exercising the new detect -> approval -> build -> push flow end-to-end. `make` already regenerated XhguiTag locally to match. Not intended to land anywhere but ddev-test/ddev. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg New commit should only push one image add actual change to xhgui
rfay
force-pushed
the
20260814_rfay_test_phase2_xhgui_change
branch
from
August 15, 2026 02:34
68c919a to
0555c66
Compare
rfay
added a commit
that referenced
this pull request
Aug 16, 2026
…sh race Buildkite has, for ddev#8609 ## Short Summary (TL;DR) test-reusable.yml and test-wsl2-reusable.yml pull pinned images the same way Buildkite does - a fresh runner never builds a changed image itself, so they can race image-push.yml's approval/build/push exactly like Buildkite could. Adds the same wait-for-images.sh guard there too, and moves the script out of .buildkite/ since it's no longer Buildkite-specific. ## The Issue Related to ddev#8609 (phase 2). Confirmed live on ddev-test/ddev PR #30: a `containers/ddev-xhgui` change passed "Test Nginx-FPM" regardless of push status only because that test never pulls ddev-xhgui (it's gated behind XHProf mode). If the changed image had been ddev-webserver (used by nearly every test), or a test that does exercise xhgui had run, it would have raced the push and failed on the pull - `autotag.sh`'s no-op fast path trusts the committed tag without verifying the image exists anywhere, and a brand-new GitHub-hosted runner has no local build to fall back on. ## How This PR Solves The Issue - Moves `wait-for-images.sh` from `.buildkite/` to `containers/`, since both Buildkite and the GitHub-hosted test workflows need it now. - Adds a "Wait for pushed images" step to `test-reusable.yml`, right after the public-variables fetch and before anything Docker-dependent. - Adds the equivalent call to `.github/workflows/wsl2-test.sh` (the script `test-wsl2-reusable.yml` runs inside the WSL2 guest), right after Docker is confirmed ready and before `make` builds the binary. - Fixes a latent bug the new test caught: `tag="$(tag_for "$tag_var")"` silently killed the whole script under `set -e -o pipefail` when a tag var was missing from `versionconstants.go`, before ever reaching the intended "could not find..." error message. Same pattern exists in Phase 1's `autotag.sh` (`EXISTING_TAG="$(grep ... | sed ...)"`) - confirmed it has the identical silent-death bug, but left it alone since it's already-shipped code and this PR's scope is the wait-for-images guard; flagging separately. - Adds `containers/wait_for_images_test.sh` (same bash-harness style as `autotag_test.sh`), covering the fast path, eventual recovery within budget, giving up after exhausting the budget, and the missing-tag-var error path. Wired into `container-tests.yml`. ## Manual Testing Instructions Run `containers/wait_for_images_test.sh` directly (stubs `docker`/`sleep`, no daemon or network). For the real path: open a PR that changes an image test-reusable.yml/test-wsl2-reusable.yml actually depend on (e.g. `containers/ddev-webserver`) before approving the corresponding `image-push` run, and confirm the "Wait for pushed images" step polls rather than failing outright, then succeeds once the push lands. ## Automated Testing Overview New `containers/wait_for_images_test.sh`, run directly and via `container-tests.yml`'s unit-test job alongside the other containers/ bash tests. ## Release/Deployment Notes No behavior change when nothing changed under `containers/` (fast path, single registry check per image, no wait). Only affects PRs where an auto-managed image's tag doesn't yet exist in the registry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
rfay
added a commit
that referenced
this pull request
Aug 16, 2026
… for ddev#8609 ## Short Summary (TL;DR) A push to `main` or a same-repo PR (no fork involved) no longer needs any manual approval to build/push a changed container image - only actual fork PRs still go through the two-approval fork-safe flow. Reported as awkward friction on #30, where a maintainer-pushed, non-fork PR still required two approval clicks. ## The Issue Related to ddev#8609 (phase 2). The `approval` gate (before `build`) and `image-push.yml`'s own approval exist purely because of the fork threat model: GitHub won't hand secrets to a job running a fork's Dockerfile, and even if it would, you don't want to. Neither concern applies to a `push` event (only write-access collaborators can push branches to the base repo) or a same-repo PR (head and base are the same repo, exactly as trusted as `main-build.yml`, which already uses this same secret unguarded). Gating those cases behind an approval was unjustified friction, not a security requirement. ## How This PR Solves The Issue `detect` now also computes `is_fork` using the same fork-check idiom already used in `push-tagged-image.yml`/`push-tagged-dbimage.yml`/`test-pull-push-providers.yml` (`github.event.pull_request.head.repo.owner.login != github.repository_owner`, false for any non-`pull_request` event). Jobs branch on it: - `is_fork == 'true'`: unchanged `approval` -> `build` (no secrets, artifact hand-off) -> `image-push.yml` (its own approval) flow. - `is_fork == 'false'`: new `build-and-push` job builds and pushes directly per arch, then `create-manifests` assembles the multi-arch manifest, cleans up the intermediary per-arch tags, and comments on the PR if there is one. Neither job declares `environment: image-push`, so they read `PUSH_SERVICE_ACCOUNT_TOKEN` as a plain repository secret with no approval gate - the same access level `main-build.yml` already has. Updated `building-contributing.md`/`release-management.md` to describe the fork-vs-non-fork split instead of a blanket "one approval click." ## Manual Testing Instructions Replayed the `is_fork` bash logic directly against `pull_request`-from-fork, `pull_request`-same-repo, and `push` event shapes - resolves to `true`/`false`/`false` respectively. Replayed `create-manifests`' push/imagetools-create/cleanup logic against a stubbed `docker`/`curl` - confirms both `ddev-webserver` and `ddev-webserver-prod` get manifests created and per-arch tags cleaned up. On `ddev-test/ddev`: push directly to a branch (non-fork) with a container change and confirm no approval prompt appears at all before the image is pushed. ## Automated Testing Overview No new bash test files (this is workflow-YAML branching); verified via direct bash replay of the fork-detection and manifest-creation logic (see above), plus `make staticrequired`. ## Release/Deployment Notes Reduces friction for maintainer/same-repo workflows; fork PRs are unaffected and keep the full two-approval flow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
rfay
added a commit
that referenced
this pull request
Aug 16, 2026
…ers, for ddev#8609 [skip ci] ## Short Summary (TL;DR) wait-for-images.sh checked ddev/<image> instead of the repo's actual DOCKER_ORG, so on ddev-test/ddev it polled a registry the image was never pushed to and timed out even after the real push succeeded. ## The Issue Observed live on ddev-test/ddev (PR #30): "Wait for pushed images" polled `ddev/ddev-webserver:<tag>` for 20 attempts and gave up, even though the image had already landed at `ddevhq/ddev-webserver:<tag>`. test-reusable.yml and test-wsl2-reusable.yml never exported DOCKER_ORG, so the script's `DOCKER_ORG="${DOCKER_ORG:-ddev}"` fallback silently used the wrong org on any repo where vars.DOCKER_ORG isn't "ddev". ## How This PR Solves The Issue Added `DOCKER_ORG: ${{ vars.DOCKER_ORG }}` to test-reusable.yml's job-level env (same pattern main-build.yml already uses), and to test-wsl2-reusable.yml's job-level env plus its `wsl -u testuser` export list, since that job crosses into a WSL2 shell that doesn't inherit GitHub Actions env directly. Buildkite's test.sh/perf.sh are unchanged - they only ever target the real ddev/ddev registry and have no vars.DOCKER_ORG equivalent to read. ## Manual Testing Instructions Re-run a GitHub-hosted test job on a ddev-test/ddev PR with a pushed container change and confirm "Wait for pushed images" checks ddevhq/... and passes on the first attempt once the image is up. ## Automated Testing Overview YAML-only change; verified with `python3 -c "import yaml; yaml.safe_load(...)"` on both files. ## Release/Deployment Notes No effect on ddev/ddev, where vars.DOCKER_ORG is already "ddev". Fixes the check for any repo (like ddev-test/ddev) using a different org. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
rfay
added a commit
that referenced
this pull request
Aug 16, 2026
…ionconstants.go, for ddev#8609 [skip ci] ## Short Summary (TL;DR) wait-for-images.sh read its expected tag straight from versionconstants.go, but that file's committed tag only has its hash kept current locally - the branch prefix can be stale from whenever that image last actually changed. It now recomputes the tag from real content the same way image-build-push.yml's detect job does, so it always checks the tag CI actually built. ## The Issue Observed live on ddev-test/ddev (PR #30): a GitHub-hosted test job waited the full retry budget and gave up on `ddevhq/ddev-traefik-router:main-c96123b524`, even though the image had been pushed - just under `ddevhq/ddev-traefik-router:<current-branch>-c96123b524`. The hash matched; only the branch prefix was wrong. autotag.sh only rewrites versionconstants.go's committed tag string when the *hash* changes (containers/autotag.sh:73-79), so an image untouched since some earlier branch keeps that branch's name in its committed tag indefinitely - a stale value the rest of this design already explicitly refuses to trust (detect never reads it either). ## How This PR Solves The Issue wait-for-images.sh now takes a required WAIT_FOR_IMAGES_BRANCH and recomputes each image's tag via hash-paths.sh, using the same repo_suffix/hash-paths list and `<branch>-<hash>` formula as image-build-push.yml's detect job - never reading versionconstants.go at all. Threaded WAIT_FOR_IMAGES_BRANCH through every caller: test-reusable.yml and test-wsl2-reusable.yml set it to `github.head_ref || github.ref_name` (matching detect exactly), and .buildkite/test.sh/perf.sh set it to `$BUILDKITE_BRANCH`. Rewrote wait_for_images_test.sh to compute expected tags via the real hash-paths.sh against this checkout's actual content instead of a fabricated versionconstants.go fixture. ## Manual Testing Instructions Re-run a GitHub-hosted or Buildkite test job on a ddev-test/ddev PR with a pushed container change and confirm "Wait for pushed images" checks `<branch>-<hash>` and passes without timing out. ## Automated Testing Overview containers/wait_for_images_test.sh rewritten and passing (fast path, delayed-recovery, give-up, and a new required-WAIT_FOR_IMAGES_BRANCH check). Ran `make staticrequired` clean. ## Release/Deployment Notes No effect on the actual build/push decision (detect's logic is unchanged) - only fixes what the downstream wait check looks for. Every caller of wait-for-images.sh must now set WAIT_FOR_IMAGES_BRANCH; all current callers do. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
rfay
added a commit
that referenced
this pull request
Aug 17, 2026
…sh race Buildkite has, for ddev#8609 ## Short Summary (TL;DR) test-reusable.yml and test-wsl2-reusable.yml pull pinned images the same way Buildkite does - a fresh runner never builds a changed image itself, so they can race image-push.yml's approval/build/push exactly like Buildkite could. Adds the same wait-for-images.sh guard there too, and moves the script out of .buildkite/ since it's no longer Buildkite-specific. ## The Issue Related to ddev#8609 (phase 2). Confirmed live on ddev-test/ddev PR #30: a `containers/ddev-xhgui` change passed "Test Nginx-FPM" regardless of push status only because that test never pulls ddev-xhgui (it's gated behind XHProf mode). If the changed image had been ddev-webserver (used by nearly every test), or a test that does exercise xhgui had run, it would have raced the push and failed on the pull - `autotag.sh`'s no-op fast path trusts the committed tag without verifying the image exists anywhere, and a brand-new GitHub-hosted runner has no local build to fall back on. ## How This PR Solves The Issue - Moves `wait-for-images.sh` from `.buildkite/` to `containers/`, since both Buildkite and the GitHub-hosted test workflows need it now. - Adds a "Wait for pushed images" step to `test-reusable.yml`, right after the public-variables fetch and before anything Docker-dependent. - Adds the equivalent call to `.github/workflows/wsl2-test.sh` (the script `test-wsl2-reusable.yml` runs inside the WSL2 guest), right after Docker is confirmed ready and before `make` builds the binary. - Fixes a latent bug the new test caught: `tag="$(tag_for "$tag_var")"` silently killed the whole script under `set -e -o pipefail` when a tag var was missing from `versionconstants.go`, before ever reaching the intended "could not find..." error message. Same pattern exists in Phase 1's `autotag.sh` (`EXISTING_TAG="$(grep ... | sed ...)"`) - confirmed it has the identical silent-death bug, but left it alone since it's already-shipped code and this PR's scope is the wait-for-images guard; flagging separately. - Adds `containers/wait_for_images_test.sh` (same bash-harness style as `autotag_test.sh`), covering the fast path, eventual recovery within budget, giving up after exhausting the budget, and the missing-tag-var error path. Wired into `container-tests.yml`. ## Manual Testing Instructions Run `containers/wait_for_images_test.sh` directly (stubs `docker`/`sleep`, no daemon or network). For the real path: open a PR that changes an image test-reusable.yml/test-wsl2-reusable.yml actually depend on (e.g. `containers/ddev-webserver`) before approving the corresponding `image-push` run, and confirm the "Wait for pushed images" step polls rather than failing outright, then succeeds once the push lands. ## Automated Testing Overview New `containers/wait_for_images_test.sh`, run directly and via `container-tests.yml`'s unit-test job alongside the other containers/ bash tests. ## Release/Deployment Notes No behavior change when nothing changed under `containers/` (fast path, single registry check per image, no wait). Only affects PRs where an auto-managed image's tag doesn't yet exist in the registry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
rfay
added a commit
that referenced
this pull request
Aug 17, 2026
… for ddev#8609 ## Short Summary (TL;DR) A push to `main` or a same-repo PR (no fork involved) no longer needs any manual approval to build/push a changed container image - only actual fork PRs still go through the two-approval fork-safe flow. Reported as awkward friction on #30, where a maintainer-pushed, non-fork PR still required two approval clicks. ## The Issue Related to ddev#8609 (phase 2). The `approval` gate (before `build`) and `image-push.yml`'s own approval exist purely because of the fork threat model: GitHub won't hand secrets to a job running a fork's Dockerfile, and even if it would, you don't want to. Neither concern applies to a `push` event (only write-access collaborators can push branches to the base repo) or a same-repo PR (head and base are the same repo, exactly as trusted as `main-build.yml`, which already uses this same secret unguarded). Gating those cases behind an approval was unjustified friction, not a security requirement. ## How This PR Solves The Issue `detect` now also computes `is_fork` using the same fork-check idiom already used in `push-tagged-image.yml`/`push-tagged-dbimage.yml`/`test-pull-push-providers.yml` (`github.event.pull_request.head.repo.owner.login != github.repository_owner`, false for any non-`pull_request` event). Jobs branch on it: - `is_fork == 'true'`: unchanged `approval` -> `build` (no secrets, artifact hand-off) -> `image-push.yml` (its own approval) flow. - `is_fork == 'false'`: new `build-and-push` job builds and pushes directly per arch, then `create-manifests` assembles the multi-arch manifest, cleans up the intermediary per-arch tags, and comments on the PR if there is one. Neither job declares `environment: image-push`, so they read `PUSH_SERVICE_ACCOUNT_TOKEN` as a plain repository secret with no approval gate - the same access level `main-build.yml` already has. Updated `building-contributing.md`/`release-management.md` to describe the fork-vs-non-fork split instead of a blanket "one approval click." ## Manual Testing Instructions Replayed the `is_fork` bash logic directly against `pull_request`-from-fork, `pull_request`-same-repo, and `push` event shapes - resolves to `true`/`false`/`false` respectively. Replayed `create-manifests`' push/imagetools-create/cleanup logic against a stubbed `docker`/`curl` - confirms both `ddev-webserver` and `ddev-webserver-prod` get manifests created and per-arch tags cleaned up. On `ddev-test/ddev`: push directly to a branch (non-fork) with a container change and confirm no approval prompt appears at all before the image is pushed. ## Automated Testing Overview No new bash test files (this is workflow-YAML branching); verified via direct bash replay of the fork-detection and manifest-creation logic (see above), plus `make staticrequired`. ## Release/Deployment Notes Reduces friction for maintainer/same-repo workflows; fork PRs are unaffected and keep the full two-approval flow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
rfay
added a commit
that referenced
this pull request
Aug 17, 2026
…ers, for ddev#8609 [skip ci] ## Short Summary (TL;DR) wait-for-images.sh checked ddev/<image> instead of the repo's actual DOCKER_ORG, so on ddev-test/ddev it polled a registry the image was never pushed to and timed out even after the real push succeeded. ## The Issue Observed live on ddev-test/ddev (PR #30): "Wait for pushed images" polled `ddev/ddev-webserver:<tag>` for 20 attempts and gave up, even though the image had already landed at `ddevhq/ddev-webserver:<tag>`. test-reusable.yml and test-wsl2-reusable.yml never exported DOCKER_ORG, so the script's `DOCKER_ORG="${DOCKER_ORG:-ddev}"` fallback silently used the wrong org on any repo where vars.DOCKER_ORG isn't "ddev". ## How This PR Solves The Issue Added `DOCKER_ORG: ${{ vars.DOCKER_ORG }}` to test-reusable.yml's job-level env (same pattern main-build.yml already uses), and to test-wsl2-reusable.yml's job-level env plus its `wsl -u testuser` export list, since that job crosses into a WSL2 shell that doesn't inherit GitHub Actions env directly. Buildkite's test.sh/perf.sh are unchanged - they only ever target the real ddev/ddev registry and have no vars.DOCKER_ORG equivalent to read. ## Manual Testing Instructions Re-run a GitHub-hosted test job on a ddev-test/ddev PR with a pushed container change and confirm "Wait for pushed images" checks ddevhq/... and passes on the first attempt once the image is up. ## Automated Testing Overview YAML-only change; verified with `python3 -c "import yaml; yaml.safe_load(...)"` on both files. ## Release/Deployment Notes No effect on ddev/ddev, where vars.DOCKER_ORG is already "ddev". Fixes the check for any repo (like ddev-test/ddev) using a different org. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
rfay
added a commit
that referenced
this pull request
Aug 17, 2026
…ionconstants.go, for ddev#8609 [skip ci] ## Short Summary (TL;DR) wait-for-images.sh read its expected tag straight from versionconstants.go, but that file's committed tag only has its hash kept current locally - the branch prefix can be stale from whenever that image last actually changed. It now recomputes the tag from real content the same way image-build-push.yml's detect job does, so it always checks the tag CI actually built. ## The Issue Observed live on ddev-test/ddev (PR #30): a GitHub-hosted test job waited the full retry budget and gave up on `ddevhq/ddev-traefik-router:main-c96123b524`, even though the image had been pushed - just under `ddevhq/ddev-traefik-router:<current-branch>-c96123b524`. The hash matched; only the branch prefix was wrong. autotag.sh only rewrites versionconstants.go's committed tag string when the *hash* changes (containers/autotag.sh:73-79), so an image untouched since some earlier branch keeps that branch's name in its committed tag indefinitely - a stale value the rest of this design already explicitly refuses to trust (detect never reads it either). ## How This PR Solves The Issue wait-for-images.sh now takes a required WAIT_FOR_IMAGES_BRANCH and recomputes each image's tag via hash-paths.sh, using the same repo_suffix/hash-paths list and `<branch>-<hash>` formula as image-build-push.yml's detect job - never reading versionconstants.go at all. Threaded WAIT_FOR_IMAGES_BRANCH through every caller: test-reusable.yml and test-wsl2-reusable.yml set it to `github.head_ref || github.ref_name` (matching detect exactly), and .buildkite/test.sh/perf.sh set it to `$BUILDKITE_BRANCH`. Rewrote wait_for_images_test.sh to compute expected tags via the real hash-paths.sh against this checkout's actual content instead of a fabricated versionconstants.go fixture. ## Manual Testing Instructions Re-run a GitHub-hosted or Buildkite test job on a ddev-test/ddev PR with a pushed container change and confirm "Wait for pushed images" checks `<branch>-<hash>` and passes without timing out. ## Automated Testing Overview containers/wait_for_images_test.sh rewritten and passing (fast path, delayed-recovery, give-up, and a new required-WAIT_FOR_IMAGES_BRANCH check). Ran `make staticrequired` clean. ## Release/Deployment Notes No effect on the actual build/push decision (detect's logic is unchanged) - only fixes what the downstream wait check looks for. Every caller of wait-for-images.sh must now set WAIT_FOR_IMAGES_BRANCH; all current callers do. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqmDPsdWxQjwGu3LFNzGLg
rfay
force-pushed
the
main
branch
2 times, most recently
from
August 24, 2026 21:28
aded40f to
3052ef8
Compare
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.
Trivial ddev-xhgui change to test ddev#8609 phase 2.