Skip to content
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
4f23c7b
Init
Jul 4, 2025
48dff13
Temporaryily remove all other workflows for faster testing
Jul 4, 2025
9bd9207
Try fix
Jul 4, 2025
4997a02
Merge branch 'release-0.293-clp-connector' into xwei/add-release-imag…
anlowee Jul 7, 2025
d74b885
Fix
Jul 7, 2025
4815351
Fix
Jul 7, 2025
e7fe20c
Still build the runtime image when updating the PR, but only publish …
Jul 7, 2025
fc2cc18
Build worker image
Jul 8, 2025
cc2c510
Merge branch 'release-0.293-clp-connector' into xwei/add-release-imag…
anlowee Jul 8, 2025
60dd19d
Restore workflows
Jul 9, 2025
a534bec
Remove the uncessary action
Jul 10, 2025
9e17bc0
Use docker build-and-push action
Jul 14, 2025
aeb4e48
Merge branch 'release-0.293-clp-connector' into xwei/add-release-imag…
anlowee Jul 14, 2025
dbb28fa
Fix
Jul 14, 2025
1e69681
Fix
Jul 14, 2025
4ca014c
Fix
Jul 14, 2025
fec665a
Fix
Jul 14, 2025
20a7054
Merge branch 'release-0.293-clp-connector' into xwei/add-release-imag…
anlowee Jul 15, 2025
283b751
Fix
Jul 15, 2025
64983f0
Merge branch 'xwei/add-release-image-publish-workflow' of github.com:…
Jul 15, 2025
a6ebbaa
Fix
Jul 15, 2025
5ca0420
Apply suggestions from code review
anlowee Jul 18, 2025
fa06d23
Address comments
Jul 18, 2025
d120672
Merge branch 'release-0.293-clp-connector' into xwei/add-release-imag…
anlowee Jul 18, 2025
4c90aad
Merge branch 'release-0.293-clp-connector' into xwei/add-release-imag…
anlowee Jul 18, 2025
3b76c12
Address comments
Jul 21, 2025
b55aeda
Merge branch 'xwei/add-release-image-publish-workflow' of github.com:…
Jul 21, 2025
74787c7
Remove TODO
Jul 21, 2025
313a648
Address comments
Jul 21, 2025
47b9c10
Merge branch 'release-0.293-clp-connector' into xwei/add-release-imag…
anlowee Jul 21, 2025
ceaa809
Minor refactoring.
kirkrodrigues Jul 21, 2025
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
67 changes: 67 additions & 0 deletions .github/workflows/maven-checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,72 @@ jobs:
run: |
export MAVEN_OPTS="${MAVEN_INSTALL_OPTS}"
./mvnw install -B -V -T 1C -DskipTests -Dmaven.javadoc.skip=true --no-transfer-progress -P ci -pl '!presto-test-coverage,!:presto-docs'
- name: Upload presto-server
if: matrix.java == '8.0.442'
uses: actions/upload-artifact@v4
with:
name: presto-server
path: presto-server/target/presto-server-0.293.tar.gz
if-no-files-found: "error"
retention-days: 1
- name: Upload presto-cli
if: matrix.java == '8.0.442'
uses: actions/upload-artifact@v4
with:
name: presto-cli
path: presto-cli/target/presto-cli-0.293-executable.jar
if-no-files-found: "error"
retention-days: 1
- name: Clean Maven Output
run: ./mvnw clean -pl '!:presto-server,!:presto-cli,!presto-test-coverage'
presto-coordinator-with-clp-connector-runtime-image:
Copy link
Member

Choose a reason for hiding this comment

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

How about presto-coordinator-image?

name: presto-coordinator-runtime-image
needs: maven-checks
runs-on: ubuntu-22.04
if: ${{ always() && success() }}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Author

Choose a reason for hiding this comment

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

if the previous step failed then this step should not be executed, because this step needs the artifacts built in the last step

Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure if you remove this line, the jobs will work in the same way. I.e., if B "needs" A, then B won't run if A fails.

  • success() is the default:

    A default status check of success() is applied unless you include one of these functions.

  • always() is the opposite of what you want, where it will ignore whether the first job fails.

steps:
- uses: "actions/checkout@v4"
with:
submodules: "recursive"

- name: Download presto-server
uses: actions/download-artifact@v4
with:
name: presto-server
path: ./docker

