develop to master #14
Conversation
…lation Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
… add docker-exec helper Co-authored-by: nielsdrost7 <[email protected]>
Updated branches for push and pull_request events.
…banners Co-authored-by: nielsdrost7 <[email protected]>
…www-data banner, sort workflow Co-authored-by: nielsdrost7 <[email protected]>
… compatibility Co-authored-by: nielsdrost7 <[email protected]>
…use IPBLUE variable Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: Copilot <[email protected]>
- 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]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
- Make pecl channel-update non-blocking to handle network issues gracefully - Create docker_env group before adding user to it - Remove -q flag from grep to ensure proper output and exit code - Update MariaDB test to use 'mariadb' command instead of 'mysql' for v11.x Co-authored-by: nielsdrost7 <[email protected]>
- Make pecl channel-update non-blocking in php-fpm, php-worker, and laravel-horizon - Apply same fixes to legacy workspace Dockerfiles (7.4, 8.1) - Add docker_env group creation to legacy workspace Dockerfiles - Ensure all Dockerfiles handle network failures gracefully Co-authored-by: nielsdrost7 <[email protected]>
- Make groupadd for ivpldock group non-blocking (base image has GID 1000) - Remove push and pull_request triggers from workflows - Workflows now only run on workflow_dispatch (manual trigger) Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
The complex fallback logic with pecl was causing build failures for newer PHP versions. Simplified to directly install php-zip package, matching the working pattern from Dockerfile.74 and Dockerfile.81. Co-authored-by: nielsdrost7 <[email protected]>
The base laradock/workspace image already includes the zip extension for PHP 8.2, 8.3, and 8.4. Removed the problematic line that tried to install php${IVPLDOCK_PHP_VERSION}-zip package which doesn't exist in default repositories.
Also fixed user/group creation to handle existing ubuntu user in base image by renaming it to ivpldock instead of trying to create a new user with the same UID.
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Separate usermod operations to avoid potential issues with -m flag and improve clarity. The docker_env group membership is now only added in the usermod path where it's needed. Co-authored-by: nielsdrost7 <[email protected]>
Instead of removing the php-zip package installation entirely, wrap it in error handling (|| true) so that if the package is unavailable or fails to install, the build continues. This is more resilient as it will install the package when available while not failing when it's not. Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
…d using correct variable Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: Copilot <[email protected]>
Remove .env.test from the ignore list and add it back.
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Co-authored-by: nielsdrost7 <[email protected]>
Removed the mapping of the '/data/tenants' directory.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR overhauls the InvoicePlane Docker development environment by introducing a branded container banner system, making PECL installations resilient to failures, standardizing Docker build contexts, enhancing shell scripts with validation and error handling, adding multi-version CI/CD testing workflows, and providing comprehensive documentation updates and guidelines. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom Pre-merge Checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker-compose.yml (1)
271-271: Use environment variable for tenants path to match project conventions.The volume
/data/tenants:/tenantsuses an absolute hard-coded path, inconsistent with other paths in the file which use environment variables (e.g.,${APP_CODE_PATH_HOST},${DATA_PATH_HOST}). This path is undocumented and may not exist on all development machines, causing container startup failures.Replace with an environment variable like other configurable paths, or at minimum document this requirement in README/setup guide.
🤖 Fix all issues with AI agents
In @.docker/invoiceplane-banner.sh:
- Around line 36-40: The echo in display_banner prints a literal "\w" because
prompt escapes aren't expanded by echo—change the third echo line in
display_banner to use an expanded working directory (e.g., ${PWD} or $(pwd))
instead of "\w" and ensure proper quoting so the path prints correctly alongside
${host_name} and ${user_name}; also either remove the unused show_info_box()
function or integrate it by having display_banner call show_info_box() (or
replace the duplicated box code in display_banner with a call to
show_info_box()) so you don't keep an unused function.
In @.docker/workspace/Dockerfile:
- Around line 321-323: The RUN that installs Xdebug currently silences all
failures via "|| true" (the three chained commands in the RUN), which can hide a
failed Xdebug install; update that RUN to verify installation after the install
steps by running "php -m | grep -q xdebug" and emit a warning if it’s missing
(e.g., append a verification/echo warning step after the echo of zend_extension
into 20-xdebug.ini), so the build still doesn’t fail but surfaces a clear
message when xdebug is not present.
- Around line 546-552: The installation block for Python3 ends with a trailing
"|| true" which masks failures when INSTALL_PYTHON3=true; remove the outer "||
true" from the multi-command block so the shell returns a non-zero exit on real
failures, and instead apply "|| true" only to truly optional sub-commands inside
the block (e.g., individual non-critical steps like pecl installs or
architecture-dependent downloads) so that critical steps (apt-get install,
get-pip.py run, pip upgrade, virtualenv install) will fail-fast and surface
errors.
- Around line 57-59: The user creation command chain misuses the useradd/usermod
flags and can fail for existing ubuntu users with mismatched UID/GID; remove the
invalid useradd -l flag and simplify the fallback: try to rename and modify the
existing ubuntu user with usermod -l ivpldock, usermod -d /home/ivpldock -m
ivpldock, usermod -u ${PUID} -g ivpldock -aG docker_env ivpldock (ensuring group
ivpldock exists or is created with groupadd -g ${PGID} ivpldock), and if that
fails create the new user with useradd -u ${PUID} -g ivpldock -m -G docker_env
ivpldock; ensure groupadd docker_env || true remains and check/handle existing
ubuntu user UID/GID mismatches rather than passing an invalid flag.
- Around line 531-537: The Dockerfile currently installs legacy Python 2 using
curl bootstrap.pypa.io/pip/2.7/get-pip.py and silences failures with "|| true";
remove or replace this block: drop the Python 2 install commands (curl ...
get-pip.py, python -m pip for 2.7) and instead install/upgrade Python 3+ and
pip/virtualenv, or if Python 2 is absolutely required preserve the guarded
INSTALL_PYTHON check but remove the "|| true" fallback so the build fails on
error; specifically edit the if-block containing "INSTALL_PYTHON" and the
sequence "(apt-get -y install python python-dev ... && curl
https://bootstrap.pypa.io/pip/2.7/get-pip.py ... ) || true" to either remove the
pip/2.7 lines entirely or replace them with python3 equivalents and ensure
failures propagate (no "|| true").
In @.docker/workspace/Dockerfile.74:
- Around line 376-378: Remove the no-op sed invocation that replaces
"xdebug.remote_autostart=0" with itself; specifically delete the sed -i
"s/xdebug.remote_autostart=0/xdebug.remote_autostart=0/"
/etc/php/${IVPLDOCK_PHP_VERSION}/cli/conf.d/xdebug.ini so only the effective
command (sed -i "s/xdebug.remote_enable=0/xdebug.remote_enable=1/" ...) remains,
preserving shell continuation and escaping as needed.
In @.docker/workspace/Dockerfile.81:
- Around line 308-310: Remove the no-op sed invocation that replaces
"xdebug.remote_autostart=0" with itself; locate the sed command string sed -i
"s/xdebug.remote_autostart=0/xdebug.remote_autostart=0/" and delete that clause
so only the meaningful sed that sets xdebug.remote_enable to 1 remains (ensure
the surrounding shell chaining and the trailing ;fi remain correctly formed
after removal).
In @.env.example:
- Around line 40-42: The .env.example sets PHP_VERSION=8.4 but the expected
mount file ./ .docker/php-fpm/php8.4.ini is missing and the accepted-values
comment doesn't list 8.4; create a new php8.4.ini in .docker/php-fpm/ (copy and
adjust ./ .docker/php-fpm/php8.3.ini as a base), update the comment line in
.env.example to include " - 8.4" among accepted values, and confirm that the
Docker images referenced (laradock/workspace:latest-8.4 and
laradock/php-fpm:latest-8.4) provide required PHP extensions before merging.
In @.github/copilot-instructions.md:
- Around line 181-185: Clarify the guidance in the "Secrets Management" section
about `.env.docker`: explain whether `.env.docker` is intended as a committed
template (like `.env.example`) that should be tracked and documented for users
to copy/modify, or as a local secrets/config file that must be excluded via
`.gitignore`; update the bullet "Never commit `.env.docker` files" to state the
intended behavior and add explicit instructions for the chosen option (e.g.,
"Commit `.env.docker` as a template" or "Never commit `.env.docker`; add to
`.gitignore` and provide `.env.example` instead"), and ensure the change
references `.env.docker` and the "Secrets Management" heading so readers can
find the clarification.
In @.github/workflows/docker-build-test.yml:
- Around line 22-28: Update the three sed replacement commands in the "Create
.env file for testing" step so they match any current default values instead of
fixed ones: change the PHP replacement to match any PHP_VERSION value (use a
pattern like PHP_VERSION=.* and replace with the matrix value) and similarly
change the WORKSPACE_INSTALL_PYTHON3 and WORKSPACE_INSTALL_WKHTMLTOPDF
replacements to match their current values using .* (e.g.,
WORKSPACE_INSTALL_PYTHON3=.* and WORKSPACE_INSTALL_WKHTMLTOPDF=.*) so future
changes in .env.example won't break the replacements.
In @.github/workflows/test-containers.yml:
- Around line 262-268: The workflow steps "Verify Redis is running" and "Test
Redis operations" call redis-cli without authenticating; update those steps to
read the REDIS_PASSWORD from the .env.docker file and pass it to redis-cli (use
redis-cli -a "$REDIS_PASSWORD" or equivalent) for all commands (ping, set, get)
so the commands authenticate before executing; ensure the shell step sources or
exports REDIS_PASSWORD from .env.docker (or parses the file) before running
docker compose exec redis redis-cli with the -a argument.
In @docker-exec.sh:
- Around line 77-88: The script uses set -e so the docker exec line will cause
the script to exit immediately on failure and never reach the EXIT_CODE=$?
handling; to fix, temporarily disable errexit before running docker exec (or use
an if/then wrapper) so you can capture the command result: turn off errexit (set
+e) immediately before invoking docker exec (the command that uses CONTAINER_ID,
PROJECT_DIR, and COMMAND), capture its exit into EXIT_CODE=$?, then restore
errexit (set -e) and proceed with the existing success/failure branch that
checks EXIT_CODE.
🧹 Nitpick comments (15)
.docker/laravel-horizon/Dockerfile (1)
43-43: Consider adding logging for failed extension installations.While the
|| truepattern provides excellent build resilience, consider adding a logging mechanism to track when pecl installations fail. This would help with debugging and monitoring without blocking builds.Example approach for logging failures
# Example: Add logging wrapper for pecl install RUN set -eux; if [ ${INSTALL_REDIS} = true ]; then \ if ! (printf "\n" | pecl install -o -f redis); then \ echo "WARNING: Redis extension installation failed, continuing..." >&2; \ fi; \ docker-php-ext-enable redis; \ fiThis preserves the non-blocking behavior while providing visibility into installation failures.
Also applies to: 77-77, 122-122, 173-173, 175-175, 251-251, 254-254, 270-270, 272-272, 274-274, 292-292, 294-294
docker-exec.sh (1)
46-56: Container may be stopped but still matched.The
docker ps -aqfcommand with-aflag returns all containers (including stopped ones) matching the name filter. If a container exists but is stopped,CONTAINER_IDwill be set, butdocker execwill fail later.Consider using
docker ps -qf(without-a) to only match running containers, or explicitly check the container state:🔎 Proposed fix
# Get the container ID -CONTAINER_ID=$(docker ps -aqf "name=${CONTAINER_NAME}") +CONTAINER_ID=$(docker ps -qf "name=${CONTAINER_NAME}") if [ -z "$CONTAINER_ID" ]; then echo "Error: Container '${CONTAINER_NAME}' is not running!".docker/workspace/Dockerfile.81 (1)
320-321: Unguardedpecl install pcovmay break the build.Unlike other PECL installs in this file (xdebug, imagick, etc.) that are wrapped with
|| true, the pcov installation is not guarded. If the PECL install fails, the entire build will fail.For consistency with the resilience pattern used elsewhere:
🔎 Proposed fix
# Install pcov for code coverage -RUN pecl install pcov && \ +RUN (pecl install pcov || true) && \ echo "extension=pcov.so" >> /etc/php/${IVPLDOCK_PHP_VERSION}/cli/conf.d/20-pcov.iniworker.sh (1)
13-20: Consider more precise container name matching.The filter
name=workerwill match any container whose name contains "worker" (e.g., "worker-test", "php-worker-dev"). If multiple matches exist,docker ps -aqreturns multiple IDs, which could cause issues.🔎 More precise container matching
-CONTAINER_ID=$(docker ps -aqf "name=worker") +CONTAINER_ID=$(docker ps -aqf "name=^worker$")Or use docker-compose to resolve the exact container name:
-CONTAINER_ID=$(docker ps -aqf "name=worker") +CONTAINER_ID=$(docker-compose --env-file .env.docker ps -q php-worker)phpmeup.sh (1)
13-20: Consider more precise container name matching.The filter
name=php-fpmwill match any container whose name contains "php-fpm". If you have containers like "php-fpm-test" or multiple php-fpm instances, this could match unintended containers.🔎 More precise container matching
-CONTAINER_ID=$(docker ps -aqf "name=php-fpm") +CONTAINER_ID=$(docker ps -aqf "name=^php-fpm$")Or use docker-compose to resolve the exact container name:
-CONTAINER_ID=$(docker ps -aqf "name=php-fpm") +CONTAINER_ID=$(docker-compose --env-file .env.docker ps -q php-fpm).docker/nginx/Dockerfile (2)
30-41: Consider using heredoc for improved readability.The multiple
echostatements work correctly but could be more maintainable. A heredoc or a separate configuration file would reduce repetition and improve clarity.🔎 Alternative approach using cat with heredoc
# Add InvoicePlane banner to bash profile COPY invoiceplane-banner.sh /root/invoiceplane-banner.sh RUN sed -i 's/\r//g' /root/invoiceplane-banner.sh && \ chmod +x /root/invoiceplane-banner.sh && \ - echo "" >> /root/.bashrc && \ - echo "# Set container type for banner" >> /root/.bashrc && \ - echo "export CONTAINER_TYPE='Nginx Container'" >> /root/.bashrc && \ - echo "# Load InvoicePlane Banner" >> /root/.bashrc && \ - echo "if [ -f ~/invoiceplane-banner.sh ]; then" >> /root/.bashrc && \ - echo " source ~/invoiceplane-banner.sh" >> /root/.bashrc && \ - echo "fi" >> /root/.bashrc && \ + cat >> /root/.bashrc <<'EOF' + +# Set container type for banner +export CONTAINER_TYPE='Nginx Container' +# Load InvoicePlane Banner +if [ -f ~/invoiceplane-banner.sh ]; then + source ~/invoiceplane-banner.sh +fi +EOF # Also add banner for www-data user \ mkdir -p /home/www-data && \This approach also works for the www-data user configuration on lines 48-54.
41-54: Duplicate banner setup logic for www-data user.The banner configuration for
www-dataduplicates the pattern used forroot. While this works correctly, the duplication is intentional across multiple containers in this PR for consistency. The same heredoc refactor suggested for lines 30-41 could apply here..github/workflows/docker-build-test.yml (1)
49-68: LGTM! Verification steps validate core functionality and cleanup is guaranteed.The verification steps confirm PHP version and zip extension in both containers. The cleanup step with
if: always()ensures resources are freed even if tests fail.Consider adding timeout-minutes to verification steps to prevent potential hangs.
.github/workflows/test-containers.yml (2)
43-44: Sed pattern assumes specific version string in .env.example.The sed command
s/PHP_VERSION=8.1/...assumes.env.examplecontains exactlyPHP_VERSION=8.1. If this value changes in the example file, the substitution will silently fail and tests will run with the wrong PHP version.Consider a more robust pattern:
- sed -i "s/PHP_VERSION=8.1/PHP_VERSION=${{ matrix.php-version }}/" .env.docker + sed -i "s/^PHP_VERSION=.*/PHP_VERSION=${{ matrix.php-version }}/" .env.docker
223-224: Sleep-based waits are fragile and may cause flaky CI.Hardcoded
sleep 15andsleep 20commands for service readiness can lead to intermittent failures if services take longer to start, or unnecessary delays if they start faster.Consider using a wait-for-it pattern or health checks:
# For MariaDB - retry until connection succeeds timeout 60 bash -c 'until docker compose --env-file .env.docker exec -T mariadb mariadb -uroot -proot -e "SELECT 1;" 2>/dev/null; do sleep 2; done'Also applies to: 329-330
.docker/php-worker/Dockerfile (1)
44-51: Resilient PECL installs may mask critical extension failures.Wrapping
pecl installwith|| trueprevents build failures from optional extensions, but subsequentdocker-php-ext-enablecommands will fail if the extension wasn't actually installed. For example at lines 87-91:(pecl install gnupg || true); \ docker-php-ext-enable gnupg; \ # Will fail if pecl install actually failed php -m | grep -oiE '^gnupg$'; # This provides validationThe
php -m | grepcommands provide some validation, but they'll cause build failures after the|| truehas already hidden the root cause.Consider either:
- Grouping the install+enable+verify in the same tolerant block
- Using conditional checks:
if pecl install gnupg; then docker-php-ext-enable gnupg; fiThis is acceptable for optional extensions but be aware of the debugging implications.
Also applies to: 87-91, 163-178
README.md (2)
263-271: Script naming may cause confusion.The script names
starmeup.sh(foreground) andstartmeup.sh(background) differ by only one letter, which could easily cause typos and confusion:
Script Mode ./starmeup.shForeground ./startmeup.shBackground Consider renaming for clarity, e.g.:
start-foreground.sh/start-background.shstart.sh/startd.sh(following the daemon convention)This is a minor usability concern and may not warrant changing if the naming is intentional or established.
117-124: Add language specifiers to code blocks.Several code blocks are missing language specifiers, which affects syntax highlighting. For example:
**On Windows:** Open Notepad as Administrator, then open: -``` +```text C:\Windows\System32\drivers\etc\hostsAdd this line:
-+text
127.0.0.1 ivplv1.testThis applies to code blocks at lines 117, 122, 155, 167, and 430.
.docker/workspace/Dockerfile (2)
121-147: Consider consolidating duplicate banner loading logic.The banner loading logic is duplicated between root (lines 121-132) and ivpldock (lines 136-147) users. While this ensures both users see the banner, consider extracting this into a shared script to reduce duplication and maintenance overhead.
💡 Alternative approach using a shared setup script
Create a shared banner setup script that can be sourced by both users:
# In a new file: .docker/workspace/setup-banner.sh #!/bin/bash cat >> ~/.bashrc << 'EOF' # Set container type for banner export CONTAINER_TYPE='Workspace Container' # Load InvoicePlane Banner if [ -f ~/invoiceplane-banner.sh ]; then source ~/invoiceplane-banner.sh fi EOFThen in the Dockerfile:
COPY workspace/setup-banner.sh /tmp/setup-banner.sh RUN chmod +x /tmp/setup-banner.sh && \ /tmp/setup-banner.sh && \ rm /tmp/setup-banner.sh USER ivpldock RUN /tmp/setup-banner.sh
241-258: Review the extension installation fallback pattern.The BZ2 and GMP installations use
|| trueto prevent build failures, but this can hide genuine installation errors. Ifpecl installfails, the extension may not be properly installed despite the build succeeding.Consider verifying the extension is loaded after installation:
if [ ${INSTALL_BZ2} = true ]; then \ (apt-get -yqq install php${IVPLDOCK_PHP_VERSION}-bz2 || \ (apt-get -yqq install libbz2-dev && \ pecl install bz2 && \ mkdir -p /etc/php/${IVPLDOCK_PHP_VERSION}/mods-available /etc/php/${IVPLDOCK_PHP_VERSION}/cli/conf.d && \ echo "extension=bz2.so" > /etc/php/${IVPLDOCK_PHP_VERSION}/mods-available/bz2.ini && \ ln -sf /etc/php/${IVPLDOCK_PHP_VERSION}/mods-available/bz2.ini /etc/php/${IVPLDOCK_PHP_VERSION}/cli/conf.d/20-bz2.ini)) && \ php -m | grep -q bz2; \ fi</review_comment_end -->
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.docker/invoiceplane-banner.sh.docker/laravel-horizon/Dockerfile.docker/mariadb/Dockerfile.docker/nginx/Dockerfile.docker/php-fpm/Dockerfile.docker/php-worker/Dockerfile.docker/redis/Dockerfile.docker/workspace/Dockerfile.docker/workspace/Dockerfile.74.docker/workspace/Dockerfile.81.docker/workspace/xdebug.ini.env.example.github/copilot-instructions.md.github/workflows/docker-build-test.yml.github/workflows/test-containers.yml.gitignore.junie/guidelines.mdDOCKER_COMPATIBILITY.mdREADME.mdbuilddmeup.shbuildmeup.shdocker-compose.ymldocker-exec.shdown.shphpmeup.shrootmeup.shstarmeup.shstartmeup.shworker.shworkmeup.sh
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/test-containers.yml
11-11: string should not be empty
(syntax-check)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 158-158: [UnorderedKey] The WORKSPACE_INSTALL_PYTHON key should go before the WORKSPACE_INSTALL_SMB key
(UnorderedKey)
🪛 LanguageTool
README.md
[uncategorized] ~347-~347: The official name of this software platform is spelled with a capital “H”.
Context: ...sites/| Nginx site configurations | |.github/workflows/` | GitHub Actions CI/CD pipe...
(GITHUB)
[uncategorized] ~688-~688: The official name of this software platform is spelled with a capital “H”.
Context: ...uidelines - Copilot Instructions: [.github/copilot-instructions.md](.github/copilo...
(GITHUB)
[uncategorized] ~688-~688: The official name of this software platform is spelled with a capital “H”.
Context: ...ions**: .github/copilot-instructions.md - AI coding as...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
README.md
101-101: Blank line inside blockquote
(MD028, no-blanks-blockquote)
117-117: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
155-155: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
167-167: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
180-180: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
258-258: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
266-266: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
275-275: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
422-422: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
430-430: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
439-439: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
461-461: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
.junie/guidelines.md
286-286: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (65)
.gitignore (1)
6-6: LGTM!The addition of
/.env.testto gitignore is appropriate and consistent with the existing pattern for environment files..github/copilot-instructions.md (8)
1-23: LGTM! Comprehensive project context and core principles.The project context and core principles sections provide clear guidance on minimalism, compatibility (PHP 7.4-8.4), and documentation requirements.
24-78: LGTM! Clear code style guidelines with examples.The code style guidelines effectively demonstrate best practices through good/bad examples for Dockerfiles, shell scripts, and environment variables.
80-144: LGTM! Practical guidance for common tasks.The common tasks section provides actionable step-by-step instructions for adding PHP extensions, modifying Xdebug configuration, and adding new services.
146-171: LGTM! Clear testing requirements and CI/CD integration.The testing requirements section provides practical commands for build, start, and functionality testing, along with CI/CD integration notes.
173-191: LGTM! Important security considerations covered.The security section appropriately emphasizes Xdebug production risks, secrets management, and container security best practices.
193-215: LGTM! Performance optimization guidance is sound.The Docker layer caching section demonstrates proper command ordering to maximize cache efficiency.
217-269: LGTM! Documentation standards and pitfall warnings are helpful.The documentation standards and common pitfalls sections provide valuable guidance for maintaining quality and avoiding common mistakes.
271-326: LGTM! Git workflow and resources are well-documented.The commit message format, PR template guidance, AI assistance best practices, and resource links provide comprehensive support for contributors.
.docker/laravel-horizon/Dockerfile (7)
43-50: LGTM! Resilient PECL channel update.The
|| truepattern makes the PECL channel update non-blocking while still proceeding with extension installations. The subsequentdocker-php-ext-installcommands will catch any critical issues.
72-79: LGTM! Resilient GnuPG extension installation.The
|| truepattern allows the build to continue even if the GnuPG extension fails to install. The subsequentdocker-php-ext-enable gnupgwill catch critical failures if the extension is truly required.
102-126: LGTM! Resilient ImageMagick extension installation.The
|| truepattern on line 122 makes the pecl install non-blocking for ImageMagick. The subsequentdocker-php-ext-enable imagickandphp -m | grep -q 'imagick'commands provide validation.
169-179: LGTM! Resilient Redis extension installation.The
|| truepattern on lines 173 and 175 makes the Redis extension installation non-blocking across different PHP versions. The subsequentdocker-php-ext-enable rediswill catch critical failures.
248-257: LGTM! Resilient MongoDB extension installation.The
|| truepattern on lines 251 and 254 makes the MongoDB extension installation non-blocking for different PHP versions while still enabling the extension afterward.
263-280: LGTM! Resilient YAML extension installation.The
|| truepattern on lines 270, 272, and 274 makes the YAML extension installation non-blocking across different PHP versions with appropriate version-specific handling.
286-298: LGTM! Resilient Memcached extension installation.The
|| truepattern on lines 292 and 294 makes the Memcached extension installation non-blocking. The subsequentphp -m | grep -r 'memcached'command validates the installation..docker/redis/Dockerfile (1)
3-4: Build context configuration is correctly aligned with the COPY paths.The redis service in docker-compose.yml is configured with
context: ./.docker, which correctly resolves the COPY instructions expectingredis/redis.confandredis/init.sh. Both files exist in.docker/redis/and the paths will resolve correctly during the build..docker/mariadb/Dockerfile (1)
13-13: Build context is correctly configured and file exists.The docker-compose.yml sets the build context to
./.dockerfor the mariadb service, and the COPY instructionCOPY mariadb/my.cnf /etc/mysql/my.cnfcorrectly resolves to./.docker/mariadb/my.cnf. The file exists at the expected location.docker-exec.sh (1)
1-7: LGTM! Well-structured helper script with good validation.The script includes proper error handling with
set -euo pipefail, validates arguments, checks for.env.docker, and verifies container/project existence before execution. The usage documentation is clear..docker/workspace/Dockerfile.81 (1)
53-58: Good resilience pattern for user/group creation.The fallback approach for group creation handles cases where the GID is already in use or the group already exists. This prevents build failures in diverse environments.
.docker/invoiceplane-banner.sh (1)
1-7: Good use of early return for non-interactive shells.The
[[ $- != *i* ]] && returnpattern correctly prevents the banner from displaying during non-interactive operations like cron jobs or piped commands..docker/workspace/Dockerfile.74 (1)
389-395: LGTM! Properly guarded pcov installation.The
(pecl install pcov || true)pattern here is correct and resilient. Note that.docker/workspace/Dockerfile.81lacks this guard for pcov—consider updating it to match this pattern..docker/workspace/xdebug.ini (1)
4-10: Good change totriggermode.Switching from
start_with_request=yestotriggerprevents Xdebug from attempting to connect on every request, avoiding connection warnings during builds and non-debugging scenarios. Developers can enable debugging on-demand using theXDEBUG_TRIGGERcookie/environment variable..env.example (1)
158-159: Python installation disabled by default.The change from
WORKSPACE_INSTALL_PYTHON=truetofalsereduces the default image size. Users needing Python 2 can enable it explicitly.Note: The dotenv-linter suggests this key should be ordered before
WORKSPACE_INSTALL_SMB(line 135), but this is a minor style concern.buildmeup.sh (1)
1-27: LGTM! Well-structured build script with proper validation.Good additions:
- Strict error handling with
set -euo pipefail- Preflight check for
.env.docker- Explicit service list for clarity
--buildflag to ensure fresh buildsOne minor note: Line 27 says "Services stopped" but services might have exited due to an error rather than being stopped gracefully. Consider rewording to "Services exited." or removing the message since
set -ewould exit on failure anyway.startmeup.sh (3)
2-2: LGTM: Strict mode enabled.The addition of
set -euo pipefailensures the script fails fast on errors, unset variables, and pipe failures, which is a best practice for production shell scripts.
7-12: LGTM: Environment file validation.The preflight check for
.env.dockerwith clear error guidance prevents downstream failures and improves the developer experience.
15-25: LGTM: Detached service startup.The explicit service list with multi-line formatting improves readability and maintainability. The detached mode (
-d) is appropriate for background services.worker.sh (3)
2-2: LGTM: Strict mode enabled.The addition of
set -euo pipefailis a best practice for production shell scripts.
7-12: LGTM: Environment file validation.The preflight check prevents downstream failures with helpful error guidance.
23-23: No action needed —docker execsupports--env-file.The
--env-fileflag is supported bydocker exec(available since API v1.25) according to official Docker documentation. The code at line 23 is correct.Likely an incorrect or invalid review comment.
down.sh (3)
2-2: LGTM: Strict mode enabled.The addition of
set -euo pipefailensures fail-fast behavior on errors.
7-12: LGTM: Environment file validation.The preflight check is consistent with other scripts and prevents downstream failures.
14-17: LGTM: Destructive cleanup with clear messaging.The script appropriately warns about volume removal (line 5) and provides clear status messages. The use of
-vflag is intentional and documented.starmeup.sh (3)
2-2: LGTM: Strict mode enabled.The addition of
set -euo pipefailis a best practice for production shell scripts.
7-12: LGTM: Environment file validation.The preflight check is consistent across all startup scripts and improves reliability.
14-26: LGTM: Foreground service startup.The explicit service list improves readability. Running in foreground mode (without
-d) allows monitoring logs in real-time, which is appropriate for this script's use case.phpmeup.sh (3)
2-2: LGTM: Strict mode enabled.The addition of
set -euo pipefailensures fail-fast behavior.
7-12: LGTM: Environment file validation.The preflight check prevents downstream failures with helpful guidance.
23-23: Thedocker execcommand does support the--env-fileflag. According to Docker's official documentation and CLI reference,--env-fileis a valid option fordocker exec(in addition todocker runanddocker-compose). The code is correct and will not fail.Likely an incorrect or invalid review comment.
rootmeup.sh (3)
1-2: LGTM! Excellent use of strict mode.The use of
set -euo pipefailensures the script fails fast on errors, undefined variables, and pipeline failures, which is a best practice for shell scripts.
6-11: LGTM! Good preflight validation.The check for
.env.dockerwith a helpful error message and actionable guidance is well implemented.
13-23: LGTM! Robust container validation and execution.The script properly validates the container is running before attempting to exec into it. The use of
--env-file .env.dockeris appropriate for a development environment.workmeup.sh (2)
1-20: LGTM! Consistent implementation pattern.This script follows the same robust validation and error handling pattern as
rootmeup.sh.
22-23: LGTM! Appropriate user selection for regular development work.Using
--user=ivpldockprovides a non-root environment for regular development tasks, following the principle of least privilege. This complementsrootmeup.shwhich provides root access for administrative tasks..docker/nginx/Dockerfile (1)
27-29: LGTM! Proper path update and line ending normalization.The updated COPY path aligns with the standardized build context changes, and the
sedcommand properly handles Windows line ending conversion..github/workflows/docker-build-test.yml (3)
1-13: LGTM! Well-structured workflow with comprehensive PHP version coverage.The workflow uses manual dispatch and tests across multiple PHP versions with
fail-fast: false, ensuring comprehensive validation even if individual builds fail.
15-21: LGTM! Using current stable action versions.The workflow uses up-to-date action versions for checkout and Docker Buildx setup.
29-48: php-worker is built but not started - verify if intentional.The workflow builds the
php-workercontainer (lines 39-42) but only startsworkspaceandphp-fpm(line 46). Ifphp-workeris not needed for verification, consider removing its build step to speed up the workflow.builddmeup.sh (1)
14-27: No action needed. The inconsistency between helper scripts usingdocker-composeand the README examples usingdocker composeis intentional and documented. The README.md explicitly states: "Helper scripts usedocker-composefor backward compatibility, butdocker compose(v2) is recommended for manual commands." This is a deliberate design choice, not an oversight.DOCKER_COMPATIBILITY.md (1)
1-96: Well-structured compatibility documentation.This is a helpful reference for tracking cross-platform compatibility issues. The documentation clearly outlines the fixed php-zip issue and provides good guidance for future troubleshooting.
One consideration: the specific line number references (e.g., "Line 222", "Line 229") will become stale as Dockerfiles evolve. Consider using comments or markers in the Dockerfiles themselves that can be searched for, rather than relying on line numbers.
.junie/guidelines.md (1)
1-293: Comprehensive and well-organized development guidelines.This documentation provides excellent coverage of the development workflow, including:
- Clear project structure overview
- Step-by-step setup instructions
- Xdebug configuration with trigger mode explanation
- Security best practices and maintenance routines
The documentation aligns well with the changes in this PR, particularly the resilient PECL installation patterns and CI/CD workflows.
docker-compose.yml (1)
20-22: Good standardization of build contexts.Consolidating all service builds to use a shared
./.dockercontext with explicit dockerfile paths is a clean DRY improvement. This makes the build configuration consistent and easier to maintain.Also applies to: 178-179, 292-293, 351-352, 378-379, 401-403, 414-416, 427-429, 439-441, 463-465, 481-483, 491-493
.github/workflows/test-containers.yml (1)
9-11: Empty string option is intentional for "test all" behavior.The actionlint warning about empty string can be safely ignored here. The empty option serves as a valid "test all containers" choice in the workflow dispatch. This is a common pattern for optional input with a default behavior.
.docker/php-worker/Dockerfile (2)
634-659: Banner integration follows consistent pattern.The banner setup for both root and laradock users is well-structured with proper:
- CRLF line ending normalization (
sed -i 's/\r//g')- Executable permissions
- Ownership settings
- Shell initialization hooks
678-678: PHP version check provides build-time validation.Good addition to verify the built image has the expected PHP version. This helps catch configuration issues early.
.docker/php-fpm/Dockerfile (3)
244-259: Well-documented Xdebug trigger mode configuration.The inline comments clearly explain the rationale for trigger mode (preventing connection warnings during builds) and the version-specific configuration differences between Xdebug 2 and 3. This is helpful for future maintainers.
1405-1429: Banner integration with www-data home directory creation.The banner setup creates
/home/www-dataas a home directory for the www-data user. This is necessary since www-data typically uses/var/wwwas its home, which may contain application code.Note: Ensure this doesn't conflict with any existing www-data home directory configuration in the base image. The
mkdir -pmakes this idempotent.
1374-1375: COPY paths updated for shared build context.The paths correctly reference the php-fpm subdirectory from the shared
./.dockercontext.README.md (2)
1-40: Excellent comprehensive documentation overhaul.The Quick Start section provides a clear, step-by-step onboarding experience. The documentation is well-structured with:
- Clear numbered steps
- Verification commands
- Emoji indicators for visual scanning
684-700: Good resource links and community guidance.The documentation provides helpful links to internal guidelines, external documentation, and community resources. The contribution guide is clear and actionable.
.docker/workspace/Dockerfile (4)
77-79: Good defensive approach to zip installation.The fallback installation with verification is well-designed. The
|| trueallows the installation to fail gracefully, while line 79 ensures the zip extension is ultimately available.</review_comment_end -->
332-347: Well-implemented Xdebug version handling.The configuration correctly differentiates between Xdebug 2 and 3, using trigger mode for Xdebug 3 to prevent build-time connection warnings. The PHP version detection logic properly handles PHP 7.3, 7.4, and 8.x.
</review_comment_end -->
626-628: Good error handling for wkhtmltopdf installation.The wkhtmltopdf installation provides a clear error message on failure rather than failing silently. This is a better pattern than the
|| trueapproach used elsewhere.</review_comment_end -->
110-111: No action needed. Theinvoiceplane-banner.shfile exists at.docker/invoiceplane-banner.sh, so the COPY commands will resolve correctly.
| pecl channel-update pecl.php.net && \ | ||
| groupadd -g ${PGID} ivpldock && \ | ||
| useradd -l -u ${PUID} -g ivpldock -m ivpldock -G docker_env && \ | ||
| (pecl channel-update pecl.php.net || true) && \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the Dockerfile to see the context around line 55
cat -n .docker/workspace/Dockerfile | sed -n '45,70p'Repository: InvoicePlane/InvoicePlane-Docker
Length of output: 1592
🏁 Script executed:
# Search for PECL install commands in the Dockerfile
grep -n "pecl install" .docker/workspace/DockerfileRepository: InvoicePlane/InvoicePlane-Docker
Length of output: 379
Suppress PECL channel-update errors safely, but consider the broader error-hiding pattern.
The || true on line 55 prevents build failures but can hide network or configuration issues. This concern is amplified by the fact that all subsequent PECL install commands (lines 243, 255, 302, 322, 357, 371, 583) also suppress errors with || true, creating a cascading pattern where extensions may silently fail to install while the Docker build succeeds.
Consider either:
- Making error suppression more selective (only for truly optional extensions)
- Adding a verification step to confirm critical extensions are installed
- Documenting which PECL operations are optional vs. required
| # Select a PHP version of the Workspace and PHP-FPM containers (Does not apply to HHVM). | ||
| # Accepted values: 8.3 - 8.2 - 8.1 - 8.0 - 7.4 - 7.3 - 7.2 - 7.1 - 7.0 - 5.6 | ||
| PHP_VERSION=8.1 | ||
| PHP_VERSION=8.4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Dockerfile structure and PHP version references
echo "=== Finding Dockerfile files ==="
find . -name "*Dockerfile*" -type f | head -20
echo ""
echo "=== Checking for PHP_VERSION references in Dockerfiles ==="
rg -n "PHP_VERSION" --type dockerfile --type plaintext -g "Dockerfile*"
echo ""
echo "=== Checking for FROM php: images and version handling ==="
rg -n "FROM.*php" --type dockerfile --type plaintext -g "Dockerfile*"
echo ""
echo "=== Checking .env.example for complete context ==="
cat -n .env.example | sed -n '35,50p'Repository: InvoicePlane/InvoicePlane-Docker
Length of output: 1746
🏁 Script executed:
#!/bin/bash
# Check the main workspace Dockerfile to understand version handling
echo "=== Workspace Dockerfile (main) ==="
head -50 ./.docker/workspace/Dockerfile
echo ""
echo "=== Checking for .84 or 8.4 variant ==="
ls -la ./.docker/workspace/ | grep -i dockerfile
echo ""
echo "=== Looking for version selection logic ==="
rg -n "PHP_VERSION|IVPLDOCK_PHP_VERSION|8\.4|84" ./.docker/workspace/Dockerfile
echo ""
echo "=== Check docker-compose for how versions are used ==="
rg -n "PHP_VERSION|context:" docker-compose.yml | head -30Repository: InvoicePlane/InvoicePlane-Docker
Length of output: 7722
🏁 Script executed:
#!/bin/bash
# Check php-fpm Dockerfile
echo "=== PHP-FPM Dockerfile (first 50 lines) ==="
head -50 ./.docker/php-fpm/Dockerfile
echo ""
echo "=== Check for PHP version-specific config files ==="
ls -la ./.docker/php-fpm/php*.ini 2>/dev/null || echo "No php*.ini files found"
echo ""
echo "=== Check if php8.4.ini exists ==="
if [ -f "./.docker/php-fpm/php8.4.ini" ]; then
echo "php8.4.ini EXISTS"
else
echo "php8.4.ini DOES NOT EXIST"
fi
echo ""
echo "=== List all php-fpm config files ==="
find ./.docker/php-fpm -name "*.ini" -type f
echo ""
echo "=== Check docker-compose php-fpm config references ==="
rg -n "php-fpm.*php.*\.ini" docker-compose.ymlRepository: InvoicePlane/InvoicePlane-Docker
Length of output: 2930
🏁 Script executed:
#!/bin/bash
# Check README for PHP 8.4 support documentation
echo "=== Search README files ==="
find . -maxdepth 2 -name "README*" -type f -exec grep -l "8\.4\|PHP.*version" {} \;
echo ""
echo "=== Check git history for php8.4.ini or recent changes ==="
git log --oneline --all --grep="8.4\|php8.4" 2>/dev/null | head -10
echo ""
echo "=== Search for any mention of PHP 8.4 in codebase ==="
rg -i "php.*8\.4|version.*8\.4" --max-count=5
echo ""
echo "=== Check for any build or setup scripts ==="
find . -maxdepth 2 -name "*.sh" -type f | head -10
echo ""
echo "=== Check what PHP versions laradock provides (via Docker Hub docs reference) ==="
rg -n "laradock/php-fpm\|laradock/workspace" ./.docker/ -A 2 -B 2 | head -30Repository: InvoicePlane/InvoicePlane-Docker
Length of output: 1034
🏁 Script executed:
#!/bin/bash
# Check the actual README content for PHP 8.4 details
echo "=== README.md PHP version section ==="
rg -n "PHP.*8\.4|configurable|PHP-FPM" README.md -B 2 -A 2
echo ""
echo "=== Check DOCKER_COMPATIBILITY.md ==="
cat DOCKER_COMPATIBILITY.md
echo ""
echo "=== Check if there's a fallback mechanism for php.ini ==="
rg -n "php\.ini|PHP_VERSION" docker-compose.yml -B 2 -A 2 | head -40
echo ""
echo "=== Check if php8.3.ini could be used as fallback for 8.4 ==="
tail -20 ./.docker/php-fpm/php8.3.iniRepository: InvoicePlane/InvoicePlane-Docker
Length of output: 7172
Missing PHP 8.4 configuration file.
The ./.docker/php-fpm/php8.4.ini file does not exist. Docker-compose will fail to mount it at line 269. Create php8.4.ini in ./.docker/php-fpm/ (can be based on php8.3.ini) and update .env.example line 41 to include 8.4 in the accepted PHP versions list: # Accepted values: 8.3 - 8.2 - 8.1 - 8.0 - 7.4 - 7.3 - 7.2 - 7.1 - 7.0 - 5.6 - 8.4.
Additionally, verify that the upstream Laradock Docker Hub images laradock/workspace:latest-8.4 and laradock/php-fpm:latest-8.4 are available and support all required PHP extensions before merging.
🤖 Prompt for AI Agents
In @.env.example around lines 40 - 42, The .env.example sets PHP_VERSION=8.4 but
the expected mount file ./ .docker/php-fpm/php8.4.ini is missing and the
accepted-values comment doesn't list 8.4; create a new php8.4.ini in
.docker/php-fpm/ (copy and adjust ./ .docker/php-fpm/php8.3.ini as a base),
update the comment line in .env.example to include " - 8.4" among accepted
values, and confirm that the Docker images referenced
(laradock/workspace:latest-8.4 and laradock/php-fpm:latest-8.4) provide required
PHP extensions before merging.
| ### Secrets Management | ||
|
|
||
| - Never commit `.env.docker` files | ||
| - Use environment variables for sensitive data | ||
| - Document required secrets in `.env.example` |
There was a problem hiding this comment.
Clarify the .env.docker file guidance.
Line 183 states "Never commit .env.docker files", but this guidance may be too absolute. If .env.docker is intended as a tracked template or example file (similar to .env.example), it should be committed. Only files containing sensitive data or local configurations should be excluded.
Consider clarifying whether .env.docker is:
- A template file that should be committed (and users copy/modify it locally)
- A local configuration file that should never be committed (and should be in
.gitignore)
🤖 Prompt for AI Agents
In @.github/copilot-instructions.md around lines 181 - 185, Clarify the guidance
in the "Secrets Management" section about `.env.docker`: explain whether
`.env.docker` is intended as a committed template (like `.env.example`) that
should be tracked and documented for users to copy/modify, or as a local
secrets/config file that must be excluded via `.gitignore`; update the bullet
"Never commit `.env.docker` files" to state the intended behavior and add
explicit instructions for the chosen option (e.g., "Commit `.env.docker` as a
template" or "Never commit `.env.docker`; add to `.gitignore` and provide
`.env.example` instead"), and ensure the change references `.env.docker` and the
"Secrets Management" heading so readers can find the clarification.
| - name: Create .env file for testing | ||
| run: | | ||
| cp .env.example .env.docker | ||
| sed -i "s/PHP_VERSION=8.1/PHP_VERSION=${{ matrix.php-version }}/" .env.docker | ||
| sed -i "s/WORKSPACE_INSTALL_PYTHON3=true/WORKSPACE_INSTALL_PYTHON3=false/" .env.docker | ||
| sed -i "s/WORKSPACE_INSTALL_WKHTMLTOPDF=true/WORKSPACE_INSTALL_WKHTMLTOPDF=false/" .env.docker | ||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name ".env.example" -type fRepository: InvoicePlane/InvoicePlane-Docker
Length of output: 90
🏁 Script executed:
cat -n .env.example | head -50Repository: InvoicePlane/InvoicePlane-Docker
Length of output: 2210
🏁 Script executed:
# Check the exact patterns in .env.example that the sed commands target
rg -n "PHP_VERSION|WORKSPACE_INSTALL_PYTHON3|WORKSPACE_INSTALL_WKHTMLTOPDF" .env.exampleRepository: InvoicePlane/InvoicePlane-Docker
Length of output: 168
🏁 Script executed:
cat -n .github/workflows/docker-build-test.yml | sed -n '20,35p'Repository: InvoicePlane/InvoicePlane-Docker
Length of output: 820
🏁 Script executed:
# Also check what PHP versions are in the matrix
cat .github/workflows/docker-build-test.yml | grep -A 5 "php-version"Repository: InvoicePlane/InvoicePlane-Docker
Length of output: 684
Fix sed pattern to match current .env.example values.
The sed command s/PHP_VERSION=8.1/... will not match because .env.example currently has PHP_VERSION=8.4. This causes the PHP version replacement to silently fail, defeating the purpose of the matrix testing. The other two sed patterns (PYTHON3 and WKHTMLTOPDF) will work correctly.
Use flexible patterns instead:
sed -i "s/PHP_VERSION=.*/PHP_VERSION=${{ matrix.php-version }}/" .env.docker
sed -i "s/WORKSPACE_INSTALL_PYTHON3=.*/WORKSPACE_INSTALL_PYTHON3=false/" .env.docker
sed -i "s/WORKSPACE_INSTALL_WKHTMLTOPDF=.*/WORKSPACE_INSTALL_WKHTMLTOPDF=false/" .env.dockerThis way the replacements remain robust if the .env.example defaults change in the future.
🤖 Prompt for AI Agents
In @.github/workflows/docker-build-test.yml around lines 22 - 28, Update the
three sed replacement commands in the "Create .env file for testing" step so
they match any current default values instead of fixed ones: change the PHP
replacement to match any PHP_VERSION value (use a pattern like PHP_VERSION=.*
and replace with the matrix value) and similarly change the
WORKSPACE_INSTALL_PYTHON3 and WORKSPACE_INSTALL_WKHTMLTOPDF replacements to
match their current values using .* (e.g., WORKSPACE_INSTALL_PYTHON3=.* and
WORKSPACE_INSTALL_WKHTMLTOPDF=.*) so future changes in .env.example won't break
the replacements.
| docker exec -it "$CONTAINER_ID" bash -c "cd /var/www/projects/${PROJECT_DIR} && ${COMMAND}" | ||
|
|
||
| EXIT_CODE=$? | ||
|
|
||
| if [ $EXIT_CODE -eq 0 ]; then | ||
| echo "----------------------------------------" | ||
| echo "Command completed successfully." | ||
| else | ||
| echo "----------------------------------------" | ||
| echo "Command failed with exit code: $EXIT_CODE" | ||
| exit $EXIT_CODE | ||
| fi |
There was a problem hiding this comment.
Exit code capture is unreachable with set -e.
Line 79 (EXIT_CODE=$?) will never execute if the docker exec command on line 77 fails, because set -e causes the script to exit immediately on any non-zero exit status.
To properly capture and handle the exit code, disable set -e temporarily or use a different pattern:
🔎 Proposed fix
# Execute the command in the container
-docker exec -it "$CONTAINER_ID" bash -c "cd /var/www/projects/${PROJECT_DIR} && ${COMMAND}"
-
-EXIT_CODE=$?
+if docker exec -it "$CONTAINER_ID" bash -c "cd /var/www/projects/${PROJECT_DIR} && ${COMMAND}"; then
+ echo "----------------------------------------"
+ echo "Command completed successfully."
+else
+ EXIT_CODE=$?
+ echo "----------------------------------------"
+ echo "Command failed with exit code: $EXIT_CODE"
+ exit $EXIT_CODE
+fi
-
-if [ $EXIT_CODE -eq 0 ]; then
- echo "----------------------------------------"
- echo "Command completed successfully."
-else
- echo "----------------------------------------"
- echo "Command failed with exit code: $EXIT_CODE"
- exit $EXIT_CODE
-fi📝 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.
| docker exec -it "$CONTAINER_ID" bash -c "cd /var/www/projects/${PROJECT_DIR} && ${COMMAND}" | |
| EXIT_CODE=$? | |
| if [ $EXIT_CODE -eq 0 ]; then | |
| echo "----------------------------------------" | |
| echo "Command completed successfully." | |
| else | |
| echo "----------------------------------------" | |
| echo "Command failed with exit code: $EXIT_CODE" | |
| exit $EXIT_CODE | |
| fi | |
| # Execute the command in the container | |
| if docker exec -it "$CONTAINER_ID" bash -c "cd /var/www/projects/${PROJECT_DIR} && ${COMMAND}"; then | |
| echo "----------------------------------------" | |
| echo "Command completed successfully." | |
| else | |
| EXIT_CODE=$? | |
| echo "----------------------------------------" | |
| echo "Command failed with exit code: $EXIT_CODE" | |
| exit $EXIT_CODE | |
| fi |
🤖 Prompt for AI Agents
In @docker-exec.sh around lines 77 - 88, The script uses set -e so the docker
exec line will cause the script to exit immediately on failure and never reach
the EXIT_CODE=$? handling; to fix, temporarily disable errexit before running
docker exec (or use an if/then wrapper) so you can capture the command result:
turn off errexit (set +e) immediately before invoking docker exec (the command
that uses CONTAINER_ID, PROJECT_DIR, and COMMAND), capture its exit into
EXIT_CODE=$?, then restore errexit (set -e) and proceed with the existing
success/failure branch that checks EXIT_CODE.
There was a problem hiding this comment.
Pull request overview
This PR merges the develop branch into master, representing a major version 2.0 release of the InvoicePlane-Docker environment. The changes focus on improving stability, error handling, documentation, and cross-platform compatibility while maintaining support for PHP 7.4 through 8.4.
Key Changes:
- Enhanced shell scripts with comprehensive error handling and validation (
set -euo pipefail, environment checks, container state validation) - Improved Docker build reliability with error-tolerant PECL installations and COPY path corrections for multi-context builds
- Extensive documentation overhaul including detailed setup guides, troubleshooting sections, and development guidelines
- Added CI/CD workflows for automated testing across PHP 8.2, 8.3, and 8.4
- Configured Xdebug in trigger mode to prevent build-time connection warnings
- Added InvoicePlane-branded welcome banners across all containers
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
*.sh (shell scripts) |
Added set -euo pipefail, .env.docker validation, container state checks, and user-friendly error messages |
docker-compose.yml |
Updated build contexts from individual service directories to .docker parent with dockerfile specifications; removed /data/tenants volume mount |
.docker/*/Dockerfile |
Wrapped PECL commands with ` |
README.md |
Complete rewrite with detailed setup instructions, configuration examples, troubleshooting guide, and comprehensive feature documentation |
.github/workflows/*.yml |
Added new CI/CD workflows for testing individual containers and full builds across PHP versions |
.junie/guidelines.md |
New development guidelines document covering Docker best practices, testing requirements, and contribution process |
.github/copilot-instructions.md |
New AI coding assistant guidelines specific to the project's needs and patterns |
DOCKER_COMPATIBILITY.md |
New compatibility tracking document for cross-platform issues |
.docker/invoiceplane-banner.sh |
New shared banner script for consistent welcome messages across all containers |
.env.example |
Updated default PHP version from 8.1 to 8.4; disabled Python 2 by default |
| # Install the zip extension if available (base image already includes it, but try anyway) | ||
| (apt-get install -yqq php${IVPLDOCK_PHP_VERSION}-zip || true) && \ |
There was a problem hiding this comment.
The installation of php-zip package is now wrapped in a try-catch pattern (apt-get install -yqq php${IVPLDOCK_PHP_VERSION}-zip || true). While this prevents build failures, it silently ignores the error without ensuring the zip extension is actually available. The subsequent php -m | grep -q 'zip' check will fail if the package installation failed and there's no PECL fallback. Consider adding a PECL fallback similar to the approach used in other parts of the file.
| # Install the zip extension if available (base image already includes it, but try anyway) | |
| (apt-get install -yqq php${IVPLDOCK_PHP_VERSION}-zip || true) && \ | |
| # Install the zip extension via distro package if available (base image already includes it, but try anyway) | |
| (apt-get install -yqq php${IVPLDOCK_PHP_VERSION}-zip || true) && \ | |
| # If zip is still not available (e.g. package missing for this PHP version), fall back to PECL | |
| if ! php -m | grep -q 'zip'; then \ | |
| printf "\nInstalling zip extension via PECL fallback...\n"; \ | |
| yes '' | pecl install zip && docker-php-ext-enable zip; \ | |
| fi && \ | |
| # Final assert: fail the build if zip extension is still not loaded |
| # | ||
|
|
||
| RUN php -v | head -n 1 | grep -q "PHP ${PHP_VERSION}." | ||
| RUN php -v | head -n 1 | grep -q "PHP ${IVPLDOCK_PHP_VERSION}." |
There was a problem hiding this comment.
The PHP version verification on line 678 uses the variable IVPLDOCK_PHP_VERSION but earlier in the file (line 675), a comment refers to PHP_VERSION. This inconsistency in variable naming could cause confusion. Ensure the correct variable is used and that it's properly set in the environment.
| - https_proxy | ||
| - no_proxy | ||
| volumes: | ||
| - ${APP_CODE_PATH_HOST}:${APP_CODE_PATH_CONTAINER}${APP_CODE_CONTAINER_FLAG} |
There was a problem hiding this comment.
The workspace volume mount removes the /data/tenants:/tenants mapping without explanation. If this volume was being used in production deployments, removing it could be a breaking change that causes data loss or application errors. This should be documented in the PR description or maintained for backward compatibility.
| - ${APP_CODE_PATH_HOST}:${APP_CODE_PATH_CONTAINER}${APP_CODE_CONTAINER_FLAG} | |
| - ${APP_CODE_PATH_HOST}:${APP_CODE_PATH_CONTAINER}${APP_CODE_CONTAINER_FLAG} | |
| - /data/tenants:/tenants |
| # Select a PHP version of the Workspace and PHP-FPM containers (Does not apply to HHVM). | ||
| # Accepted values: 8.3 - 8.2 - 8.1 - 8.0 - 7.4 - 7.3 - 7.2 - 7.1 - 7.0 - 5.6 | ||
| PHP_VERSION=8.1 | ||
| PHP_VERSION=8.4 |
There was a problem hiding this comment.
The default PHP version is changed from 8.1 to 8.4 without explanation. While 8.4 is the latest version, this is a breaking change for users who depend on PHP 8.1 behavior or extensions. This change should be clearly documented in release notes, and users should be advised to explicitly set their PHP version in .env.docker if they need a specific version.
.docker/invoiceplane-banner.sh
Outdated
| host_name=$(hostname 2>/dev/null || echo "unknown") | ||
| pwd=$(pwd 2>/dev/null || echo "~") | ||
|
|
||
| echo -e "Container: ${GREEN}${host_name}${NC} | User: ${YELLOW}${user_name}${NC} | Dir: ${GREEN}${pwd}${NC}" |
There was a problem hiding this comment.
The echo statement on line 93 uses an unescaped backtick-style command substitution inside a double-quoted string. The pwd variable is set on line 91, so this line should use the variable ${pwd} instead of calling pwd again. This creates unnecessary process spawning.
| WORKSPACE_INSTALL_MC=false | ||
| WORKSPACE_INSTALL_SYMFONY=false | ||
| WORKSPACE_INSTALL_PYTHON=true | ||
| WORKSPACE_INSTALL_PYTHON=false |
There was a problem hiding this comment.
The WORKSPACE_INSTALL_PYTHON setting is changed from true to false as the default. This is a breaking change for users who may rely on Python 2.7 being available in the workspace. Given that Python 2 is deprecated, this change makes sense, but it should be documented as a breaking change in release notes.
| WORKSPACE_INSTALL_PYTHON=false | |
| # Deprecated: Python 2.7 is end-of-life. This remains enabled by default for backward compatibility. | |
| # For new projects that do not need Python 2.7, set WORKSPACE_INSTALL_PYTHON=false in your .env file. | |
| WORKSPACE_INSTALL_PYTHON=true |
| CONTAINER_NAME="$1" | ||
| PROJECT_DIR="$2" | ||
| COMMAND="$3" | ||
|
|
||
| # Check if .env.docker exists | ||
| if [ ! -f .env.docker ]; then | ||
| echo "Error: .env.docker file not found!" | ||
| echo "Please create it from .env.example: cp .env.example .env.docker" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Get the container ID | ||
| CONTAINER_ID=$(docker ps -aqf "name=${CONTAINER_NAME}") | ||
|
|
||
| if [ -z "$CONTAINER_ID" ]; then | ||
| echo "Error: Container '${CONTAINER_NAME}' is not running!" | ||
| echo "Start it with: ./startmeup.sh or ./buildmeup.sh" | ||
| echo "" | ||
| echo "Available running containers:" | ||
| docker ps --format "table {{.Names}}\t{{.Status}}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Check if project directory exists in container | ||
| if ! docker exec "$CONTAINER_ID" test -d "/var/www/projects/${PROJECT_DIR}"; then | ||
| echo "Warning: Project directory '/var/www/projects/${PROJECT_DIR}' does not exist in container!" | ||
| echo "Available projects in /var/www/projects:" | ||
| docker exec "$CONTAINER_ID" ls -1 /var/www/projects/ 2>/dev/null || echo " (directory is empty or not accessible)" | ||
| echo "" | ||
| read -p "Continue anyway? (y/N) " -n 1 -r | ||
| echo | ||
| if [[ ! $REPLY =~ ^[Yy]$ ]]; then | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| echo "Running command in ${CONTAINER_NAME} container..." | ||
| echo "Project: /var/www/projects/${PROJECT_DIR}" | ||
| echo "Command: ${COMMAND}" | ||
| echo "----------------------------------------" | ||
|
|
||
| # Execute the command in the container | ||
| docker exec -it "$CONTAINER_ID" bash -c "cd /var/www/projects/${PROJECT_DIR} && ${COMMAND}" |
There was a problem hiding this comment.
The command passed to docker exec is constructed via string interpolation with untrusted CLI arguments (PROJECT_DIR and especially COMMAND) inside a double-quoted bash -c string. An attacker who can influence these arguments (e.g., via a copy-pasted command) can inject shell metacharacters or quotes into COMMAND/PROJECT_DIR, break out of the intended command, and execute arbitrary commands on the host before docker exec runs. To fix this, avoid embedding user-controlled strings into a single bash -c command; instead, pass the project directory and command as separate, safely-quoted arguments (e.g., by treating the command as an argument list rather than a pre-built shell string) or robustly escape/validate them so that metacharacters cannot alter the command structure.
|
@copilot open a new pull request to apply changes based on the comments in this thread AND In @.docker/invoiceplane-banner.sh:
In @.docker/workspace/Dockerfile:
In @.docker/workspace/Dockerfile.74:
In @.docker/workspace/Dockerfile.81:
In @.env.example:
In @.github/copilot-instructions.md:
In @.github/workflows/docker-build-test.yml:
In @.github/workflows/test-containers.yml:
In @docker-exec.sh:
|
|
@nielsdrost7 I've opened a new pull request, #15, to work on those changes. Once the pull request is ready, I'll request review from you. |
…rkflow updates Co-authored-by: nielsdrost7 <[email protected]>
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.