Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDockerfile now supports configurable PHP installation via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (1)
Dockerfile (1)
2-4: Consider whether PHP 8.5 should be the default version.PHP 8.5 may still be in development/alpha. Setting it as the default could cause issues with extension availability (as evidenced by the fallback logic for xdebug/imagick later). Consider using PHP 8.4 as the stable default unless cutting-edge testing is explicitly required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 2 - 4, The Dockerfile currently sets ENV PHP_VERSION_DEFAULT="8.5" which may be unstable; change the default to a stable release by updating ENV PHP_VERSION_DEFAULT to "8.4" (and keep ARG PHP_VERSION and ENV PHP_VERSION_ALL as-is), and verify any extension fallback logic that references PHP_VERSION_DEFAULT (e.g., xdebug/imagick handling) still behaves correctly with 8.4—adjust tests/docs or CI matrix if you intend to keep 8.5 for cutting-edge testing only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Line 95: The Docker repo line hardcodes the Ubuntu codename "noble" which
breaks portability; change the echo that writes to
/etc/apt/sources.list.d/docker.list (the line using dpkg --print-architecture
and $SUDO tee) to dynamically determine the release codename (e.g., using
lsb_release -cs or parsing /etc/os-release for VERSION_CODENAME) instead of the
literal "noble", so the repository entry matches the base image at runtime.
- Around line 8-10: The Dockerfile currently creates /home/runner with
overly-permissive permissions and uses incorrect sudoers syntax; update the
mkdir command in the RUN chain to use mode 755 instead of 777 (change "mkdir -m
777 -p /home/runner" to "mkdir -m 755 -p /home/runner") and fix the sed
replacement on /etc/sudoers to remove the extra space before the colon so it
produces a valid entry (e.g., replace "NOPASSWD : ALL" with "NOPASSWD: ALL" or
"NOPASSWD:ALL") while keeping the same RUN chain and the existing usermod -aG
sudo runner step.
- Line 77: The RUN currently uses "set -ex" which omits pipefail and allows
piped command failures to be ignored; update the RUN invocation to enable
pipefail (e.g., add "-o pipefail" to the shell options so the RUN becomes "set
-ex -o pipefail" or equivalent) or alternatively add a preceding SHELL
instruction like 'SHELL ["/bin/bash", "-o", "pipefail", "-c"]' so piped commands
such as the curl | gpg / curl | tee / php after curl will fail loudly; change
the RUN line labeled "RUN set -ex" accordingly.
- Around line 117-118: The apt-get install calls for php"$v"-xdebug and
php"$v"-imagick currently silence stderr with 2>/dev/null, hiding real failures;
change this to first check package availability (e.g., use apt-cache policy or
apt-get -s install) for php"$v"-xdebug and php"$v"-imagick, log or surface the
actual apt error output if the installation fails, and only then fall back to
the spc commands (spc -p "$v" -e xdebug-xdebug/... and IMAGICK_LIBS=... spc -p
"$v" -e imagick-imagick/...) on a clear "package not available" condition—do not
blindly redirect stderr to /dev/null and ensure the failure path reports the apt
error before invoking the spc fallback.
- Around line 78-141: The Dockerfile sets SUDO and uses $SUDO repeatedly (see
SUDO variable and commands like $SUDO apt-get, $SUDO add-apt-repository, $SUDO
install, and the for-loops over PHP_VERSIONS), which will trigger hadolint rule
DL3004 since `.hadolint.yaml` does not ignore it; either add DL3004 to the
ignored rules in `.hadolint.yaml` if using sudo intentionally, or remove/replace
$SUDO by executing those package-install and system configuration steps as root
(before switching to USER runner) so commands like apt-get/install,
add-apt-repository, curl -o /usr/bin/systemctl, and usermod run without sudo;
update references to SUDO in the loops that install php"$v" and
php"$v"-xdebug/php"$v"-imagick accordingly.
- Around line 146-149: The piped install line using "curl | php" can hide curl
failures; change the Composer install RUN so it first downloads the installer
with curl -f to a temp file, verify curl exit status, then execute that file
with php and remove it afterward (or alternatively enable shell pipefail before
piping). Update the Dockerfile RUN that currently does "curl | php ...
--install-dir=/usr/local/bin --filename=composer" to the two-step
download-then-execute approach and ensure chmod +x is applied to
/usr/local/bin/composer only after a successful install.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 2-4: The Dockerfile currently sets ENV PHP_VERSION_DEFAULT="8.5"
which may be unstable; change the default to a stable release by updating ENV
PHP_VERSION_DEFAULT to "8.4" (and keep ARG PHP_VERSION and ENV PHP_VERSION_ALL
as-is), and verify any extension fallback logic that references
PHP_VERSION_DEFAULT (e.g., xdebug/imagick handling) still behaves correctly with
8.4—adjust tests/docs or CI matrix if you intend to keep 8.5 for cutting-edge
testing only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
Dockerfile (5)
11-14:⚠️ Potential issue | 🟠 MajorHarden home directory permissions and avoid broad sudoers in-place rewrite.
Line 13 makes
/home/runnerworld-writable (777), and Line 14 rewrites the%sudoentry globally with fragile syntax.Proposed safer patch
RUN usermod -aG sudo runner \ - && mkdir -p /home/runner \ - && chmod 777 /home/runner \ - && sed -i 's/%sudo\s.*/%sudo ALL=(ALL:ALL) NOPASSWD : ALL/g' /etc/sudoers + && install -d -m 755 /home/runner \ + && echo 'runner ALL=(ALL) NOPASSWD:ALL' > /etc/sudoers.d/runner-nopasswd \ + && chmod 0440 /etc/sudoers.d/runner-nopasswd🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 11 - 14, The RUN sequence makes /home/runner world-writable and dangerously rewrites /etc/sudoers in-place; change it to create the home directory with correct ownership and restrictive perms (use mkdir -p /home/runner then chown runner:runner and chmod 0755 instead of chmod 777) and stop using sed on /etc/sudoers — keep usermod -aG sudo runner but grant passwordless sudo by adding a dedicated file in /etc/sudoers.d (e.g., create /etc/sudoers.d/runner with "runner ALL=(ALL) NOPASSWD:ALL" and set mode 0440) rather than editing /etc/sudoers directly.
150-153:⚠️ Potential issue | 🟠 MajorReplace
curl | phpComposer install with verified two-step installer.Line 150 pipes remote content directly into PHP. Use download + signature verification before execution.
Safer Composer install pattern
-RUN curl -sS https://getcomposer.org/installer | php -- \ - --install-dir=/usr/local/bin \ - --filename=composer \ - && chmod +x /usr/local/bin/composer +RUN curl -fsSL https://getcomposer.org/installer -o /tmp/composer-setup.php \ + && curl -fsSL https://composer.github.io/installer.sig -o /tmp/composer-setup.sig \ + && echo "$(cat /tmp/composer-setup.sig) /tmp/composer-setup.php" | sha384sum -c - \ + && php /tmp/composer-setup.php --install-dir=/usr/local/bin --filename=composer \ + && rm -f /tmp/composer-setup.php /tmp/composer-setup.sig \ + && chmod +x /usr/local/bin/composer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 150 - 153, The Dockerfile currently pipes curl into php to install Composer; replace that with a two-step verified install: download the installer to a temporary file (e.g., composer-setup.php), fetch the official installer signature (e.g., from https://composer.github.io/installer.sig), verify the downloaded installer's SHA-384 matches the signature, only then run php composer-setup.php to install to /usr/local/bin/composer, make it executable (chmod +x /usr/local/bin/composer), and remove the temporary files; reference the existing RUN that pipes curl|php and the target /usr/local/bin/composer so you update that exact install flow to use composer-setup.php and signature verification instead of piping remote content directly to php.
81-123:⚠️ Potential issue | 🟠 Major
sudousage in Dockerfile still conflicts with Hadolint DL3004 configuration.Lines 82+ rely on
$SUDO, but.hadolint.yamldoes not ignoreDL3004. This is likely to keep linting noisy/failing unless explicitly intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 81 - 123, The Dockerfile defines and uses the SUDO variable (SUDO=sudo) and prefixes many commands with $SUDO (e.g., $SUDO apt-get, $SUDO add-apt-repository), which triggers Hadolint DL3004; either remove the sudo pattern or explicitly silence the linter—best fix is to eliminate the SUDO variable and all $SUDO prefixes in the RUN block (so commands run as root during build) and remove related logic, or alternatively add DL3004 to .hadolint.yaml ignores if using sudo is intentional; locate the SUDO variable assignment and every $SUDO usage in the RUN block to apply the change (e.g., the SUDO variable declaration and lines calling $SUDO apt-get, $SUDO add-apt-repository, $SUDO chmod, $SUDO cp, etc.).
99-99:⚠️ Potential issue | 🟡 MinorDo not hardcode Ubuntu codename in Docker repo source.
Line 99 hardcodes
noble; this breaks when the base image codename changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 99, The Docker repo line hardcodes the Ubuntu codename "noble"; replace that literal with a runtime substitution that reads the image codename (e.g. from /etc/os-release). Edit the command that currently echoes 'deb ... noble stable' (the line using dpkg --print-architecture and tee to /etc/apt/sources.list.d/docker.list) and replace "noble" with a shell expansion like $(. /etc/os-release && echo "$VERSION_CODENAME") so the repo entry uses the base image's codename dynamically and remains correct across different Ubuntu variants.
121-122:⚠️ Potential issue | 🟡 MinorSuppressing apt stderr masks real install failures before fallback.
Lines 121-122 redirect apt errors to
/dev/null, so transient/network/dependency failures are indistinguishable from “package unavailable”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 121 - 122, The current Dockerfile lines that install php"$v"-xdebug and php"$v"-imagick suppress apt errors with "2>/dev/null", which hides real apt failures before falling back to spc; remove the stderr redirection and instead let apt-get emit errors (or capture them) and rely on the existing "||" fallback to spc, so that apt failures are visible for debugging; update the two installation commands referencing php"$v"-xdebug, php"$v"-imagick, $SUDO, and the spc fallback invocations (including the IMAGICK_LIBS wrapper) to no longer redirect stderr and ensure the fallback only runs if apt-get exits non-zero.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 2-4: The Dockerfile currently defaults ARG PHP_VERSION=all which
causes all PHP versions (ENV PHP_VERSION_ALL) to be installed by default and
negates CI performance gains; change the ARG default to the intended
single-version default (match ENV PHP_VERSION_DEFAULT, e.g. set ARG PHP_VERSION
to "8.5" or to the PHP_VERSION_DEFAULT value) so builds only install one PHP
version, or alternatively keep ARG PHP_VERSION=all but update your workflows
(.github/workflows/*) to pass the appropriate build-arg (PHP_VERSION) when
invoking the build; update references around ARG PHP_VERSION, ENV
PHP_VERSION_ALL and ENV PHP_VERSION_DEFAULT accordingly.
- Around line 113-117: The Dockerfile currently downloads and executes remote
artifacts (the systemctl-shim into /usr/bin/systemctl and the pear installer to
/tmp/pear.phar) without integrity checks; update the curl commands that fetch
https://raw.githubusercontent.com/shivammathur/node-docker/main/systemctl-shim
and
https://raw.githubusercontent.com/pear/pearweb_phars/master/install-pear-nozlib.phar
to also fetch a checksum or GPG signature, import the publisher's public key (or
include expected SHA256 sums as build args), verify the downloaded file before
chmod/exec (use gpg --verify for .asc signatures or sha256sum and compare to the
expected value), and abort the build if verification fails — specifically modify
the steps that write to /usr/bin/systemctl and /tmp/pear.phar so verification
happens prior to chmod/execute and removal.
---
Duplicate comments:
In `@Dockerfile`:
- Around line 11-14: The RUN sequence makes /home/runner world-writable and
dangerously rewrites /etc/sudoers in-place; change it to create the home
directory with correct ownership and restrictive perms (use mkdir -p
/home/runner then chown runner:runner and chmod 0755 instead of chmod 777) and
stop using sed on /etc/sudoers — keep usermod -aG sudo runner but grant
passwordless sudo by adding a dedicated file in /etc/sudoers.d (e.g., create
/etc/sudoers.d/runner with "runner ALL=(ALL) NOPASSWD:ALL" and set mode 0440)
rather than editing /etc/sudoers directly.
- Around line 150-153: The Dockerfile currently pipes curl into php to install
Composer; replace that with a two-step verified install: download the installer
to a temporary file (e.g., composer-setup.php), fetch the official installer
signature (e.g., from https://composer.github.io/installer.sig), verify the
downloaded installer's SHA-384 matches the signature, only then run php
composer-setup.php to install to /usr/local/bin/composer, make it executable
(chmod +x /usr/local/bin/composer), and remove the temporary files; reference
the existing RUN that pipes curl|php and the target /usr/local/bin/composer so
you update that exact install flow to use composer-setup.php and signature
verification instead of piping remote content directly to php.
- Around line 81-123: The Dockerfile defines and uses the SUDO variable
(SUDO=sudo) and prefixes many commands with $SUDO (e.g., $SUDO apt-get, $SUDO
add-apt-repository), which triggers Hadolint DL3004; either remove the sudo
pattern or explicitly silence the linter—best fix is to eliminate the SUDO
variable and all $SUDO prefixes in the RUN block (so commands run as root during
build) and remove related logic, or alternatively add DL3004 to .hadolint.yaml
ignores if using sudo is intentional; locate the SUDO variable assignment and
every $SUDO usage in the RUN block to apply the change (e.g., the SUDO variable
declaration and lines calling $SUDO apt-get, $SUDO add-apt-repository, $SUDO
chmod, $SUDO cp, etc.).
- Line 99: The Docker repo line hardcodes the Ubuntu codename "noble"; replace
that literal with a runtime substitution that reads the image codename (e.g.
from /etc/os-release). Edit the command that currently echoes 'deb ... noble
stable' (the line using dpkg --print-architecture and tee to
/etc/apt/sources.list.d/docker.list) and replace "noble" with a shell expansion
like $(. /etc/os-release && echo "$VERSION_CODENAME") so the repo entry uses the
base image's codename dynamically and remains correct across different Ubuntu
variants.
- Around line 121-122: The current Dockerfile lines that install php"$v"-xdebug
and php"$v"-imagick suppress apt errors with "2>/dev/null", which hides real apt
failures before falling back to spc; remove the stderr redirection and instead
let apt-get emit errors (or capture them) and rely on the existing "||" fallback
to spc, so that apt failures are visible for debugging; update the two
installation commands referencing php"$v"-xdebug, php"$v"-imagick, $SUDO, and
the spc fallback invocations (including the IMAGICK_LIBS wrapper) to no longer
redirect stderr and ensure the fallback only runs if apt-get exits non-zero.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7860fd4b-f75c-4bb1-93d6-53bacebeac1d
📒 Files selected for processing (2)
.hadolint.yamlDockerfile
✅ Files skipped from review due to trivial changes (1)
- .hadolint.yaml
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.
✅ Checklist