- name: Download presto-cli
uses: actions/download-artifact@v4
with:
name: presto-cli
path: ./docker

- name: "Login to image registry"
uses: "docker/login-action@v3"
with:
registry: ghcr.io
username: ${{github.actor}}
password: ${{secrets.GITHUB_TOKEN}}

- name: "Set up container image metadata"
id: "meta"
uses: "docker/metadata-action@v5"
with:
images: >-
ghcr.io/${{github.repository}}/coordinator
tags: |-
type=raw,value=dev

- name: "Build and push"
uses: "docker/build-push-action@v6"
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Possible GHCR push failure with mixed-case repo name

${{ github.repository }} keeps capital letters; GHCR requires lower-case paths. Convert to lower-case (e.g., ${{ github.repository }} | toLower).

🤖 Prompt for AI Agents
In .github/workflows/maven-checks.yml around lines 100 to 106, the usage of `${{
github.repository }}` in the image path may cause GHCR push failures because it
retains uppercase letters, while GHCR requires lowercase paths. Fix this by
converting `${{ github.repository }}` to lowercase using the `toLower` function,
ensuring the image path is fully lowercase.

with:
build-args: |-
JMX_PROMETHEUS_JAVA_AGENT_VERSION=0.20.0
PRESTO_VERSION=0.293
context: "./docker/"
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Build args repeat the hard-coded version

PRESTO_VERSION=0.293 duplicates the literal again. Once you introduce a workflow-level PRESTO_VERSION, reference it here instead.

-            PRESTO_VERSION=0.293
+            PRESTO_VERSION=${{ env.PRESTO_VERSION }}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
build-args: |-
JMX_PROMETHEUS_JAVA_AGENT_VERSION=0.20.0
PRESTO_VERSION=0.293
context: "./docker/"
build-args: |-
JMX_PROMETHEUS_JAVA_AGENT_VERSION=0.20.0
PRESTO_VERSION=${{ env.PRESTO_VERSION }}
context: "./docker/"
🤖 Prompt for AI Agents
In .github/workflows/maven-checks.yml around lines 108 to 111, the build
argument PRESTO_VERSION is hard-coded as 0.293, duplicating the version literal.
Replace this hard-coded value with a reference to the workflow-level
PRESTO_VERSION variable to avoid repetition and improve maintainability.

file: "./docker/Dockerfile"
push: >-
${{github.event_name != 'pull_request'
&& github.ref == 'refs/heads/release-0.293-clp-connector'}}
tags: "${{steps.meta.outputs.tags}}"
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider skipping the heavy Docker build on pull-requests
You already guard the push, but the image is still fully built during PRs, adding ~5-10 min to CI. Add the same condition to the build step or wrap the whole job in if: github.event_name == 'push' && github.ref == 'refs/heads/release-0.293-clp-connector'.

🤖 Prompt for AI Agents
In .github/workflows/maven-checks.yml around lines 104 to 115, the Docker image
build runs during pull requests, causing unnecessary CI delays. To fix this, add
a condition to skip the build step during pull requests by adding an if clause
that checks if the event is a push to the release-0.293-clp-connector branch, or
wrap the entire job with this condition. This ensures the heavy Docker build
only runs on relevant pushes, not on PRs.

labels: "${{steps.meta.outputs.labels}}"
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
name: prestocpp-worker-with-clp-connector-runtime-image-build
Copy link
Member

@kirkrodrigues kirkrodrigues Jul 18, 2025

Choose a reason for hiding this comment

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

How about prestissimo-worker-images-build?


on:
# TODO: specifiy the branch to the release-0.293 when finalize the PR
pull_request:
push:

Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Stale TODO & typo; tighten branch filters instead of post-build conditions

The comment still says “specifiy” and the TODO is no longer needed—the branch name is already hard-coded further down.
While you are here, move the branch restriction into the on.push.branches filter so the workflow does not run (and waste minutes) on every push.

-# TODO: specifiy the branch to the release-0.293 when finalize the PR
+# Workflow only runs for the release branch
+
+push:
+  branches:
+    - release-0.293-clp-connector

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In .github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml
around lines 4 to 7, remove the outdated TODO comment with the typo "specifiy"
since the branch is already specified later. Then, add the branch restriction
directly under the on.push.branches filter to prevent the workflow from running
on every push, optimizing build time by limiting runs to the intended branch
only.

Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

TODO & broad trigger – restrict the workflow before merge

The comment is still TODO and the push trigger has no branch filter, so every branch push will execute an expensive multi-image build.
Either:

on:
  push:
    branches: [ "release-0.293-clp-connector" ]
  pull_request:
    branches: [ "release-0.293-clp-connector" ]

or drop the TODO entirely.

🤖 Prompt for AI Agents
In .github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml
around lines 3 to 7, the workflow trigger is too broad because the push event
has no branch filter, causing expensive builds on every branch push. To fix
this, restrict the push and pull_request triggers to only the
"release-0.293-clp-connector" branch by adding a branches filter under each
trigger or remove the TODO comment if no restriction is needed.

jobs:
prestocpp-worker-with-clp-connector-runtime-image:
Copy link
Member

Choose a reason for hiding this comment

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

How about prestissimo-worker-images-build?

name: prestocpp-worker-with-clp-connector-runtime-image
runs-on: ubuntu-22.04
steps:
- uses: "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683"
with:
submodules: "recursive"

- name: "Login to image registry"
uses: "docker/login-action@74a5d142397b4f367a81961eba4e8cd7edddf772"
with:
registry: ghcr.io
username: ${{github.actor}}
password: ${{secrets.GITHUB_TOKEN}}

- name: "Set up metadata for dependency image"
id: "meta-dependency"
Copy link
Member

Choose a reason for hiding this comment

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

How about metadata-deps-image?

uses: "docker/metadata-action@902fa8ec7d6ecbf8d84d538b9b233a880e428804"
with:
images: >-
ghcr.io/${{github.repository}}/prestissimo-worker-dev-env
tags: |-
type=raw,value=dev
- name: "Set up metadata for runtime image"
id: "meta-runtime"
Copy link
Member

Choose a reason for hiding this comment

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

How about metadata-runtime-image?

uses: "docker/metadata-action@902fa8ec7d6ecbf8d84d538b9b233a880e428804"
with:
images: >-
ghcr.io/${{github.repository}}/prestissimo-worker
tags: |-
type=raw,value=dev
- name: Check for dependency changes or missing dependency image
id: dependency-changes
run: |
git fetch origin ${{ github.event.before }} --depth=1 || true
dep_changed=false
if git diff --name-only ${{ github.event.before }} ${{ github.sha }} | grep -q '^presto-native-execution/scripts'; then
dep_changed=true
fi
velox_old_sha=$(git ls-tree ${{ github.event.before }} presto-native-execution/velox | awk '{print $3}')
velox_new_sha=$(git ls-tree ${{ github.sha }} presto-native-execution/velox | awk '{print $3}')
cd presto-native-execution/velox
if git diff --name-only ${velox_old_sha} ${velox_new_sha} | grep -q '^scripts'; then
dep_changed=true
fi
echo "dep-changed=${dep_changed}" >> $GITHUB_OUTPUT
image_exists=true
if ! docker manifest inspect "${{ steps.meta-dependency.outputs.tags }}" > /dev/null 2>&1; then
image_exists=false
fi
echo "image-exists=${image_exists}" >> $GITHUB_OUTPUT
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a better way to do this using dorny/paths-filter that we use in clp. So for now, can we delete this and defer the effort to another PR?

- name: "Build and push dependency image"
if: steps.dependency-changes.outputs.dep-changed == 'true' || steps.dependency-changes.outputs.image-exists == 'false'
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Hyphens in output names break workflow parsing

Outputs must match ^[A-Za-z0-9_]+$. dep-changed and image-exists are invalid, causing a syntax error at workflow load time.

-echo "dep-changed=${dep_changed}" >> $GITHUB_OUTPUT
+echo "dep_changed=${dep_changed}" >> $GITHUB_OUTPUT
...
-echo "image-exists=${image_exists}" >> $GITHUB_OUTPUT
+echo "image_exists=${image_exists}" >> $GITHUB_OUTPUT
...
-if: steps.dependency-changes.outputs.dep-changed == 'true' || steps.dependency-changes.outputs.image-exists == 'false'
+if: steps.dependency-changes.outputs.dep_changed == 'true' || steps.dependency-changes.outputs.image_exists == 'false'
🤖 Prompt for AI Agents
In .github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml
around lines 57 to 66, the output names 'dep-changed' and 'image-exists' use
hyphens, which are invalid characters for GitHub Actions output names. Rename
these outputs to use only alphanumeric characters and underscores, such as
'dep_changed' and 'image_exists', and update all references to these outputs
accordingly to fix the syntax error.

