Skip to content

Conversation

@stnguyen90
Copy link
Contributor

@stnguyen90 stnguyen90 commented Oct 28, 2025

What does this PR do?

Because libwebp was not installed, imagemagick-dev wasn't installed with webp support. To ensure downstream dependencies to support webp, libwebp must be installed along with imagemagick-dev

Test Plan

Updated tests to check for WEBP as supported format

Related PRs and Issues

Have you read the Contributing Guidelines on issues?

Yes

Summary by CodeRabbit

  • Chores

    • Added WebP image format support library to runtime dependencies to enable WebP handling.
  • Tests

    • Updated system verification tests to check for WebP support and adjusted PHP-related test naming to reflect module validation.

@coderabbitai
Copy link

coderabbitai bot commented Oct 28, 2025

Walkthrough

The Dockerfile was changed to add the libwebp package to the APK runtime dependencies in the final image. tests.yaml was updated: the PHP test was renamed from "PHP info" to "PHP modules" and continues to validate PHP modules with php -m; a new test "ImageMagick supported formats" was added that runs php -i and checks for WEBP support (inserted before the PHP intl test). No other functional changes were made.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Check that libwebp is the correct package name for the target Alpine version.
  • Confirm the PHP modules test still correctly uses php -m and the rename is consistent.
  • Verify the new php -i test reliably detects WEBP/ImageMagick support and that its placement in tests.yaml is appropriate.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "feat: add support for webp in imagick" is directly and accurately aligned with the main objective and changes in the changeset. The PR adds libwebp to the Dockerfile to enable WebP support in ImageMagick, and updates tests to verify this support, which is exactly what the title conveys. The title is concise, clear, and specific enough that a teammate reviewing the history would immediately understand the primary change being made. It avoids vague language and accurately reflects the developer's intent.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat-imagick-webp

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 998df1c and cba2f04.

📒 Files selected for processing (2)
  • Dockerfile (1 hunks)
  • tests.yaml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests.yaml
  • Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
Dockerfile (1)

31-32: Consider adding libwebp-dev to the compile stage for consistency and completeness.

The libwebp package is correctly added to the final stage (line 197), but libwebp-dev is missing from the compile stage where imagemagick-dev is installed. For consistency with other image format development libraries (e.g., libjpeg-turbo-dev at line 33, libpng-dev at line 35, libjxl-dev at line 36), consider adding libwebp-dev to the compile stage as well.

This ensures that if imagemagick-dev needs to detect or support WebP formats during the compilation phase, the necessary development headers are available.

Apply this diff to add libwebp-dev to the compile stage:

 RUN apk update && apk upgrade && apk add --no-cache --virtual .deps \
   linux-headers \
   make \
   automake \
   autoconf \
   gcc \
   g++ \
   git \
   zlib-dev \
   openssl-dev \
   yaml-dev \
   imagemagick \
   imagemagick-dev \
   libjpeg-turbo-dev \
   jpeg-dev \
   libpng-dev \
   libjxl-dev \
+  libwebp-dev \
   libmaxminddb-dev \
   zstd-dev \
   brotli-dev \
   lz4-dev \
   curl-dev

Also applies to: 174-210

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6924997 and 998df1c.

📒 Files selected for processing (2)
  • Dockerfile (1 hunks)
  • tests.yaml (2 hunks)
🔇 Additional comments (1)
tests.yaml (1)

20-36: Test is correct; clarify where WEBP appears in output.

The test change from php -m to php -i is appropriate, and WEBP will be correctly detected. However, WEBP appears in the "ImageMagick supported formats" section of php -i output, not in the imagick module configuration line. It appears as part of a comma-separated formats list (e.g., "...WMV, WEBP, WPG...") when ImageMagick is compiled with libwebp support.

@stnguyen90 stnguyen90 requested a review from abnegate October 28, 2025 05:52
tests.yaml Outdated
- name: 'PHP info'
command: "php"
args: ["-m"]
args: ["-i"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep this as m, i can show like x support => disabled which will still match

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants