Skip to content

fix: Fix test workflow and add more platforms #34

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

Open
M4dhav wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Fix test workflow and add more platforms #34

Show file tree
Hide file tree
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
29 changes: 23 additions & 6 deletions .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ jobs:
tests:
name: Unit & E2E
runs-on: ubuntu-latest
strategy:
matrix:
platform: [amd64, arm64, arm/v7, ppc64le]
Comment on lines +8 to +10
Copy link
Author

@M4dhav M4dhav Dec 6, 2025

Choose a reason for hiding this comment

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

Added parallel running and more platforms to the workflow


env:
PLATFORM: ${{ matrix.platform == 'arm/v7' && 'armv7' || matrix.platform }}
Comment on lines +12 to +13
Copy link
Author

@M4dhav M4dhav Dec 6, 2025

Choose a reason for hiding this comment

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

Setting an env for use by docker-compose

Copy link
Member

@ChiragAgg5k ChiragAgg5k Dec 6, 2025

Choose a reason for hiding this comment

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

should matrix platform just be armv7 so we dont have to add this check for it?

Copy link
Author

@M4dhav M4dhav Dec 6, 2025

Choose a reason for hiding this comment

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

The platform param under the build-push-action expects linux/arm/v7 and will not work with armv7.

We could set it as armv7 and add a check there to set it as arm/v7 if that would be preferred

Copy link
Member

@ChiragAgg5k ChiragAgg5k Dec 6, 2025

Choose a reason for hiding this comment

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

err ok, this is fine for now


steps:
- name: Checkout repository
Expand All @@ -14,19 +20,27 @@ jobs:
submodules: recursive

- run: git checkout HEAD^2

- name: Install dependencies
run: composer install --ignore-platform-reqs --no-interaction --prefer-dist
Comment on lines +24 to +25
Copy link
Author

@M4dhav M4dhav Dec 6, 2025

Choose a reason for hiding this comment

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

Installing composer dependencies before is necessary because of how the container volumes are mounted by docker-compose



- name: Set up QEMU
uses: docker/setup-qemu-action@v3

Comment on lines +28 to +30
Copy link
Author

@M4dhav M4dhav Dec 6, 2025

Choose a reason for hiding this comment

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

Setting up Qemu to run other platforms on an amd64 machine

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v2

- name: Build image
- name: Build image for ${{ matrix.platform }}
uses: docker/build-push-action@v3
with:
context: .
push: false
tags: database-dev
tags: database-dev:${{ matrix.platform == 'arm/v7' && 'armv7' || matrix.platform }}
Copy link
Author

@M4dhav M4dhav Dec 6, 2025

Choose a reason for hiding this comment

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

Tags cannot have slashes so added a special check

load: true
cache-from: type=gha
cache-to: type=gha,mode=max
platforms: linux/${{ matrix.platform }}
Comment on lines +34 to +43
Copy link
Author

@M4dhav M4dhav Dec 6, 2025

Choose a reason for hiding this comment

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

Conditionally spin up instances for specific architectures


- name: Start Containers
run: |
Expand All @@ -35,9 +49,12 @@ jobs:

- name: Run Tests
run: |
arch=$(uname -m)
if [ "${arch}" = "amd64" ]; then
if [[ "${{ matrix.platform }}" == "amd64" ]]; then
docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml --testsuite X86;
elif [ "${arch}" = "arm64" ] || [ "${arch}" = "aarch64" ]; then
docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml --testsuite ARM64;
elif [[ "${{ matrix.platform }}" == "arm64" ]]; then
docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml --testsuite ARM64;
elif [[ "${{ matrix.platform }}" == "arm/v7" ]]; then
docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml --testsuite ARMV7;
elif [[ "${{ matrix.platform }}" == "ppc64le" ]]; then
docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml --testsuite PPC;
fi
1 change: 1 addition & 0 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ services:
tests:
build:
context: .
image: database-dev:${PLATFORM}
Copy link
Author

@M4dhav M4dhav Dec 6, 2025

Choose a reason for hiding this comment

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

Necessary to ensure docker always builds the container with the right architecture

Copy link
Member

@ChiragAgg5k ChiragAgg5k Dec 6, 2025

Choose a reason for hiding this comment

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

how is PLATFORM set here? should there be a fallback?

Copy link
Author

@M4dhav M4dhav Dec 6, 2025

Choose a reason for hiding this comment

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

compose fetches PLATFORM from env variables, which are set in the workflow. No matter what strategy is running, an env var will aways be set, so the fallback is intrinsic. The same var is used as the tag while building the containers so the compose step will never fail

Copy link
Member

@ChiragAgg5k ChiragAgg5k Dec 6, 2025

Choose a reason for hiding this comment

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

what if i run docker compose locally

Copy link
Author

@M4dhav M4dhav Dec 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You will have to set the env var and build the container with the correct tag. If you would prefer another approach, I can duplicate the compose and make one specifically for CI that is used in the CI and keep the default compose unchanged

Copy link
Member

@ChiragAgg5k ChiragAgg5k Dec 6, 2025

Choose a reason for hiding this comment

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

yeah either works, as long as we dont break local setup without extra steps

mem_limit: 512m
mem_reservation: 128M
cpus: 0.5
Expand Down