Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a basic smoke test to the pull request CI workflow. After successfully building the Docker image, the new steps start the container, poll the HTTP endpoint until it's ready (up to 60 seconds), verify that four required PHP extensions are loaded, and clean up the container regardless of outcome. This complements the existing lint and build checks to give earlier signal that the produced image is functional.
Changes:
- The build step now loads the image locally (changed from
load: falsetoload: true) and assigns a tag so it can be referenced by later steps. - A "Smoke test image" step runs the container, polls the HTTP endpoint with retries, and checks that
gd,ldap,mysqli, andtimezonedbPHP modules are present. - "Dump smoke logs on failure" and "Cleanup smoke container" housekeeping steps are added.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| sleep 2 | ||
| done | ||
|
|
||
| docker exec librebooking-smoke php -m | grep -E '^(gd|ldap|mysqli|timezonedb)$' |
There was a problem hiding this comment.
The grep -E '^(gd|ldap|mysqli|timezonedb)$' command only checks that at least one of the four PHP modules appears in the output. grep exits 0 as long as any single match is found, so the check would pass even if three of the four modules are missing.
To verify that all four modules are present, the count of matching lines should be confirmed to equal 4. For example, pipe to wc -l and compare against 4, or run four separate grep commands (one per module), or use grep -c and verify the result equals 4.
| docker exec librebooking-smoke php -m | grep -E '^(gd|ldap|mysqli|timezonedb)$' | |
| modules_count="$(docker exec librebooking-smoke php -m | grep -E '^(gd|ldap|mysqli|timezonedb)$' | wc -l)" | |
| if [ "${modules_count}" -ne 4 ]; then | |
| echo "Smoke test failed: expected 4 required PHP modules (gd, ldap, mysqli, timezonedb), but found ${modules_count}" >&2 | |
| docker exec librebooking-smoke php -m || true | |
| exit 1 | |
| fi |
|
|
||
| if [ "${attempt}" = "30" ]; then | ||
| echo "Smoke test failed: HTTP endpoint did not become ready" >&2 | ||
| docker logs librebooking-smoke || true |
There was a problem hiding this comment.
The "Dump smoke logs on failure" step (lines 76–78) is redundant because the smoke test step already runs docker logs librebooking-smoke inline at line 67 before calling exit 1. Container logs will be printed twice in the failure case that the HTTP endpoint does not become ready. Consider removing the inline docker logs call at line 67 (inside the smoke test step) and relying solely on the dedicated "Dump smoke logs on failure" step, which will also catch failures from the docker exec … grep command at line 74.
| docker logs librebooking-smoke || true |
fae5977 to
49ed359
Compare
This runs the docker image and attempts to connect to it.
49ed359 to
edb6afd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| name: Cleanup smoke container | ||
| if: always() | ||
| run: | | ||
| docker rm --force librebooking-smoke >/dev/null 2>&1 || true |
There was a problem hiding this comment.
The librebooking-smoke-src container (created on line 88) is only removed inline on line 90 within the smoke test step. If docker cp (line 89) or any subsequent command before line 90 fails — which is possible since set -euo pipefail is active — the script exits early and the container is never removed. The Cleanup smoke container step (lines 140–144) only removes librebooking-smoke and does not clean up librebooking-smoke-src. Consider adding docker rm --force librebooking-smoke-src to the cleanup step to handle this case.
| docker rm --force librebooking-smoke >/dev/null 2>&1 || true | |
| docker rm --force librebooking-smoke >/dev/null 2>&1 || true | |
| docker rm --force librebooking-smoke-src >/dev/null 2>&1 || true |
| for attempt in $(seq 1 20); do | ||
| if docker exec "${mysql_service_id}" mysqladmin ping --host 127.0.0.1 --user root --password=root --silent; then | ||
| break | ||
| fi | ||
|
|
||
| if [ "${attempt}" = "20" ]; then | ||
| echo "Smoke test failed: MySQL did not become ready" >&2 | ||
| docker logs "${mysql_service_id}" || true | ||
| exit 1 | ||
| fi | ||
|
|
||
| sleep 2 | ||
| done | ||
|
|
There was a problem hiding this comment.
The check_build job already configures the MySQL service with health check options (--health-cmd, --health-interval, --health-timeout, --health-retries). In GitHub Actions, the runner waits for a service container's health check to pass before executing the job's steps. Therefore the manual MySQL readiness polling loop (lines 74–86) is redundant and can be removed.
| for attempt in $(seq 1 20); do | |
| if docker exec "${mysql_service_id}" mysqladmin ping --host 127.0.0.1 --user root --password=root --silent; then | |
| break | |
| fi | |
| if [ "${attempt}" = "20" ]; then | |
| echo "Smoke test failed: MySQL did not become ready" >&2 | |
| docker logs "${mysql_service_id}" || true | |
| exit 1 | |
| fi | |
| sleep 2 | |
| done |
There was a problem hiding this comment.
Apparently Copilot tells you that the waiting loop is useless...
This runs the docker image and attempts to connect to it.