feat: Add Drupal testing workflow and Docker build trigger#24
feat: Add Drupal testing workflow and Docker build trigger#24hussainweb merged 1 commit intomainfrom
Conversation
This commit introduces a new GitHub Actions workflow (`test-drupal.yml`) designed to test Drupal installations. It also modifies the existing `docker-buildx.yml` workflow to be triggered by the successful completion of the Drupal testing workflow when the target branch is `main`.
Key changes include:
- **New `test-drupal.yml` workflow:**
- Sets up basic Drupal project structure and installs Drupal using Drush.
- Utilizes Docker Compose to spin up Nginx and PHP-FPM/Apache services.
- Runs validation scripts to ensure Drupal is installed and functional.
- Captures logs and uploads test artifacts for analysis.
- **Modified `docker-buildx.yml` workflow:**
- Now includes a `workflow_run` trigger that activates after the "Test Drupal Installation" workflow completes successfully on the `main` branch.
- The `push` and `pull_request` triggers on `main` are still present for direct pushes.
- Conditionals are updated to ensure Docker images are only pushed to Docker Hub for successful runs on the `main` branch.
- **New Docker Compose files and scripts:**
- `tests/docker-compose.yml` and `tests/docker-compose.fpm.yml` for defining test environments.
- `tests/install-drupal.sh` for automating Drupal installation.
- `tests/verify-drupal.sh` for performing post-installation verification.
- `tests/nginx.conf` for Nginx configuration within the test environment.
📝 WalkthroughWalkthroughThis PR adds comprehensive Drupal installation and verification test infrastructure. It modifies the Docker buildx workflow to use workflow_run triggers with conditional gating, introduces a new test-drupal workflow that builds and tests Drupal across multiple PHP versions and container variants, and includes supporting docker-compose configurations, shell scripts, and an nginx configuration for testing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🤖 Fix all issues with AI agents
In `@tests/install-drupal.sh`:
- Around line 4-5: The script accepts a DRUPAL_VERSION argument (variable
DRUPAL_VERSION) but never uses it; update the installation commands (where
composer or drush bootstrap/install is invoked, e.g., the composer
create-project or drush site:install steps) to include the DRUPAL_VERSION
variable so the installed Drupal is pinned (for example by passing composer
create-project drupal/recommended-project:"$DRUPAL_VERSION" or the equivalent
version specifier), or if you don't need version pinning remove the
DRUPAL_VERSION parameter and related echo to avoid a no-op argument; ensure you
reference the DRUPAL_VERSION variable (and keep VARIANT handling unchanged).
- Around line 53-59: The sh -c string contains nested double quotes that break
the argument parsing for the drush command; update the docker compose exec
invocation (the line building the sh -c argument that cd's to ${WEBROOT} and
runs vendor/bin/drush site:install) so that inner string literals (site DB URL
and --site-name value) are either single-quoted or properly escaped to avoid
closing the outer double-quoted sh -c argument; ensure the full command
(including --account-name, --account-pass, --yes, --no-interaction) is passed as
a single argument to sh -c and verify variables SERVICE and WEBROOT remain
unchanged.
🧹 Nitpick comments (6)
.github/workflows/docker-buildx.yml (1)
53-55: Push/login conditions are consistent but consider a reusable expression.The same branch-check expression is duplicated on lines 53–55 and 85–87. This works correctly —
workflow_runevents usehead_branchwhile other events usegithub.ref. If you ever need to adjust the logic, you'd have to update both places.Consider extracting this into a job-level environment variable or a step output to keep it DRY, though this is minor for a workflow file.
Also applies to: 85-87
tests/nginx.conf (1)
63-67:deny allis dead code when combined withreturn 404.In nginx,
returndirectives take precedence over access-control directives likedeny all. Thedeny allon line 65 (and similarly on line 71) is never evaluated. If the intent is to return 404 (hiding the resource existence), onlyreturn 404is needed. If you want a 403, remove thereturn.This applies equally to lines 70–73.
Proposed fix
# Don't allow direct access to PHP files in the vendor directory. location ~ /vendor/.*\.php$ { - deny all; return 404; } # Protect files and directories from prying eyes. location ~* \.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\.(?!well-known).*|Entries.*|Repository|Root|Tag|Template|composer\.(json|lock)|web\.config)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig|\.save)$ { - deny all; return 404; }.github/workflows/test-drupal.yml (3)
62-71: Quote matrix expression arguments to script invocations.Lines 66 and 71 pass
${{ matrix.variant }}unquoted to shell scripts. While current matrix values don't contain spaces or glob characters, quoting is a best practice to prevent breakage if a variant name with special characters is ever added.Static analysis (SC2086) also flags unquoted variable usage in this workflow.
Proposed fix
- ./tests/install-drupal.sh ${{ matrix.variant }} "11.x" + ./tests/install-drupal.sh "${{ matrix.variant }}" "11.x"- ./tests/verify-drupal.sh ${{ matrix.variant }} + ./tests/verify-drupal.sh "${{ matrix.variant }}"
20-20: Checkout action version differs fromdocker-buildx.yml.This workflow uses
actions/checkout@v4whiledocker-buildx.ymlusesactions/checkout@v6. Consider aligning to the same version across workflows for consistency.
56-60: Fixed sleep before readiness check is fine but could be trimmed.The 10-second sleep on line 59 is followed by the install script's own 60-second readiness loop (polling every 5 seconds). The initial sleep is somewhat redundant but harmless — just adds to CI time.
tests/verify-drupal.sh (1)
176-185: Extension check runs a separatedocker compose execper extension — consider batching.Each iteration calls
docker compose exec -T $SERVICE php -m, creating a new container exec for every extension. With 5 extensions, this is 5 round-trips. A single exec capturing all modules would be faster.Proposed optimization
+ALL_MODULES=$(docker compose exec -T $SERVICE php -m 2>/dev/null) for ext in $REQUIRED_EXTENSIONS; do # Use case-insensitive matching and handle "Zend OPcache" naming if [ "$ext" = "opcache" ]; then - if ! docker compose exec -T $SERVICE php -m | grep -qi "opcache"; then + if ! echo "$ALL_MODULES" | grep -qi "opcache"; then MISSING_EXTENSIONS="$MISSING_EXTENSIONS $ext" fi - elif ! docker compose exec -T $SERVICE php -m | grep -qi "^$ext$"; then + elif ! echo "$ALL_MODULES" | grep -qi "^$ext$"; then MISSING_EXTENSIONS="$MISSING_EXTENSIONS $ext" fi done
| VARIANT=$1 | ||
| DRUPAL_VERSION=$2 |
There was a problem hiding this comment.
DRUPAL_VERSION parameter is accepted but never used.
$2 is assigned to DRUPAL_VERSION on line 5 and echoed on line 8, but it's never passed to any drush or composer command. The caller passes "11.x" from the workflow (line 66 of test-drupal.yml), which has no effect.
Either remove the parameter or use it to pin the Drupal version during installation.
🤖 Prompt for AI Agents
In `@tests/install-drupal.sh` around lines 4 - 5, The script accepts a
DRUPAL_VERSION argument (variable DRUPAL_VERSION) but never uses it; update the
installation commands (where composer or drush bootstrap/install is invoked,
e.g., the composer create-project or drush site:install steps) to include the
DRUPAL_VERSION variable so the installed Drupal is pinned (for example by
passing composer create-project drupal/recommended-project:"$DRUPAL_VERSION" or
the equivalent version specifier), or if you don't need version pinning remove
the DRUPAL_VERSION parameter and related echo to avoid a no-op argument; ensure
you reference the DRUPAL_VERSION variable (and keep VARIANT handling unchanged).
| docker compose exec -T $SERVICE sh -c "cd ${WEBROOT} && vendor/bin/drush site:install minimal \ | ||
| --db-url="sqlite://localhost/sites/default/files/.ht.sqlite" \ | ||
| --site-name="Drupal Test Site" \ | ||
| --account-name=admin \ | ||
| --account-pass=admin \ | ||
| --yes \ | ||
| --no-interaction" |
There was a problem hiding this comment.
Critical: Nested double quotes break the sh -c argument, dropping flags.
The inner double quotes on lines 54–55 ("sqlite://..." and "Drupal Test Site") prematurely close/reopen the outer sh -c "..." string. After shell word-splitting, sh -c receives only a partial command as its first argument — everything after the unquoted space in Drupal Test Site is lost (including --account-name, --account-pass, --yes, and --no-interaction).
Without --yes, drush will prompt for confirmation and likely fail/hang under -T (no TTY). The site name would also be truncated to just "Drupal".
Shellcheck (SC2140) flagged this same issue.
Proposed fix — use single quotes for inner values
docker compose exec -T $SERVICE sh -c "cd ${WEBROOT} && vendor/bin/drush site:install minimal \
- --db-url="sqlite://localhost/sites/default/files/.ht.sqlite" \
- --site-name="Drupal Test Site" \
+ --db-url='sqlite://localhost/sites/default/files/.ht.sqlite' \
+ --site-name='Drupal Test Site' \
--account-name=admin \
--account-pass=admin \
--yes \
--no-interaction"🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 54-54: Word is of the form "A"B"C" (B indicated). Did you mean "ABC" or "A"B"C"?
(SC2140)
🤖 Prompt for AI Agents
In `@tests/install-drupal.sh` around lines 53 - 59, The sh -c string contains
nested double quotes that break the argument parsing for the drush command;
update the docker compose exec invocation (the line building the sh -c argument
that cd's to ${WEBROOT} and runs vendor/bin/drush site:install) so that inner
string literals (site DB URL and --site-name value) are either single-quoted or
properly escaped to avoid closing the outer double-quoted sh -c argument; ensure
the full command (including --account-name, --account-pass, --yes,
--no-interaction) is passed as a single argument to sh -c and verify variables
SERVICE and WEBROOT remain unchanged.
1 participant
This commit introduces a new GitHub Actions workflow (
test-drupal.yml) designed to test Drupal installations. It also modifies the existingdocker-buildx.ymlworkflow to be triggered by the successful completion of the Drupal testing workflow when the target branch ismain.Key changes include:
test-drupal.ymlworkflow:docker-buildx.ymlworkflow:workflow_runtrigger that activates after the "Test Drupal Installation" workflow completes successfully on themainbranch.pushandpull_requesttriggers onmainare still present for direct pushes.mainbranch.tests/docker-compose.ymlandtests/docker-compose.fpm.ymlfor defining test environments.tests/install-drupal.shfor automating Drupal installation.tests/verify-drupal.shfor performing post-installation verification.tests/nginx.conffor Nginx configuration within the test environment.Summary by CodeRabbit
New Features
CI/CD Improvements