uses: "docker/build-push-action@471d1dc4e07e5cdedd4c2171150001c434f0b7a4"
with:
context: "./presto-native-execution/"
file: "./presto-native-execution/scripts/dockerfiles/ubuntu-22.04-dependency.dockerfile"
push: >-
${{github.event_name != 'pull_request'
&& github.ref == 'refs/heads/release-0.293-clp-connector'}}
tags: "${{steps.meta-dependency.outputs.tags}}"
labels: "${{steps.meta-dependency.outputs.labels}}"

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

DEPENDENCY_IMAGE may contain multiple tags → invalid ARG

steps.meta-dependency.outputs.tags is newline-separated. Extract the first tag before passing it.

build-args: |
  DEPENDENCY_IMAGE=${{ steps.meta-dependency.outputs.tags %% $'\n'* }}
🤖 Prompt for AI Agents
In .github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml
around lines 65 to 76, the tags input uses steps.meta-dependency.outputs.tags
which contains multiple newline-separated tags, causing an invalid ARG error. To
fix this, extract only the first tag from the tags output before passing it as
DEPENDENCY_IMAGE in build-args by using shell parameter expansion or equivalent
syntax to split on the newline and select the first tag.

- name: Get number of cores
id: get-cores
run: |-
echo "num-threads=$(nproc)" >> $GITHUB_OUTPUT
Copy link
Member

Choose a reason for hiding this comment

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

How about num_cores?

- name: "Build and push runtime image"
uses: "docker/build-push-action@471d1dc4e07e5cdedd4c2171150001c434f0b7a4"
with:
build-args: |-
NUM_THREADS=${{steps.get-cores.outputs.num-threads}}
DEPENDENCY_IMAGE=${{steps.meta-dependency.outputs.tags}}
BASE_IMAGE=ubuntu:22.04
OSNAME=ubuntu
EXTRA_CMAKE_FLAGS=-DPRESTO_ENABLE_TESTING=OFF -DPRESTO_ENABLE_PARQUET=ON -DPRESTO_ENABLE_S3=ON
context: "./presto-native-execution/"
file: "./presto-native-execution/scripts/dockerfiles/prestissimo-runtime.dockerfile"
push: >-
${{github.event_name != 'pull_request'
&& github.ref == 'refs/heads/release-0.293-clp-connector'}}
tags: "${{steps.meta-runtime.outputs.tags}}"
labels: "${{steps.meta-runtime.outputs.labels}}"
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

ARG DEPENDENCY_IMAGE=presto/prestissimo-dependency:centos9
ARG BASE_IMAGE=quay.io/centos/centos:stream9
FROM ${DEPENDENCY_IMAGE} as prestissimo-image

Check warning on line 15 in presto-native-execution/scripts/dockerfiles/prestissimo-runtime.dockerfile

View workflow job for this annotation

GitHub Actions / prestocpp-worker-with-clp-connector-runtime-image

The 'as' keyword should match the case of the 'from' keyword

FromAsCasing: 'as' and 'FROM' keywords' casing do not match More info: https://docs.docker.com/go/dockerfile/rule/from-as-casing/

ARG OSNAME=centos
ARG BUILD_TYPE=Release
Expand All @@ -39,6 +39,15 @@
ENV BUILD_BASE_DIR=_build
ENV BUILD_DIR=""

# Temporary fix for https://github.com/prestodb/presto/issues/25531
# TODO: Update this code when there's a proper fix
RUN apt-get update && \
DEBIAN_FRONTEND=noninteractive apt-get install -y tzdata && \
ln -snf /usr/share/zoneinfo/America/Toronto /etc/localtime && \
echo "America/New_York" > /etc/timezone && \
Copy link
Member

Choose a reason for hiding this comment

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

What error do we get if we don't have these lines?

Copy link
Author

Choose a reason for hiding this comment

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

prestodb#25531
In the description

apt-get clean && \
rm -rf /var/lib/apt/lists/*

COPY --chmod=0775 --from=prestissimo-image /prestissimo/${BUILD_BASE_DIR}/${BUILD_DIR}/presto_cpp/main/presto_server /usr/bin/
COPY --chmod=0775 --from=prestissimo-image /runtime-libraries/* /usr/lib64/prestissimo-libs/
COPY --chmod=0755 ./etc /opt/presto-server/etc
Expand Down
Loading