Skip to content
Merged
Show file tree
Hide file tree
Changes from 21 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
64 changes: 64 additions & 0 deletions .github/workflows/maven-checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,69 @@ 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 tarball
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
- name: Upload presto-cli executable
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
- name: Clean Maven Output
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Hard-coding 0.293 couples the workflow to a single release

The tarball/jar paths and the later ./build.sh 0.293 call embed the version string three times.
When upgrading Presto the CI will silently break until every occurrence is updated.

Introduce a single environment variable at the top of the workflow:

env:
  PRESTO_VERSION: "0.293"

and replace the literals with ${{ env.PRESTO_VERSION }} (including the build-script argument).
This keeps bumps to one-line changes.

🤖 Prompt for AI Agents
In .github/workflows/maven-checks.yml around lines 49 to 61, the Presto version
"0.293" is hard-coded multiple times in artifact paths and build script calls,
causing maintenance issues. Define a single environment variable PRESTO_VERSION
at the top of the workflow with the version string, then replace all occurrences
of the literal "0.293" in paths and script arguments with the variable reference
${{ env.PRESTO_VERSION }} to centralize version management and simplify updates.

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@11bd71901bbe5b1630ceea73d27597364c9af683"
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@74a5d142397b4f367a81961eba4e8cd7edddf772"
with:
registry: ghcr.io
username: ${{github.actor}}
password: ${{secrets.GITHUB_TOKEN}}

- name: "Update Metadata"
id: "meta"
uses: "docker/metadata-action@902fa8ec7d6ecbf8d84d538b9b233a880e428804"
with:
images: >-
ghcr.io/${{github.repository}}/coordinator-with-clp-connector-runtime
tags: |
type=raw,value=centos9

- name: "Build and push"
uses: "docker/build-push-action@471d1dc4e07e5cdedd4c2171150001c434f0b7a4"
with:
build-args: |-
PRESTO_VERSION=0.293
JMX_PROMETHEUS_JAVA_AGENT_VERSION=0.20.0
context: "./docker/"
file: "./docker/Dockerfile"
push: >-
${{github.event_name != 'pull_request'
&& github.ref == 'refs/heads/release-0.293-clp-connector'}}
tags: "${{steps.meta.outputs.tags}}"
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: "Update Metadata for Dependency Image"
Copy link
Member

Choose a reason for hiding this comment

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

Can we reorder these steps so the order is:

  • Set up metadata for deps image
  • Build deps image
  • Set up metadata for runtime image
  • Build runtime 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-with-clp-connector-dependency
tags: |
type=raw,value=ubuntu-22.04
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Image repository is not lower-cased – GHCR rejects mixed-case paths
${{ github.repository }} preserves capital letters in the org/user name. GHCR (and Docker) require all-lowercase repositories, so pushes fail for maintainers with uppercase chars.

-            ghcr.io/${{github.repository}}/prestissimo-with-clp-connector-dependency
+            ghcr.io/${{ github.repository  | toLower }} /prestissimo-with-clp-connector-dependency

(The same change is needed for the runtime image block below.)
GitHub Actions lacks a built-in toLower function; one common workaround is:

env:
  REPO_LOWER: ${{ github.repository }} 
...
          images: ghcr.io/${{ env.REPO_LOWER,, }}/prestissimo-with-clp-connector-dependency

Please adopt a lower-casing strategy before merging.

Also applies to: 48-51

🤖 Prompt for AI Agents
In .github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml
around lines 28 to 31 and also lines 48 to 51, the image repository path uses
`${{ github.repository }}` which can contain uppercase letters, causing GHCR
push failures. Fix this by defining an environment variable that stores the
lowercase version of the repository name using shell parameter expansion (e.g.,
`REPO_LOWER: ${{ github.repository }}`) and then reference this variable with
lowercase conversion syntax `${{ env.REPO_LOWER,, }}` in the image paths to
ensure all repository names are lowercase.

- name: "Update 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-with-clp-connector-runtime
tags: |
type=raw,value=ubuntu-22.04
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Repository path may be rejected by GHCR – force lowercase

