WEB-11226: make php_tests_worktree zero-config against the running stack - #3
Conversation
The initial worktree-test target was a working skeleton but forced per-repo
compose edits and never provisioned the isolated database, so it couldn't run a
repo's suite for real. Rework it to reuse the already-running local stack with
zero changes in the consuming repo:
- Reuse the running cli container's exact compose files (from the
com.docker.compose.project.config_files label), image and services; redirect
ONLY the code bind-mount to the worktree via an auto-generated ephemeral
compose override. This removes the need for consumers to reference
${DKR_COMPOSE_SRC}, which is dropped from docker.mk and the docs.
- Pin the exact image the running cli container uses (docker inspect) instead of
falling back to a stale <name>/cli:latest build.
- Auto-detect the app directory (shallowest composer.json bind mount under the
checkout), fixing the vendor/composer.lock drift check for nested-app repos
such as api-backend (api/laravel).
- Create and grant the per-worktree database as root via the mysql container's
own MYSQL_ROOT_PASSWORD, then migrate it (DB_SEED=1 also seeds, DB_FRESH=1
rebuilds) so DatabaseTransactions feature tests work.
- Rewrite the CI test to assert both the make wiring and the rendered shell
program, without needing Docker.
Validated against api-backend with an unmodified checkout: unit and
DatabaseTransactions feature tests pass on the correct PHP 8.4 image in an
isolated, migrated+seeded database; re-runs reuse vendor and the database.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe old ChangesWorktree test target rework
May the Force guide this review, young reviewer. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@php.mk`:
- Around line 118-121: The DB_FRESH cleanup block in php.mk is swallowing DROP
DATABASE failures by redirecting output and forcing success, which hides real
drop errors. Update the DB_FRESH branch around the docker exec mysql DROP
DATABASE call so failures are allowed to surface instead of using a success
override, keeping the existing log message but letting the shell return a
non-zero status when the drop fails.
- Around line 64-67: The APP_DIR mount check in php.mk is comparing the raw
APP_DIR value against Docker mount sources, so relative values like . or paths
with trailing slashes can fail even when mounted. Normalize APP_DIR before
building app_host in the APP_DIR validation block, then use that normalized path
for the awk match so the existing app_target lookup in the shell logic accepts
equivalent paths.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22c3c2c1-e5b2-4071-b665-e2735aa873c0
📒 Files selected for processing (5)
docker.mkdocs/docker.mddocs/php.mdphp.mktest/php_tests_worktree.sh
💤 Files with no reviewable changes (2)
- docs/docker.md
- docker.mk
- DB_FRESH: surface a failed DROP DATABASE (die with a clear message) instead of `|| true`, which silently continued and could leave a non-fresh database. - APP_DIR: normalise a leading ./ and trailing slashes before matching the Docker mount source, so '.', 'api/laravel/' etc. resolve instead of spuriously failing the "not bind-mounted" check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
WEB-11226 added
php_tests_worktree(merged in #2) as a working skeleton, but it forcedper-repo compose edits and never provisioned the isolated database, so it couldn't run a
repo's suite for real. This reworks it to reuse the already-running local stack with zero
changes in the consuming repo: bring the stack up (
make dkr_up_local) and run from themain checkout pointing at a worktree — decrypted env/secrets (gitignored, absent in worktrees)
still come from the main stack.
What changed
${DKR_COMPOSE_SRC}compose editcom.docker.compose.project.config_fileslabel — soenv_file/secrets/network still come from main.DKR_COMPOSE_SRCremoved fromdocker.mk+ docs.<name>/cli:latestimagedocker inspect … {{.Image}}).composer.jsonbind mount under the checkout);vendor/composer.lockchecks use the real app dir (e.g.api/laravel), so the "reuse vendor" fast path is reachable.$MYSQL_ROOT_PASSWORD, then migrates it.DB_SEED=1also seeds;DB_FRESH=1rebuilds.Also rewrote the CI test (
test/php_tests_worktree.sh) to assert both themakewiring (make -n) and the rendered shell program — still Docker-free.Knobs (all auto-detected unless set)
APP_DIR·DB_DATABASE·DB_FRESH=1·DB_SEED=1— seedocs/php.md.Validation (api-backend, unmodified checkout)
QueueConnectionsTest→ OK, 2 testsDatabaseTransactions) —TrackElasticSearchIndexTransformerTest→ OK, 1 assertion, in the isolated migrated+seeded DBtest_wt_apibackend_…api/laravelauto-detected; re-runs reuse vendor + DB (Nothing to migrate); distinct worktree paths → distinct DBs.Notes for the reviewer / release
DKR_COMPOSE_SRCremoved. A grep of the locally checked-out repos found no consumer wiring${DKR_COMPOSE_SRC}into compose (the feature was never usable), so removal is safe — worth a wider confirmation before release.v1is a required follow-up. The floatingv1(andv1.14) still point at387cb34, pre-WEB-11226, sov1-pinned repos don't receive this. After merge, publish av1.15.0GitHub release to triggergit-release.yamland movev1/v1.15onto this commit.🤖 Generated with Claude Code