test(ci): add ci/container/test-container-run.sh and run it per container runtime - #11461
Draft
basvandijk wants to merge 5 commits into
Draft
test(ci): add ci/container/test-container-run.sh and run it per container runtime#11461basvandijk wants to merge 5 commits into
basvandijk wants to merge 5 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The concurrency test can falsely pass when Bazel fails in the worktree container.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds integration coverage for container-run.sh across Podman and Docker while preserving supplementary container groups.
Changes:
- Preserves supplementary groups when selecting the container user.
- Adds worktree, Bazel isolation, and
build-ic.shintegration tests. - Runs tests for both container runtimes in CI and documents usage.
File summaries
| File | Description |
|---|---|
ci/container/test-container-run.sh |
Adds integration tests. |
ci/container/README.md |
Documents the test script. |
ci/container/container-run.sh |
Preserves supplementary groups. |
.github/workflows/ci-main.yml |
Adds Podman and Docker test jobs. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
basvandijk
force-pushed
the
bas/test-container-run
branch
from
September 5, 2026 16:14
d096520 to
10de995
Compare
Base automatically changed from
bas/container-run-mount-checkout-at-host-path
to
master
September 7, 2026 14:49
…h finds its marker /home/ubuntu is mode 750 in the base image and buildifier is a member of group ubuntu precisely so that it can read the /home/ubuntu/.ic-build-container marker that build-ic.sh tests to detect that it already runs inside the container. The explicit `--user "$CTR_USER:$CTR_USER"` form makes both podman and docker drop the user's supplementary groups, so on uid-1001 hosts (GitHub runners, namespace.so devboxes) build-ic.sh could not see the marker, re-executed container-run.sh inside the container and failed on its nesting guard. Pass the user by name only; the primary group still comes from /etc/passwd. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
An integration test for container-run.sh that can run locally and on CI. It starts containers from the checkout and from a temporary linked worktree next to it and asserts that the checkout is mounted at its host path and is the working directory, that nothing is mounted at /ic and no BAZELRC is injected, that git works in both checkouts with gc.worktreePruneExpire=never, that bazel's default output base is md5(host path) and differs per checkout, that the main checkout's bazel server survives a bazel run from the other checkout, and that build-ic.sh drops into the ic-build container from both a repo-relative and an absolute host path. All containers use a throwaway cache directory so the developer's real output bases and running servers are never touched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The test-container-run job only started a container without asserting anything, and never exercised the docker code path of container-run.sh. Run ci/container/test-container-run.sh once with podman and once with docker instead. The crun-pinning workaround is podman-only: container-run.sh forwards PODMAN_RUN_USR_ARGS to `docker run` as well, where `--runtime /usr/local/bin/crun` is not a valid runtime. The job's own workflow file joins the paths filter so that edits to the job run it on the PR instead of only on master. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…oncurrency check The worktree container's command ended with `touch`, so a failed or timed-out `bazel version` still exited 0 and the concurrency check could pass without bazel ever running in the second checkout. Propagate bazel's exit status and signal the waiting main container from the host regardless of the result. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
basvandijk
force-pushed
the
bas/test-container-run
branch
from
September 7, 2026 14:49
c8c2c60 to
5dde06a
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
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.
Stacked on #11460 (mount the checkout at its host path); review that one first.
Change
fix(ci):--user "$CTR_USER"instead of--user "$CTR_USER:$CTR_USER"./home/ubuntuis mode 750 in the base image andbuildifieris in groupubuntuso that it can read the/home/ubuntu/.ic-build-containermarker thatbuild-ic.shtests to detect that it already runs inside the container. The explicituser:groupform makes both podman and docker drop supplementary groups (verified with podman 4.9.3 and docker 29.5.2), so on uid-1001 hosts (GitHub runners, namespace.so devboxes)build-ic.shre-executedcontainer-run.shinside the container and failed on its nesting guard. The primary group still comes from/etc/passwd.test(ci):ci/container/test-container-run.sh. Runs on the host, locally or on CI, with podman (default) orCONTAINER_RUNTIME=docker. It starts containers from the checkout and from a temporary linked worktree next to it and asserts: cwd is the host path, nothing at/ic,BAZELRCunset, checkout owned by the host uid, git works in both checkouts (toplevel, common dir, HEAD,status,gc.worktreePruneExpire=never, the other worktree listed as prunable,workspace_status.sh --stamp), bazel's default output base ismd5(host path)and differs per checkout, no rc file is written to the cache dir, the main checkout's bazel server keeps its pid while the worktree container runs bazel (the fix(ci): give each checkout its own bazel output base in container-run.sh #11441 regression), andbuild-ic.sh --helpdrops into the ic-build container from a repo-relative and from an absolute host path. All containers use a throwaway cache directory, so real output bases and running servers are never touched; a dirty tree is fine. About 30 s locally with warm images.ci:test-container-runruns the script once per runtime (podman,docker) withfail-fast: false. The crun-pinning workaround is gated onmatrix.runtime == 'podman'becausecontainer-run.shforwardsPODMAN_RUN_USR_ARGStodocker runtoo..github/workflows/ci-main.ymljoins the job's paths filter so edits to the job run it on the PR. The oldTestCase 1/2legs (start a container, assert nothing) are replaced; no ruleset names them.Verified on a devenv: the script passes under podman and docker from the main checkout, and
idinside a container now lists the user's supplementary groups.