Copilot commented on code in PR #17997:
URL: https://github.com/apache/pinot/pull/17997#discussion_r2999387855
##########
.github/workflows/build-pinot-base-docker-image.yml:
##########
@@ -25,26 +25,30 @@ on:
jobs:
build-pinot-build-docker-image:
name: Build Pinot Base Docker Image
- runs-on: ubuntu-latest
+ runs-on: ${{ matrix.runner }}
strategy:
matrix:
baseImageType: [ "build", "runtime" ]
openJdkDist: [ "amazoncorretto", "ms-openjdk" ]
+ arch: [ "amd64", "arm64" ]
+ include:
+ - arch: amd64
+ runner: ubuntu-24.04
+ - arch: arm64
+ runner: ubuntu-24.04-arm
Review Comment:
The matrix `include` block here will add two extra matrix jobs (with only
`arch`/`runner`) rather than enriching the existing `baseImageType` ×
`openJdkDist` × `arch` combinations. As a result, most jobs will have
`matrix.runner` unset (breaking `runs-on`) and the extra jobs will have
`openJdkDist` / `baseImageType` undefined.
Consider mapping `arch` -> `runner` in the `runs-on` expression, or switch
to an `include`-only matrix that enumerates `baseImageType`, `openJdkDist`,
`arch`, and `runner` for each combination.
```suggestion
runs-on: ${{ matrix.arch == 'arm64' && 'ubuntu-24.04-arm' ||
'ubuntu-24.04' }}
strategy:
matrix:
baseImageType: [ "build", "runtime" ]
openJdkDist: [ "amazoncorretto", "ms-openjdk" ]
arch: [ "amd64", "arm64" ]
```
##########
.github/workflows/build-pinot-base-docker-image.yml:
##########
@@ -25,26 +25,30 @@ on:
jobs:
build-pinot-build-docker-image:
name: Build Pinot Base Docker Image
- runs-on: ubuntu-latest
+ runs-on: ${{ matrix.runner }}
strategy:
matrix:
baseImageType: [ "build", "runtime" ]
openJdkDist: [ "amazoncorretto", "ms-openjdk" ]
+ arch: [ "amd64", "arm64" ]
+ include:
+ - arch: amd64
+ runner: ubuntu-24.04
+ - arch: arm64
+ runner: ubuntu-24.04-arm
steps:
- name: Login to DockerHub
uses: docker/login-action@v4
with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}
- - uses: docker/setup-qemu-action@v4
- name: Set up QEMU
- uses: docker/setup-buildx-action@v4
name: Set up Docker Buildx
- uses: actions/checkout@v5
- name: Build and push the Docker image
env:
OPEN_JDK_DIST: ${{ matrix.openJdkDist }}
BASE_IMAGE_TYPE: ${{ matrix.baseImageType }}
- BUILD_PLATFORM: "linux/amd64,linux/arm64"
+ BUILD_PLATFORM: "linux/${{ matrix.arch }}"
TAG: "11-${{ matrix.openJdkDist }}"
run:
.github/workflows/scripts/docker/.pinot_base_docker_image_build_and_push.sh
Review Comment:
Building a single platform per job but pushing to the same tag
(`apachepinot/pinot-base-${BASE_IMAGE_TYPE}:${TAG}`) will cause the amd64 and
arm64 jobs to race/overwrite each other, and it will no longer produce a
multi-arch manifest for that tag. This breaks downstream Dockerfiles like
`docker/images/pinot/Dockerfile.build` that expect
`apachepinot/pinot-base-build:11-...` to be multi-arch.
To keep native builds, push arch-suffixed tags per job (e.g., add
`-${platformTag}`) and add a follow-up manifest job (using `docker manifest
create/push`) to publish the multi-arch tag, similar to the
`create-multi-arch-manifest` job in the other workflow.
```suggestion
TAG: "11-${{ matrix.openJdkDist }}-${{ matrix.arch }}"
run:
.github/workflows/scripts/docker/.pinot_base_docker_image_build_and_push.sh
create-multi-arch-manifest:
name: Create multi-arch manifest for Pinot Base Docker Image
runs-on: ubuntu-24.04
needs: build-pinot-build-docker-image
strategy:
matrix:
baseImageType: [ "build", "runtime" ]
openJdkDist: [ "amazoncorretto", "ms-openjdk" ]
steps:
- name: Login to DockerHub
uses: docker/login-action@v4
with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}
- name: Create and push multi-arch manifest
run: |
BASE_IMAGE_TYPE="${{ matrix.baseImageType }}"
OPEN_JDK_DIST="${{ matrix.openJdkDist }}"
BASE_TAG="11-${OPEN_JDK_DIST}"
docker manifest create
"apachepinot/pinot-base-${BASE_IMAGE_TYPE}:${BASE_TAG}" \
--amend
"apachepinot/pinot-base-${BASE_IMAGE_TYPE}:${BASE_TAG}-amd64" \
--amend
"apachepinot/pinot-base-${BASE_IMAGE_TYPE}:${BASE_TAG}-arm64"
docker manifest push
"apachepinot/pinot-base-${BASE_IMAGE_TYPE}:${BASE_TAG}"
```
##########
.github/workflows/build-multi-arch-pinot-docker-image.yml:
##########
@@ -55,11 +55,16 @@ jobs:
run: |
.github/workflows/scripts/docker/.pinot_build_info_gen.sh
build-pinot-docker-image:
- runs-on: ubuntu-latest
+ runs-on: ${{ matrix.runner }}
strategy:
matrix:
arch: [ "amd64", "arm64" ]
base-image-tag: [ "11-amazoncorretto", "11-ms-openjdk" ]
+ include:
+ - arch: amd64
+ runner: ubuntu-24.04
+ - arch: arm64
+ runner: ubuntu-24.04-arm
Review Comment:
The matrix `include` section here doesn't attach `runner` to the existing
`arch` × `base-image-tag` combinations; it adds *additional* matrix rows. That
leaves `matrix.runner` unset for the main combinations (breaking `runs-on`),
and also creates extra jobs where `matrix.base-image-tag` is empty/undefined.
Use a deterministic mapping from `arch` -> `runner` (e.g., `runs-on: ${{
fromJSON('{"amd64":"ubuntu-24.04","arm64":"ubuntu-24.04-arm"}')[matrix.arch]
}}`), or expand the matrix to an `include`-only list that enumerates `arch`,
`base-image-tag`, and `runner` together for each combination.
```suggestion
runs-on: ${{
fromJSON('{"amd64":"ubuntu-24.04","arm64":"ubuntu-24.04-arm"}')[matrix.arch] }}
strategy:
matrix:
arch: [ "amd64", "arm64" ]
base-image-tag: [ "11-amazoncorretto", "11-ms-openjdk" ]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
