feat: complete the Docker setup — wire the compose stack, add a PHP 8.4 CLI image (IP-149)#601
Closed
nielsdrost7 wants to merge 2 commits into
Closed
feat: complete the Docker setup — wire the compose stack, add a PHP 8.4 CLI image (IP-149)#601nielsdrost7 wants to merge 2 commits into
nielsdrost7 wants to merge 2 commits into
Conversation
…lopment Brings in custom Dockerfiles for PHP-FPM, Apache, and Node from the original feature/docker-env branch, ported cleanly onto develop. - docker-resources/php-fpm/Dockerfile — PHP 8.4-fpm-alpine with all required extensions (pdo_mysql, gd, intl, redis, opcache, etc.) - docker-resources/apache/Dockerfile — Apache 2.4 with PHP-FPM proxy - docker-resources/apache/config/invoiceplane-vhost.conf - docker-resources/node/scripts/entrypoint.sh — Vite dev server with Docker-aware path and HMR configuration Refs #149 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The branch shipped apache and php-fpm Dockerfiles that docker-compose.yml
never started, and DOCKER.md described an app container that did not
exist. The compose file now runs web (apache, APP_PORT with 8080
default) -> app (php-fpm, matching the vhost's fcgi://app:9000) -> db,
plus mailcatcher.
Adds docker-resources/php-cli: a CLI twin of the php-fpm image (same
extension set: intl, gd, pdo_mysql, bcmath, zip, exif, soap, redis)
with Composer, a 1G memory limit for the test suite, and UID/GID build
args so files written into the mounted repo keep host ownership. It
runs behind a "tools" compose profile as a one-off runner:
docker compose run --rm cli vendor/bin/phpunit
The test suite runs on in-memory sqlite, so tests need no services at
all. DOCKER.md rewritten to match (correct v2 clone URL, services
table, no-host-PHP quick start, ownership troubleshooting).
Refs #149
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe Docker setup adds PHP CLI and FPM images, Apache PHP-FPM proxying, Docker-specific Vite startup, Compose services and networking, and updated documentation for startup, testing, ownership, commands, and troubleshooting. ChangesDocker development stack
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DockerCompose
participant web
participant app
participant db
DockerCompose->>web: start web service and expose APP_PORT
web->>app: proxy PHP requests to app:9000
app->>db: connect through laravel network
DockerCompose->>db: attach db to laravel network
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docker-compose.yml (1)
14-15: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a health-conditioned
depends_onfor thedbservice.The
appservice starts as soon as thedbcontainer launches, but MariaDB may not be ready to accept connections yet. Laravel retries, so this works in practice, but the app logs spurious connection errors during initial startup. Adding ahealthcheckondband usingcondition: service_healthywould provide a cleaner startup.Proposed change
depends_on: - - db + db: + condition: service_healthyAnd add a health check to the
dbservice (outside the changed range):db: ... healthcheck: test: ["CMD-SHELL", "mariadb-admin ping -h localhost -u root -p$$MYSQL_ROOT_PASSWORD || exit 1"] interval: 3s timeout: 3s retries: 10🤖 Prompt for 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. In `@docker-compose.yml` around lines 14 - 15, Update the app service’s depends_on configuration to use the db service with condition service_healthy, and add the specified healthcheck under the db service using mariadb-admin ping with the existing root password, 3-second interval and timeout, and 10 retries.docker-resources/php-cli/Dockerfile (1)
13-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtension installation block is duplicated across both PHP Dockerfiles. The build-dependency, runtime-dependency, PHP extension, and PECL redis installation steps are nearly identical in both images, creating a maintenance burden where any extension or package change must be applied in two places.
docker-resources/php-cli/Dockerfile#L13-L55: Extract the shared install steps into a common base image (e.g.,php:8.4-alpinewith extensions pre-installed) or a reusable shell script that both DockerfilesCOPYandRUN.docker-resources/php-fpm/Dockerfile#L6-L47: Same — consume the shared base image or script instead of maintaining a separate copy.🤖 Prompt for 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. In `@docker-resources/php-cli/Dockerfile` around lines 13 - 55, Deduplicate the shared PHP dependency and extension installation logic used by the Dockerfiles: in docker-resources/php-cli/Dockerfile lines 13-55 and docker-resources/php-fpm/Dockerfile lines 6-47, extract the build dependencies, runtime packages, PHP extensions, and PECL redis setup into a common base image or reusable script, then make both Dockerfiles consume it while preserving their image-specific steps.
🤖 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 `@docker-resources/node/scripts/entrypoint.sh`:
- Around line 1-18: Add `set -e` immediately after the shebang in the entrypoint
script so failures from `mkdir`, `cp`, or either `sed` command terminate
execution before reaching the npm install and Vite startup command.
In `@docker-resources/php-fpm/Dockerfile`:
- Line 3: Update the PHP-FPM Dockerfile user setup to declare and use
configurable UID and GID build arguments, matching the existing CLI Dockerfile
pattern instead of hardcoding dockeruser’s IDs. Update the app service in
docker-compose.yml to pass the host-aligned UID and GID build arguments through
to the FPM image.
---
Nitpick comments:
In `@docker-compose.yml`:
- Around line 14-15: Update the app service’s depends_on configuration to use
the db service with condition service_healthy, and add the specified healthcheck
under the db service using mariadb-admin ping with the existing root password,
3-second interval and timeout, and 10 retries.
In `@docker-resources/php-cli/Dockerfile`:
- Around line 13-55: Deduplicate the shared PHP dependency and extension
installation logic used by the Dockerfiles: in
docker-resources/php-cli/Dockerfile lines 13-55 and
docker-resources/php-fpm/Dockerfile lines 6-47, extract the build dependencies,
runtime packages, PHP extensions, and PECL redis setup into a common base image
or reusable script, then make both Dockerfiles consume it while preserving their
image-specific steps.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: be3f85f5-2a8b-4d92-8089-dcd455da8036
📒 Files selected for processing (7)
.github/DOCKER.mddocker-compose.ymldocker-resources/apache/Dockerfiledocker-resources/apache/config/invoiceplane-vhost.confdocker-resources/node/scripts/entrypoint.shdocker-resources/php-cli/Dockerfiledocker-resources/php-fpm/Dockerfile
Comment on lines
+1
to
+18
| #!/bin/sh | ||
|
|
||
| # Copy original vite.config.js to Docker-specific version | ||
| mkdir -p /app/.docker | ||
| cp /app/vite.config.js /app/.docker/vite.config.docker.js | ||
|
|
||
| # Update resource paths to absolute paths | ||
| sed -i "s|'resources/css/app.css'|'/app/resources/css/app.css'|g" /app/.docker/vite.config.docker.js | ||
| sed -i "s|'resources/js/app.js'|'/app/resources/js/app.js'|g" /app/.docker/vite.config.docker.js | ||
|
|
||
| # Add Docker-specific server configuration if not already present | ||
| if ! grep -q "server:" /app/.docker/vite.config.docker.js; then | ||
| # Insert server config before the closing }); of defineConfig | ||
| sed -i '/^});$/i\ server: {\n host: '\''0.0.0.0'\'',\n port: 5173,\n hmr: {\n host: '\''localhost'\'',\n },\n },' /app/.docker/vite.config.docker.js | ||
| fi | ||
|
|
||
| # Install dependencies and start Vite with Docker config | ||
| npm install && npm run dev -- --config .docker/vite.config.docker.js |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add set -e to fail fast on copy/sed errors.
Without set -e, if cp or sed fails (e.g., vite.config.js missing), the script continues to npm install && npm run dev, which fails with a confusing "config file not found" error instead of surfacing the original failure.
Proposed fix
#!/bin/sh
+set -e
+
# Copy original vite.config.js to Docker-specific version📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #!/bin/sh | |
| # Copy original vite.config.js to Docker-specific version | |
| mkdir -p /app/.docker | |
| cp /app/vite.config.js /app/.docker/vite.config.docker.js | |
| # Update resource paths to absolute paths | |
| sed -i "s|'resources/css/app.css'|'/app/resources/css/app.css'|g" /app/.docker/vite.config.docker.js | |
| sed -i "s|'resources/js/app.js'|'/app/resources/js/app.js'|g" /app/.docker/vite.config.docker.js | |
| # Add Docker-specific server configuration if not already present | |
| if ! grep -q "server:" /app/.docker/vite.config.docker.js; then | |
| # Insert server config before the closing }); of defineConfig | |
| sed -i '/^});$/i\ server: {\n host: '\''0.0.0.0'\'',\n port: 5173,\n hmr: {\n host: '\''localhost'\'',\n },\n },' /app/.docker/vite.config.docker.js | |
| fi | |
| # Install dependencies and start Vite with Docker config | |
| npm install && npm run dev -- --config .docker/vite.config.docker.js | |
| #!/bin/sh | |
| set -e | |
| # Copy original vite.config.js to Docker-specific version | |
| mkdir -p /app/.docker | |
| cp /app/vite.config.js /app/.docker/vite.config.docker.js | |
| # Update resource paths to absolute paths | |
| sed -i "s|'resources/css/app.css'|'/app/resources/css/app.css'|g" /app/.docker/vite.config.docker.js | |
| sed -i "s|'resources/js/app.js'|'/app/resources/js/app.js'|g" /app/.docker/vite.config.docker.js | |
| # Add Docker-specific server configuration if not already present | |
| if ! grep -q "server:" /app/.docker/vite.config.docker.js; then | |
| # Insert server config before the closing }); of defineConfig | |
| sed -i '/^});$/i\ server: {\n host: '\''0.0.0.0'\'',\n port: 5173,\n hmr: {\n host: '\''localhost'\'',\n },\n },' /app/.docker/vite.config.docker.js | |
| fi | |
| # Install dependencies and start Vite with Docker config | |
| npm install && npm run dev -- --config .docker/vite.config.docker.js |
🤖 Prompt for 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.
In `@docker-resources/node/scripts/entrypoint.sh` around lines 1 - 18, Add `set
-e` immediately after the shebang in the entrypoint script so failures from
`mkdir`, `cp`, or either `sed` command terminate execution before reaching the
npm install and Vite startup command.
| @@ -0,0 +1,58 @@ | |||
| FROM php:8.4-fpm-alpine | |||
|
|
|||
| RUN adduser -D -s /bin/bash dockeruser | |||
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
FPM container lacks configurable UID/GID.
The CLI Dockerfile accepts ARG UID / ARG GID so the container user can match the host user, but the FPM Dockerfile hardcodes adduser -D -s /bin/bash dockeruser (default UID 1000). The FPM process writes to storage/ and bootstrap/cache/ on the mounted volume; if the host user's UID differs, those files will be owned by UID 1000 and may be unwritable or unmanageable by the host user. Apply the same ARG UID / ARG GID pattern here, and pass build args from the app service in docker-compose.yml.
Proposed fix
+ARG UID=1000
+ARG GID=1000
+
+RUN addgroup -g ${GID} dockeruser \
+ && adduser -D -s /bin/bash -u ${UID} -G dockeruser dockeruser
-RUN adduser -D -s /bin/bash dockeruserAnd in docker-compose.yml:
app:
container_name: 'ivplflmnt_app'
build:
context: ./docker-resources/php-fpm
+ args:
+ UID: ${UID:-1000}
+ GID: ${GID:-1000}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| RUN adduser -D -s /bin/bash dockeruser | |
| ARG UID=1000 | |
| ARG GID=1000 | |
| RUN addgroup -g ${GID} dockeruser \ | |
| && adduser -D -s /bin/bash -u ${UID} -G dockeruser dockeruser |
🤖 Prompt for 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.
In `@docker-resources/php-fpm/Dockerfile` at line 3, Update the PHP-FPM Dockerfile
user setup to declare and use configurable UID and GID build arguments, matching
the existing CLI Dockerfile pattern instead of hardcoding dockeruser’s IDs.
Update the app service in docker-compose.yml to pass the host-aligned UID and
GID build arguments through to the FPM image.
Collaborator
Author
|
Recreated as #611 with the head branch on the fork (underdogg-forks) per the repo workflow — development on the fork, PRs land here. Same commits. |
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.
Completes the standalone Docker setup from
feature/149-docker-setup(issue #149) and adds the missing piece: a CLI image so users and contributors can run the test suite, artisan, and composer with no PHP on the host.What this adds on top of the existing branch
docker-resources/php-cli/Dockerfile— PHP 8.4 CLI (alpine), the exact extension twin of the existing php-fpm image (intl,gd,pdo_mysql,bcmath,zip,exif,soap,redis) plus Composer, a 1G memory limit (the full suite OOMs at the 128M default), and bundledpdo_sqlite— the test suite runs on in-memory sqlite, so tests need zero services.UID/GIDbuild args let Linux users match host ownership for files written into the mounted repo (vendor/, storage/) — this exact mismatch produces hundreds of confusingtempnam()test errors otherwise.docker-compose.ymlactually wires the images in. The branch shipped apache + php-fpm Dockerfiles but the compose file only starteddb+mailcatcher, and DOCKER.md referenced anappcontainer that didn't exist. Now:web(apache, port${APP_PORT:-8080}) →app(php-fpm, matching the vhost'sfcgi://app:9000) →db, plus the newcliservice behind atoolsprofile so it never auto-starts..github/DOCKER.mdrewritten to match reality — corrected clone URL (pointed at v1's repo), a services table, a no-host-PHP quick start, test-suite instructions, and the UID/GID troubleshooting entry.Why a CLI image at all
The vendor tree requires PHP ≥ 8.3 with
intl+gd; typical host PHPs miss at least one of those (Filament table rendering hard-requiresintl, dompdf wantsgd). This image is the supported, reproducible way to runvendor/bin/phpunit— validated by developing the Report Builder with a hand-rolled equivalent of exactly this image.Verified
docker compose configvalid;--profile toolsexposesclialongside the five servicesphp -mshows the full extension set;composer --versionworksdocker compose run --rm cli vendor/bin/phpunit(sqlite in-memory). Note: some company-panel tests fail on this branch's lineage because it predates the permission-seeding fixes ondevelop— pre-existing to this PR; the same command is fully green on branches that include those fixes.web+app) is config-validated against the existing vhost (fcgi://app:9000, docroothtdocs/public); it still needs a bootstrapped.env+ key to serve, per DOCKER.md's quick start.Closes #149
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation