Copilot commented on code in PR #18010:
URL: https://github.com/apache/pinot/pull/18010#discussion_r3004424168
##########
.github/workflows/scripts/docker/.superset_docker_image_build_and_push.sh:
##########
@@ -51,9 +51,12 @@ cd ${DOCKER_FILE_BASE_DIR}
docker build \
--no-cache \
- --platform=${BUILD_PLATFORM} \
--file Dockerfile \
--build-arg SUPERSET_IMAGE_TAG=${SUPERSET_IMAGE_TAG} \
${DOCKER_BUILD_TAGS} \
- --push \
.
+
+for tag in "${tags[@]}"
+do
+ docker push ${DOCKER_IMAGE_NAME}:${tag}
+done
Review Comment:
The script now pushes multiple tags in a loop but does not enable `set -e`
(or otherwise check command exit codes). A failed `docker push` can be masked
if a later push succeeds, causing the workflow to pass without publishing all
intended tags. Consider enabling fail-fast (`set -e` / `set -euo pipefail`) or
explicitly checking each push result and exiting non-zero on failure.
##########
.github/workflows/build-pinot-base-docker-image.yml:
##########
@@ -42,8 +42,6 @@ jobs:
with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}
- - uses: docker/setup-buildx-action@v4
- name: Set up Docker Buildx
- uses: actions/checkout@v5
Review Comment:
This workflow still references `docker/login-action@v4` by tag. Since
`docker/*` is a third-party namespace, it should be pinned to an
ASF-allowlisted commit SHA (per this PR’s stated goal).
##########
.github/workflows/build-pinot-docker-image.yml:
##########
@@ -51,10 +51,6 @@ jobs:
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
Review Comment:
This workflow still references `docker/login-action@v4` by tag. Since
`docker/*` is a third-party namespace, it should be pinned to an
ASF-allowlisted commit SHA (per this PR’s stated goal).
##########
.github/workflows/scripts/docker/.pinot_docker_image_build_and_push.sh:
##########
@@ -59,11 +55,14 @@ done
cd ${DOCKER_FILE_BASE_DIR}
-docker buildx build \
+docker build \
--no-cache \
- --platform=${BUILD_PLATFORM} \
--file Dockerfile \
--build-arg PINOT_GIT_URL=${PINOT_GIT_URL} --build-arg
PINOT_BRANCH=${PINOT_BRANCH} \
${DOCKER_BUILD_TAGS} \
- --push \
.
+
+for tag in "${tags[@]}"
+do
+ docker push ${DOCKER_IMAGE_NAME}:${tag}
+done
Review Comment:
The script now pushes multiple tags in a loop but does not enable `set -e`
(or otherwise check command exit codes). A failed `docker push` can be masked
if a later push succeeds, causing the workflow to pass without publishing all
intended tags. Consider enabling fail-fast (`set -e` / `set -euo pipefail`) or
explicitly checking each push result and exiting non-zero on failure.
##########
.github/workflows/build-superset-docker-image.yml:
##########
@@ -47,10 +47,6 @@ jobs:
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
Review Comment:
This workflow still references `docker/login-action@v4` by tag. Since
`docker/*` is a third-party namespace, it should be pinned to an
ASF-allowlisted commit SHA (per this PR’s stated goal).
##########
.github/workflows/scripts/docker/.pinot_base_docker_image_build_and_push.sh:
##########
@@ -30,11 +30,11 @@ fi
cd docker/images/pinot-base/pinot-base-${BASE_IMAGE_TYPE}
-docker buildx build \
+docker build \
--no-cache \
- --platform=${BUILD_PLATFORM} \
--file ${OPEN_JDK_DIST}.dockerfile \
--tag apachepinot/pinot-base-${BASE_IMAGE_TYPE}:${TAG} \
--build-arg JAVA_VERSION=${JDK_VERSION:-11} \
Review Comment:
`BUILD_PLATFORM` is still required (`exit 1` if unset) but is no longer used
in the `docker build` command after switching away from buildx/platform builds.
Either remove the `BUILD_PLATFORM` requirement or pass it to `docker build` so
the env var remains meaningful.
##########
.github/workflows/build-multi-arch-pinot-docker-image.yml:
##########
@@ -73,8 +73,6 @@ jobs:
with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}
Review Comment:
This workflow still references `docker/login-action@v4` by tag. Since
`docker/*` is a third-party namespace, it should be pinned to an
ASF-allowlisted commit SHA (per this PR’s stated goal). Update this usage (and
any other `docker/login-action` occurrences in this workflow) to the approved
SHA.
--
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]