${{ github.repository }} preserves upper-case characters, but GHCR requires all-lowercase names. Past review flagged this; the issue is still present.

-images: ghcr.io/${{github.repository}}/prestissimo-with-clp-connector-dependency
+images: ghcr.io/${{ env.REPO_LOWER }}/prestissimo-with-clp-connector-dependency

Add a job-level env:

env:
  REPO_LOWER: ${{ github.repository }}

and reference with Bash lowercase ${{ env.REPO_LOWER,, }}.
Do the same for the runtime image block.

🤖 Prompt for AI Agents
In .github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml
around lines 24 to 41, the use of `${{ github.repository }}` in image names may
include uppercase letters, which GHCR rejects. To fix this, add a job-level
environment variable `REPO_LOWER` set to `${{ github.repository }}`, then update
the image references to use the lowercase form `${{ env.REPO_LOWER,, }}` in both
the dependency and runtime image metadata update steps.

- 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 CPU
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
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Unused/unsafe build args – consider quoting and sanitising

EXTRA_CMAKE_FLAGS contains spaces; quoting the entire arg prevents word-splitting issues:

EXTRA_CMAKE_FLAGS="-DPRESTO_ENABLE_TESTING=OFF -DPRESTO_ENABLE_PARQUET=ON -DPRESTO_ENABLE_S3=ON"

Also ensure downstream Dockerfile uses "$EXTRA_CMAKE_FLAGS" with quotes.

🤖 Prompt for AI Agents
In .github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml
around lines 76 to 81, the build argument EXTRA_CMAKE_FLAGS contains spaces and
is not quoted, which can cause word-splitting issues. Fix this by enclosing the
entire value of EXTRA_CMAKE_FLAGS in double quotes. Additionally, verify that
the Dockerfile uses "$EXTRA_CMAKE_FLAGS" with quotes when referencing this
argument to ensure it is passed correctly without splitting.

Copy link
Member

Choose a reason for hiding this comment

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

Can we alphabetize these so they're easier to read later?

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=""

# TODO: This is a temporary fix for issue: https://github.com/prestodb/presto/issues/25531
# When there is a proper fix, update this piece of code.
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 link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Timezone hot-fix assumes an Ubuntu base – breaks the default CentOS build

apt-get exists only when the build actually overrides BASE_IMAGE with an Ubuntu image (as the CI workflow does).
If anyone builds this Dockerfile with the default BASE_IMAGE=quay.io/centos/centos:stream9, the layer fails immediately.

-RUN apt-get update && \
-    DEBIAN_FRONTEND=noninteractive apt-get install -y tzdata && \
+RUN if command -v apt-get >/dev/null; then \
+      apt-get update && \
+      DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends tzdata; \
+    elif command -v dnf >/dev/null; then \
+      dnf install -y tzdata; \
+    else \
+      echo "Unsupported package manager" && exit 1; \
+    fi && \

• Makes the fix distro-agnostic.
• Adds --no-install-recommends per Hadolint DL3015.
• Still keeps the clean-up commands below.

Alternatively, split into two dedicated Dockerfiles (centos vs ubuntu) to avoid conditional logic.
Please patch before the next CentOS-based release build.

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

🧰 Tools
🪛 Hadolint (2.12.0)

[info] 44-44: Avoid additional packages by specifying --no-install-recommends

(DL3015)


[warning] 44-44: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>

(DL3008)

🤖 Prompt for AI Agents
In presto-native-execution/scripts/dockerfiles/prestissimo-runtime.dockerfile
around lines 42 to 49, the current timezone fix uses apt-get which only works on
Ubuntu-based images and fails on the default CentOS base image. To fix this,
make the timezone setup commands conditional based on the base image or split
the Dockerfile into separate versions for Ubuntu and CentOS. Also, add
--no-install-recommends to the apt-get install command to follow best practices
and keep the cleanup commands intact. This will ensure the build works correctly
regardless of the base image used.


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