Copilot commented on code in PR #2790:
URL: https://github.com/apache/tika/pull/2790#discussion_r3148047911
##########
.github/workflows/docker-snapshot.yml:
##########
@@ -89,6 +115,32 @@ jobs:
tar xzf
"tika-server/tika-server-standard/target/tika-server-standard-${TIKA_VERSION}-bin.tgz"
-C "${OUT_DIR}/tika-server"
cp "tika-server/docker-build/full/Dockerfile.snapshot"
"${OUT_DIR}/Dockerfile"
+ - name: Build tika-server full image for smoke test
+ uses:
docker/build-push-action@10e90e3645eae34f1e60eeb005ba3a3d33f178e8 # v6.19.2
+ with:
+ context: target/tika-server-full-docker
+ platforms: linux/amd64
+ load: true
+ build-args: |
+ TIKA_VERSION=${{ steps.version.outputs.tika_version }}
+ tags: tika-server-full-smoke:ci
+
+ - name: Smoke test tika-server full image
+ run: |
+ docker run -d --name tika-server-full-smoke -p 9999:9998
tika-server-full-smoke:ci
+ for i in $(seq 1 20); do
+ if docker logs tika-server-full-smoke 2>&1 | grep -q "Started
Apache Tika server"; then
+ echo "tika-server full started successfully"
+ docker stop tika-server-full-smoke
+ exit 0
+ fi
+ sleep 2
+ done
+ echo "ERROR: tika-server full did not start within 40 seconds"
+ docker logs tika-server-full-smoke
+ docker stop tika-server-full-smoke
Review Comment:
Same issue as above: `docker stop tika-server-full-smoke` can fail under
`bash -e` if the container has already exited, preventing log
collection/cleanup. Use `docker rm -f` (or guard `docker stop`) in a trap to
ensure consistent cleanup and clearer failures.
```suggestion
cleanup() {
status=$?
if [ "$status" -ne 0 ]; then
docker logs tika-server-full-smoke || true
fi
docker rm -f tika-server-full-smoke >/dev/null 2>&1 || true
exit "$status"
}
docker run -d --name tika-server-full-smoke -p 9999:9998
tika-server-full-smoke:ci
trap cleanup EXIT
for i in $(seq 1 20); do
if docker logs tika-server-full-smoke 2>&1 | grep -q "Started
Apache Tika server"; then
echo "tika-server full started successfully"
exit 0
fi
sleep 2
done
echo "ERROR: tika-server full did not start within 40 seconds"
```
##########
.github/workflows/docker-snapshot.yml:
##########
@@ -135,6 +190,32 @@ jobs:
cp "tika-grpc/docker-build/start-tika-grpc.sh" "${OUT_DIR}/bin/"
cp "tika-grpc/docker-build/Dockerfile" "${OUT_DIR}/Dockerfile"
+ - name: Build tika-grpc image for smoke test
+ uses:
docker/build-push-action@10e90e3645eae34f1e60eeb005ba3a3d33f178e8 # v6.19.2
+ with:
+ context: target/tika-grpc-docker
+ platforms: linux/amd64
+ load: true
+ build-args: |
+ VERSION=${{ steps.version.outputs.tika_version }}
+ tags: tika-grpc-smoke:ci
+
+ - name: Smoke test tika-grpc image
+ run: |
+ docker run -d --name tika-grpc-smoke -p 9090:9090 tika-grpc-smoke:ci
+ for i in $(seq 1 15); do
+ if docker logs tika-grpc-smoke 2>&1 | grep -q "Server started,
listening on"; then
+ echo "tika-grpc started successfully"
+ docker stop tika-grpc-smoke
+ exit 0
+ fi
+ sleep 2
+ done
+ echo "ERROR: tika-grpc did not start within 30 seconds"
+ docker logs tika-grpc-smoke
+ docker stop tika-grpc-smoke
Review Comment:
Same robustness concern here: if the container exits quickly, `docker stop
tika-grpc-smoke` may error under `bash -e` and mask the real startup failure.
Prefer `docker rm -f` (or guard the stop) and consider always printing `docker
ps -a`/exit code on timeout to aid debugging.
```suggestion
docker rm -f tika-grpc-smoke >/dev/null 2>&1 || true
exit 0
fi
sleep 2
done
echo "ERROR: tika-grpc did not start within 30 seconds"
docker logs tika-grpc-smoke || true
docker ps -a --filter "name=^tika-grpc-smoke$" || true
docker inspect -f '{{.State.ExitCode}}' tika-grpc-smoke || true
docker rm -f tika-grpc-smoke >/dev/null 2>&1 || true
```
##########
tika-grpc/pom.xml:
##########
@@ -474,7 +474,7 @@
<manifest>
<mainClass>org.apache.tika.pipes.grpc.TikaGrpcServer</mainClass>
<addClasspath>true</addClasspath>
- <classpathPrefix>lib/</classpathPrefix>
+ <classpathPrefix>tika-grpc/</classpathPrefix>
Review Comment:
Changing the jar manifest Class-Path prefix to `tika-grpc/` will break the
existing tika-grpc binary assembly: `tika-grpc/src/main/assembly/assembly.xml`
places runtime dependencies under `lib/`, so `java -jar` from the assembled zip
will no longer find its dependencies. Either keep `classpathPrefix` as `lib/`
and adjust the Docker image to copy deps into a `lib/` subdirectory next to the
jar, or update the assembly descriptor outputDirectory to match the new
`tika-grpc/` layout.
```suggestion
<classpathPrefix>lib/</classpathPrefix>
```
##########
.github/workflows/docker-snapshot.yml:
##########
@@ -69,6 +69,32 @@ jobs:
tar xzf
"tika-server/tika-server-standard/target/tika-server-standard-${TIKA_VERSION}-bin.tgz"
-C "${OUT_DIR}/tika-server"
cp "tika-server/docker-build/minimal/Dockerfile.snapshot"
"${OUT_DIR}/Dockerfile"
+ - name: Build tika-server minimal image for smoke test
+ uses:
docker/build-push-action@10e90e3645eae34f1e60eeb005ba3a3d33f178e8 # v6.19.2
+ with:
+ context: target/tika-server-minimal-docker
+ platforms: linux/amd64
+ load: true
+ build-args: |
+ TIKA_VERSION=${{ steps.version.outputs.tika_version }}
+ tags: tika-server-minimal-smoke:ci
+
+ - name: Smoke test tika-server minimal image
+ run: |
+ docker run -d --name tika-server-minimal-smoke -p 9998:9998
tika-server-minimal-smoke:ci
+ for i in $(seq 1 20); do
+ if docker logs tika-server-minimal-smoke 2>&1 | grep -q "Started
Apache Tika server"; then
+ echo "tika-server minimal started successfully"
+ docker stop tika-server-minimal-smoke
+ exit 0
+ fi
+ sleep 2
+ done
+ echo "ERROR: tika-server minimal did not start within 40 seconds"
+ docker logs tika-server-minimal-smoke
+ docker stop tika-server-minimal-smoke
Review Comment:
This smoke test step runs under `bash -e`, so `docker stop
tika-server-minimal-smoke` can fail (and fail the job) if the container exits
early or isn’t running. Consider using a cleanup trap and `docker rm -f` (or
`docker stop ... || true` followed by `docker rm ... || true`) so the job
reliably collects logs and cleans up even on failure.
```suggestion
cleanup() {
docker logs tika-server-minimal-smoke 2>&1 || true
docker rm -f tika-server-minimal-smoke >/dev/null 2>&1 || true
}
trap cleanup EXIT
docker run -d --name tika-server-minimal-smoke -p 9998:9998
tika-server-minimal-smoke:ci
for i in $(seq 1 20); do
if docker logs tika-server-minimal-smoke 2>&1 | grep -q "Started
Apache Tika server"; then
echo "tika-server minimal started successfully"
exit 0
fi
sleep 2
done
echo "ERROR: tika-server minimal did not start within 40 seconds"
```
##########
.github/workflows/docker-release.yml:
##########
@@ -112,9 +112,12 @@ jobs:
TIKA_VERSION="${{ steps.version.outputs.tag }}"
OUT_DIR=target/tika-grpc-docker
- mkdir -p "${OUT_DIR}/libs" "${OUT_DIR}/plugins" "${OUT_DIR}/config"
"${OUT_DIR}/bin"
+ mkdir -p "${OUT_DIR}/libs/tika-grpc" "${OUT_DIR}/plugins"
"${OUT_DIR}/config" "${OUT_DIR}/bin"
cp "tika-grpc/target/tika-grpc-${TIKA_VERSION}.jar"
"${OUT_DIR}/libs/"
+ mvn -pl tika-grpc dependency:copy-dependencies \
+ -DoutputDirectory="${PWD}/${OUT_DIR}/libs/tika-grpc" \
+ -DincludeScope=runtime -q -B
Review Comment:
The PR description says smoke tests were added before the final multi-arch
push for all three images, but `docker-release.yml` currently has no smoke test
steps (only snapshot does). Either update the release workflow to include the
same smoke testing, or adjust the PR description to reflect that smoke tests
are snapshot-only.
--
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]
