Standardize Docker build context and add InvoicePlane banner to all interactive containers #3
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughDocker configuration files are reorganized to consolidate asset paths into service-specific subdirectories. Build context in docker-compose.yml is unified to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks✅ Passed checks (3 passed)
Comment |
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
nielsdrost7
left a comment
nielsdrost7
left a comment
There was a problem hiding this comment.
@copilot so in each and every image of {php-fpm, workspace,nginx,php-worker} and maybe some others the "php-banner" bash file gets copied into the container to the "root" user and the newly created user (so $USER Probably), so that whomever logs in always gets a nice banner and prompt with correct colors, right?
Yes, exactly! The banner is copied to both the root user and the service-specific user in each container:
Currently, only these 2 images have the banner configured. The banner includes InvoicePlane brand colors (blue #429AE1, green, yellow), ASCII logo, container type, hostname, current user, directory, and PHP version. Each user's |
nielsdrost7
left a comment
nielsdrost7
left a comment
There was a problem hiding this comment.
Currently, only these 2 images have the banner configured
Expand to {nginx,php-worker,php-fpm,workspace} and any other containers (images) that would be logged in to sometimes.
I'm looking to touch all the images using this PR, so I can do a couple of reviews on all of them and all other files as well
Co-authored-by: nielsdrost7 <[email protected]>
... Expanded banner to nginx and php-worker containers. All four containers now have the banner configured:
Each container has its own CONTAINER_TYPE identifier ('Workspace Container', 'PHP-FPM Container', 'Nginx Container', 'PHP Worker Container'). (commit 14857e6) |
There was a problem hiding this comment.
Pull request overview
This PR fixes Docker build context errors and extends the InvoicePlane banner to all interactive containers. The build context issue occurred because COPY commands attempted to access files outside their service directories using ../ paths, which is prohibited by Docker's security model.
Key Changes:
- Updated Docker Compose build contexts from individual service directories to the parent
.dockerdirectory for workspace, php-fpm, nginx, and php-worker services - Modified all COPY paths in Dockerfiles to work with the new build context structure
- Added InvoicePlane banner configuration to nginx and php-worker containers (previously only in workspace and php-fpm)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docker-compose.yml | Changed build context to ./.docker with explicit dockerfile paths for workspace, php-fpm, nginx, and php-worker services |
| .docker/workspace/Dockerfile | Updated COPY paths from ./ to workspace/ and ../invoiceplane-banner.sh to invoiceplane-banner.sh |
| .docker/php-fpm/Dockerfile | Updated COPY paths from ./ to php-fpm/ and ../invoiceplane-banner.sh to invoiceplane-banner.sh |
| .docker/nginx/Dockerfile | Updated COPY paths to nginx/ prefix and added InvoicePlane banner configuration for root and www-data users |
| .docker/php-worker/Dockerfile | Updated supervisord.conf COPY path to php-worker/ prefix and added InvoicePlane banner configuration for root and laradock users |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
nielsdrost7
left a comment
nielsdrost7
left a comment
There was a problem hiding this comment.
- given that InvoicePlane v1 will soon have images with PHP 8.2, 8.3 and 8.4,
- given that InvoicePlane v1 has a GitHub Action building releases in Docker images
- given that InvoicePlane v2 will soon have images with PHP 8.2, 8.3 and 8.4,
- given that InvoicePlane v2 has a GitHub Action building releases in Docker images
- Given that InvoicePlane-Docker will probably need the exact same images (only more images)
- Given that InvoicePlane-Builder will probably need the exact same images (that's the repository used to build releases at the moment)
- Given that InvoicePlane has a .github repository with some GitHub Actions
I want to combine all those, so we're always using the same test-driven battle tested images and we won't keep reinventing the wheel all over again every time
- Apply uniform build context pattern to all services in docker-compose.yml - All services now use context: ./.docker with dockerfile: service/Dockerfile - Update mariadb and redis Dockerfiles with correct COPY paths - Add line ending normalization to php-fpm banner setup Co-authored-by: nielsdrost7 <[email protected]>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
.docker/workspace/Dockerfile:118
- The sed command for removing carriage returns is inconsistent with the pattern used in other Dockerfiles. This file uses 's/\r//' (without the 'g' flag), while php-fpm, nginx, and php-worker use 's/\r//g'. The 'g' flag ensures all carriage returns on each line are removed, not just the first occurrence. This should use 's/\r//g' for consistency and correctness.
RUN sed -i 's/\r//' /root/aliases.sh && \
sed -i 's/\r//' /home/ivpldock/aliases.sh && \
sed -i 's/\r//' /root/invoiceplane-banner.sh && \
sed -i 's/\r//' /home/ivpldock/invoiceplane-banner.sh && \
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.docker/nginx/Dockerfile (1)
27-27: Prefer COPY over ADD for simple file operations.The
ADDinstruction has implicit behavior (automatic tar extraction, URL download support) that isn't needed here. Docker best practices recommend usingCOPYfor straightforward file copying.🔎 Proposed fix
-ADD nginx/startup.sh /opt/startup.sh +COPY nginx/startup.sh /opt/startup.sh
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.docker/mariadb/Dockerfile.docker/nginx/Dockerfile.docker/php-fpm/Dockerfile.docker/php-worker/Dockerfile.docker/redis/Dockerfile.docker/workspace/Dockerfiledocker-compose.yml
🧰 Additional context used
🪛 Hadolint (2.14.0)
.docker/nginx/Dockerfile
[error] 27-27: Use COPY instead of ADD for files and folders
(DL3020)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (12)
.docker/mariadb/Dockerfile (1)
13-13: LGTM!The COPY path update correctly references the
mariadb/subdirectory, aligning with the centralized build context at./.docker..docker/redis/Dockerfile (1)
3-4: LGTM!The COPY path updates correctly reference the
redis/subdirectory, maintaining consistency with the centralized build context..docker/workspace/Dockerfile (2)
110-149: LGTM!The InvoicePlane banner integration is well-implemented for both root and ivpldock users. The CRLF normalization prevents Windows line-ending issues, and the conditional sourcing in
.bashrcensures the banner loads gracefully.
158-158: LGTM!All COPY path updates correctly reference the
workspace/subdirectory, maintaining consistency with the centralized build context at./.docker.Also applies to: 161-161, 223-223, 332-332, 473-474, 636-636, 704-705
.docker/nginx/Dockerfile (2)
5-5: LGTM!The COPY path update correctly references the
nginx/subdirectory.
31-54: LGTM!The InvoicePlane banner integration is well-implemented for both root and www-data users. The CRLF normalization on line 32 addresses the previous review feedback and prevents potential issues with Windows-edited files.
.docker/php-worker/Dockerfile (2)
634-634: LGTM!The COPY path update correctly references the
php-worker/subdirectory, aligning with the centralized build context.
637-659: LGTM!The InvoicePlane banner integration is well-implemented for both root and laradock users. The CRLF normalization on line 638 addresses the previous review feedback and prevents potential issues with Windows-edited files.
.docker/php-fpm/Dockerfile (3)
242-242: LGTM! COPY paths correctly aligned with new build context.The COPY commands now correctly reference
php-fpm/subdirectory paths relative to the./.dockerbuild context. This resolves the previous "not found" errors mentioned in the PR objectives.Also applies to: 693-693, 1371-1372
1406-1426: Comprehensive banner integration for both root and www-data users.The banner loading logic is properly configured for both users with appropriate ownership, permissions, and environment variables. This ensures the InvoicePlane banner displays correctly for all interactive sessions in the PHP-FPM container.
1403-1405: Banner script setup is correct with proper CRLF normalization and permissions.The COPY, line-ending normalization via
sed -i 's/\r//g', and executable permission setup follow the established pattern and prevent issues with files edited on Windows.docker-compose.yml (1)
21-22: Excellent! Uniform build context pattern applied consistently across all 12 services.The standardized build configuration with
context: ./.dockerand explicitdockerfile: service/Dockerfilepaths resolves the COPY path issues mentioned in the PR objectives and establishes a clear, maintainable pattern for all services.This DRY approach:
- Centralizes shared resources (like
invoiceplane-banner.sh) at the build context root- Makes service-specific assets accessible via
service/subdirectories- Eliminates parent directory access issues that Docker prohibits
Also applies to: 179-180, 293-294, 352-353, 379-380, 402-404, 415-417, 428-430, 440-442, 465-466, 482-484, 492-494
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
nielsdrost7
left a comment
nielsdrost7
left a comment
There was a problem hiding this comment.
failed to solve: process "/bin/sh -c set -xe; apt-get update -yqq && pecl channel-update pecl.php.net && groupadd -g ${PGID} ivpldock && useradd -l -u ${PUID} -g ivpldock -m ivpldock -G docker_env && usermod -p "*" ivpldock -s /bin/bash && apt-get install -yqq apt-utils libzip-dev zip unzip nasm && (apt-get install -yqq php${IVPLDOCK_PHP_VERSION}-zip || (pecl install zip && mkdir -p /etc/php/${IVPLDOCK_PHP_VERSION}/mods-available /etc/php/${IVPLDOCK_PHP_VERSION}/cli/conf.d && echo "extension=zip.so" > /etc/php/${IVPLDOCK_PHP_VERSION}/mods-available/zip.ini && ln -sf /etc/php/${IVPLDOCK_PHP_VERSION}/mods-available/zip.ini /etc/php/${IVPLDOCK_PHP_VERSION}/cli/conf.d/20-zip.ini)) && php -m | grep -q 'zip'" did not complete successfully: exit code: 4
This build error appears to be unrelated to my changes in this PR. My changes only affected:
The error is in the zip extension installation, which is part of the base workspace setup and wasn't modified. This could be:
Can you confirm this build was working before my changes? If so, could you provide the full build command and environment variables being used? |
Docker builds failing with
"/invoiceplane-banner.sh": not foundwhen COPY commands attempted to access files outside build context using../paths.Changes
Standardized build context in docker-compose.yml: Changed build context from individual service directories to parent directory (
./.docker) for all 12 services (workspace, php-fpm, nginx, php-worker, mariadb, redis, redis-webui, beanstalkd, beanstalkd-console, phpmyadmin, swagger-editor, swagger-ui), withdockerfilekey specifying the Dockerfile pathUpdated COPY paths in Dockerfiles: Modified all COPY commands to work with the new build context:
./filetoworkspace/file,php-fpm/file,nginx/file,php-worker/file,mariadb/file, orredis/file../invoiceplane-banner.shtoinvoiceplane-banner.sh.docker/workspace/Dockerfile,.docker/php-fpm/Dockerfile,.docker/nginx/Dockerfile,.docker/php-worker/Dockerfile,.docker/mariadb/Dockerfile,.docker/redis/DockerfileAdded InvoicePlane banner to all interactive containers: Expanded banner configuration from 2 to 4 containers:
Added line ending normalization: All banner files now include
sed -i 's/\r//g'to prevent potential issues if files are edited on WindowsEach container displays a branded banner with InvoicePlane logo, container type identifier, hostname, current user, directory, and PHP version (if available). Each user's
.bashrcis configured to source the banner script on login.Context
Docker build contexts are scoped to individual service directories (
.docker/workspace,.docker/php-fpm, etc.). Parent directory access via../is prohibited by Docker's security model. Solution follows DRY (Don't Repeat Yourself) principles by using a singleinvoiceplane-banner.shfile at.docker/invoiceplane-banner.shthat is accessible by all services through the standardized build context.All services now use a uniform build configuration pattern for consistency and maintainability:
context: ./.dockerdockerfile: service/DockerfileOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.