Skip to content

ci: add a basic smoke test check #174

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
JohnVillalovos wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

ci: add a basic smoke test check #174

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 95 additions & 1 deletion .github/workflows/build_pull_request.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,21 @@ jobs:

check_build:
runs-on: ubuntu-latest
services:
mysql:
image: mysql:8.0
env:
MYSQL_ROOT_PASSWORD: root
MYSQL_DATABASE: librebooking
MYSQL_USER: librebooking
MYSQL_PASSWORD: librebooking
ports:
- 3306:3306
options: >-
--health-cmd="mysqladmin ping -h 127.0.0.1 -proot"
--health-interval=10s
--health-timeout=5s
--health-retries=5
steps:
-
name: Checkout github repository
Expand Down Expand Up @@ -46,5 +61,84 @@ jobs:
VERSION_COMPOSER=lts
APP_GH_REF=${{ steps.pr_meta.outputs.appGitRef }}
APP_GH_ADD_SHA=${{ steps.pr_meta.outputs.appAddSha }}
load: false
tags: librebooking-pr:${{ github.sha }}
load: true
push: false
-
name: Smoke test image
run: |
set -euo pipefail
image_tag="librebooking-pr:${GITHUB_SHA}"
mysql_service_id="${{ job.services.mysql.id }}"

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

Comment on lines +74 to +87
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@colisee colisee Mar 9, 2026

Choose a reason for hiding this comment

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

Apparently Copilot tells you that the waiting loop is useless...

docker create --name librebooking-smoke-src "${image_tag}" >/dev/null
docker cp librebooking-smoke-src:/var/www/html/database_schema /tmp/librebooking-database_schema
docker rm librebooking-smoke-src

cat \
/tmp/librebooking-database_schema/create-db.sql \
/tmp/librebooking-database_schema/create-schema.sql \
| docker exec --interactive "${mysql_service_id}" mysql --user root --password=root

docker run --rm \
--add-host host.docker.internal:host-gateway \
"${image_tag}" \
php /var/www/html/phing-tasks/UpgradeDbTask.php \
librebooking \
librebooking \
host.docker.internal \
librebooking \
/var/www/html/database_schema

docker run --detach \
--name librebooking-smoke \
--add-host host.docker.internal:host-gateway \
--publish 18080:8080 \
--env LB_DATABASE_HOSTSPEC=host.docker.internal \
--env LB_DATABASE_NAME=librebooking \
--env LB_DATABASE_USER=librebooking \
--env LB_DATABASE_PASSWORD=librebooking \
"${image_tag}"

for attempt in $(seq 1 30); do
if curl --silent --show-error --fail --max-time 2 http://127.0.0.1:18080/Web/; then
echo "Connected to docker image successfully"
break
fi

if [ "${attempt}" = "30" ]; then
echo "Smoke test failed: HTTP endpoint did not become ready" >&2
docker logs librebooking-smoke || true
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
docker logs librebooking-smoke || true

Copilot uses AI. Check for mistakes.
exit 1
fi

sleep 2
done

docker exec librebooking-smoke php -m | grep -E '^(gd|ldap|mysqli|timezonedb)$'
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
-
name: Dump smoke logs on failure
if: failure()
run: |
docker logs librebooking-smoke || true
docker logs "${{ job.services.mysql.id }}" || true
-
name: Cleanup smoke container
if: always()
run: |
docker rm --force librebooking-smoke >/dev/null 2>&1 || true
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
rm -rf /tmp/librebooking-database_schema