Alpine 3.21 update #70
Conversation
WalkthroughThis pull request introduces several updates primarily focused on upgrading the PHP version used in the Moodle Docker image from 8.3 to 8.4, alongside changes to the GitHub Actions workflow for Docker image tagging. The workflow now reflects the branch name change from "master" to "main," affecting how Docker image tags are generated. Additionally, updates are made to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
Dockerfile (2)
11-11: Consider pinning PHP package versions for reproducible builds.The PHP packages are installed without version constraints, which could lead to unexpected updates. Consider pinning specific versions for better reproducibility.
-RUN apk add --no-cache dcron libcap php84-exif php84-pecl-redis php84-pecl-igbinary php84-ldap && \ +RUN apk add --no-cache dcron libcap \ + php84-exif=8.4.0-r0 \ + php84-pecl-redis=6.0.2-r0 \ + php84-pecl-igbinary=3.2.15-r0 \ + php84-ldap=8.4.0-r0 && \
Security: Update gnu-libiconv to version 1.17-r2 from the current Alpine repository
The current installation uses gnu-libiconv 1.15-r3 from an outdated Alpine v3.13 repository, while version 1.17-r2 is available in the latest stable Alpine repository. The issue #26 remains open and appears to be a Moodle-specific problem rather than a libiconv issue.
Consider:
- Update the package installation command to:
RUN apk add gnu-libiconv --update-cache --repository https://dl-cdn.alpinelinux.org/alpine/latest-stable/community/
- Remove the
--allow-untrustedflag as it's not needed when using HTTPS- Let the package manager select the latest available version (1.17-r2) instead of pinning to an old version
🔗 Analysis chain
Line range hint
15-16: Security concern: Using outdated Alpine repository and old libiconv version.The Dockerfile installs an old version of gnu-libiconv (1.15-r3) from an outdated Alpine repository (v3.13). This could pose security risks.
Consider:
- Updating to a newer version of libiconv from the current Alpine repository
- Investigating if the hack for issue Unable to Create New Role with "Incorrect role short name" #26 is still necessary with PHP 8.4
Let me try another approach to get the available libiconv versions more effectively.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if issue #26 is still relevant with PHP 8.4 gh issue view 26 # Check available libiconv versions in current Alpine repositories curl -s https://dl-cdn.alpinelinux.org/alpine/latest-stable/community/x86_64/APKINDEX.tar.gz | tar -xz -O | grep -A3 "gnu-libiconv"Length of output: 1132
Script:
#!/bin/bash # Check available libiconv versions in current Alpine repositories using a different approach curl -s https://dl-cdn.alpinelinux.org/alpine/latest-stable/community/x86_64/APKINDEX.tar.gz | tar -xzf - APKINDEX && cat APKINDEX | grep -A3 "gnu-libiconv" # Also check the version in edge repository as a reference curl -s https://dl-cdn.alpinelinux.org/alpine/edge/community/x86_64/APKINDEX.tar.gz | tar -xzf - APKINDEX && cat APKINDEX | grep -A3 "gnu-libiconv" # Let's also check the base image version being used cat Dockerfile | grep -i "FROM"Length of output: 1951
rootfs/docker-entrypoint-init.d/02-configure-moodle.sh (2)
58-58: Consider extracting PHP flags to variables.The script repeatedly uses the same PHP flags (
-d max_input_vars=10000). Consider extracting this to a variable for better maintainability.+# At the beginning of the script +PHP_FLAGS="-d max_input_vars=10000" +PHP_CMD="php84 ${PHP_FLAGS}" - ENV_VAR='var' php84 -d max_input_vars=10000 /var/www/html/admin/cli/install.php \ + ENV_VAR='var' ${PHP_CMD} /var/www/html/admin/cli/install.php \Also applies to: 83-83, 142-150, 155-156, 159-160, 177-179, 212-212, 219-219, 233-233
142-150: Consider using a loop for repetitive configuration commands.The script has multiple similar configuration commands that could be simplified using an array and loop.
+ # Define configurations as key-value pairs + declare -A configs=( + ["pathtophp"]="/usr/bin/php84" + ["pathtodu"]="/usr/bin/du" + ["enableblogs"]="0" + ["smtphosts"]="$SMTP_HOST:$SMTP_PORT" + ["smtpuser"]="$SMTP_USER" + ["smtppass"]="$SMTP_PASSWORD" + ["smtpsecure"]="$SMTP_PROTOCOL" + ["noreplyaddress"]="$MOODLE_MAIL_NOREPLY_ADDRESS" + ["emailsubjectprefix"]="$MOODLE_MAIL_PREFIX" + ) + + # Apply configurations in a loop + for key in "${!configs[@]}"; do + ${PHP_CMD} /var/www/html/admin/cli/cfg.php --name="$key" --set="${configs[$key]}" + doneREADME.md (1)
19-19: Consider adding performance metrics to support claims.The description claims PHP 8.4 provides "better performance, lower cpu usage & memory footprint". Consider adding specific performance metrics or benchmarks to support these claims, helping users understand the concrete benefits of upgrading.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.github/workflows/build.yml(1 hunks)Dockerfile(1 hunks)README.md(2 hunks)rootfs/docker-entrypoint-init.d/02-configure-moodle.sh(6 hunks)rootfs/etc/crontabs/nobody(1 hunks)rootfs/etc/nginx/nginx.conf(0 hunks)
💤 Files with no reviewable changes (1)
- rootfs/etc/nginx/nginx.conf
✅ Files skipped from review due to trivial changes (1)
- rootfs/etc/crontabs/nobody
🔇 Additional comments (1)
.github/workflows/build.yml (1)
28-29: LGTM: Branch name update follows best practices.
The change from "master" to "main" aligns with current Git naming conventions and best practices.
Also applies to: 35-36
| if php84 -d max_input_vars=10000 /var/www/html/admin/cli/isinstalled.php ; then | ||
| install_database | ||
| configure_moodle_settings | ||
| final_configurations |
There was a problem hiding this comment.
Critical: Database installation logic appears incorrect.
The condition seems reversed. It runs install_database when Moodle is already installed (isinstalled.php returns true).
-if php84 -d max_input_vars=10000 /var/www/html/admin/cli/isinstalled.php ; then
+if ! php84 -d max_input_vars=10000 /var/www/html/admin/cli/isinstalled.php ; then
install_database
configure_moodle_settings
final_configurations📝 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.
| if php84 -d max_input_vars=10000 /var/www/html/admin/cli/isinstalled.php ; then | |
| install_database | |
| configure_moodle_settings | |
| final_configurations | |
| if ! php84 -d max_input_vars=10000 /var/www/html/admin/cli/isinstalled.php ; then | |
| install_database | |
| configure_moodle_settings | |
| final_configurations |
|  | ||
|  |
There was a problem hiding this comment.
💡 Codebase verification
Version compatibility mismatch detected in implementation
The badge in README.md shows PHP 8.4, but there are inconsistencies in the implementation:
- The Dockerfile uses
php84packages, correctly aligned with PHP 8.4 - The Moodle version in Dockerfile is set to
MOODLE_405_STABLE, which corresponds to Moodle 4.0.5, not Moodle 4.5.0 as shown in the badge - All PHP CLI commands in configuration scripts correctly use
php84
🔗 Analysis chain
Verify version compatibility with implementation.
The badge updates reflect PHP 8.4 and Moodle 4.5.0. Let's verify these versions are correctly implemented across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify PHP and Moodle versions in implementation files
# Check PHP version in Dockerfile and configuration scripts
echo "Checking PHP version references..."
rg -l 'php8[34]'
# Check Moodle version references
echo "Checking Moodle version references..."
rg -l '4\.[45]\.0'
# Check if there are any remaining old version references
echo "Checking for any remaining old version references..."
rg -l 'php83|4\.4\.0'
Length of output: 445
Script:
#!/bin/bash
# Let's check the actual content of files to see version references
echo "=== Checking Dockerfile content ==="
cat Dockerfile
echo -e "\n=== Checking crontab configuration ==="
cat rootfs/etc/crontabs/nobody
echo -e "\n=== Checking Moodle configuration script ==="
cat rootfs/docker-entrypoint-init.d/02-configure-moodle.sh
echo -e "\n=== Checking composer.json for PHP version constraints ==="
if [ -f "composer.json" ]; then
cat composer.json
fi
Length of output: 11489
This pull request includes several updates to the project, focusing on upgrading the PHP version from 8.3 to 8.4, updating related configurations, and removing an outdated Nginx configuration file.
PHP Version Upgrade:
Dockerfile: Updated PHP packages from version 8.3 to 8.4.rootfs/docker-entrypoint-init.d/02-configure-moodle.sh: Replaced all instances ofphp83withphp84in various functions for generating config files, installing databases, configuring settings, upgrading Moodle, and other configurations. [1] [2] [3] [4] [5] [6]rootfs/etc/crontabs/nobody: Updated the cron job to usephp84instead ofphp83.Documentation Updates:
README.md: Updated the PHP version badge from 8.3 to 8.4 and the Moodle version badge from 4.4.0 to 4.5.0. Additionally, updated the PHP version mentioned in the repository description. [1] [2]Configuration Changes:
.github/workflows/build.yml: Changed the branch reference frommastertomainfor tagging Docker images.Removal of Outdated Configuration:
rootfs/etc/nginx/nginx.conf: Removed the entire Nginx configuration file, as it is no longer needed.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores