ci: Add GitHub workflows to build and publish coordinator runtime, worker runtime, and worker development environment container images. #26
Conversation
|
""" WalkthroughThis update introduces a new GitHub Actions workflow for building and pushing prestissimo worker Docker images, modifies the existing Maven workflow to upload artifacts and add a coordinator image build job, and applies a timezone configuration workaround in a Dockerfile for the prestissimo runtime image. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant Docker Registry
Developer->>GitHub Actions: Push or PR triggers workflow
GitHub Actions->>GitHub Actions: Checkout repository (with submodules)
GitHub Actions->>GitHub Actions: Build prestissimo-worker-dev-env Docker image
GitHub Actions->>Docker Registry: (If release branch & not PR) Push dev-env image
GitHub Actions->>GitHub Actions: Build prestissimo-worker runtime Docker image
GitHub Actions->>Docker Registry: (If release branch & not PR) Push runtime image
sequenceDiagram
participant GitHub Actions
participant Artifact Storage
participant Docker Registry
GitHub Actions->>GitHub Actions: Run maven-checks (Java 8.0.442)
GitHub Actions->>Artifact Storage: Upload presto-server and presto-cli artifacts
GitHub Actions->>GitHub Actions: Run presto-coordinator-image job
GitHub Actions->>Artifact Storage: Download artifacts to ./docker
GitHub Actions->>Docker Registry: (If release branch & not PR) Push coordinator image
Estimated code review effort3 (~45 minutes) Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
anlowee
force-pushed
the
xwei/add-release-image-publish-workflow
branch
14 times, most recently
from
6e2a5b7 to
97badb5
Compare
anlowee
force-pushed
the
xwei/add-release-image-publish-workflow
branch
from
8d50a75 to
9bd9207
Compare
…e-publish-workflow
anlowee
force-pushed
the
xwei/add-release-image-publish-workflow
branch
from
ccb6bed to
79a72a3
Compare
anlowee
force-pushed
the
xwei/add-release-image-publish-workflow
branch
from
79a72a3 to
d74b885
Compare
…the image when pushing to our main branch
anlowee
force-pushed
the
xwei/add-release-image-publish-workflow
branch
2 times, most recently
from
e34e3a9 to
bf86445
Compare
anlowee
force-pushed
the
xwei/add-release-image-publish-workflow
branch
from
bf86445 to
fc2cc18
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
.github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml (3)
4-6: Remove stale TODO & fix the “specifiy” typo
The branch has already been hard-coded further down (lines 39-40 & 65-66). Keeping this TODO is misleading and the typo hurts professionalism.- # TODO: specifiy the branch to the release-0.293 when finalize the PR + # Workflow triggers on PRs for any branch and on pushes (see below).
33-42: Enable Docker layer caching to speed up CI
The dependency image rebuilds from scratch every run, adding several minutes to the workflow.docker/build-push-actionsupports registry caching out-of-the-box.uses: "docker/build-push-action@471d1dc4e07e5cdedd4c2171150001c434f0b7a4" with: + cache-from: type=registry,ref=ghcr.io/${{ github.repository }}/prestissimo-with-clp-connector-dependency:cache + cache-to: type=registry,ref=ghcr.io/${{ github.repository }}/prestissimo-with-clp-connector-dependency:cache,mode=max context: "./presto-native-execution/"
33-34: Duplicate step names make job logs hard to scan
Both build steps are called “Build and push”. Give the runtime build a distinctive name.- - name: "Build and push" + - name: "Build and push (dependency)" ... - - name: "Build and push" + - name: "Build and push (runtime)"Also applies to: 53-54
.github/workflows/maven-checks.yml (3)
68-68:always() && success()is self-contradictory
always()makes the condition true even on failed upstream jobs;success()then forces false, but the expression is confusing. A plainsuccess()(or no condition at all) is clearer.- if: ${{ always() && success() }} + # Downstream job runs only on successful maven-checks + if: ${{ success() }}
49-60: Hard-coded version0.293appears in 4 places
Upgrading Presto will require editing each occurrence. Define a singlePRESTO_VERSIONenv var and reuse it.-env: +env: + PRESTO_VERSION: "0.293" ... - path: presto-server/target/presto-server-0.293.tar.gz + path: presto-server/target/presto-server-${{ env.PRESTO_VERSION }}.tar.gz ... - path: presto-cli/target/presto-cli-0.293-executable.jar + path: presto-cli/target/presto-cli-${{ env.PRESTO_VERSION }}-executable.jar ... - PRESTO_VERSION=0.293 + PRESTO_VERSION=${{ env.PRESTO_VERSION }}Also applies to: 106-108
61-63: Strip trailing whitespace to satisfy YAML linters
Line 63 contains only spaces and failsyamllint.- +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/maven-checks.yml(1 hunks).github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml (1)
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/maven-checks.yml (1)
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
🪛 YAMLlint (1.37.1)
.github/workflows/maven-checks.yml
[error] 63-63: trailing spaces
(trailing-spaces)
| images: >- | ||
| ghcr.io/${{github.repository}}/prestissimo-with-clp-connector-dependency | ||
| tags: | | ||
| type=raw,value=ubuntu-22.04 |
There was a problem hiding this comment.
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-dependencyPlease 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.
| build-args: |- | ||
| NUM_THREADS=4 | ||
| DEPENDENCY_IMAGE=${{ steps.meta-dependency.outputs.tags }} | ||
| BASE_IMAGE=ubuntu:22.04 |
There was a problem hiding this comment.
DEPENDENCY_IMAGE may contain multiple tags → invalid ARG
steps.meta-dependency.outputs.tags is a newline-separated list (often multiple tags). Passing it verbatim to docker build --build-arg yields unknown flag: –t or pulls the wrong image.
- DEPENDENCY_IMAGE=${{ steps.meta-dependency.outputs.tags }}
+ # Use the first tag only
+ DEPENDENCY_IMAGE=${{ steps.meta-dependency.outputs.tags%%$'\n'* }}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 56 to 59, the DEPENDENCY_IMAGE build argument is set directly from
steps.meta-dependency.outputs.tags, which can contain multiple newline-separated
tags causing invalid ARG errors. To fix this, modify the workflow to extract and
pass only a single valid tag (e.g., the first tag) from the output instead of
the entire list, ensuring the build-arg receives a proper single image tag
string.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml(1 hunks)presto-native-execution/scripts/dockerfiles/prestissimo-runtime.dockerfile(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml (1)
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
🪛 Hadolint (2.12.0)
presto-native-execution/scripts/dockerfiles/prestissimo-runtime.dockerfile
[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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (17.0.13, :presto-main-base)
- GitHub Check: test (17.0.13, :presto-main)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-distributed-queries)
- GitHub Check: test (8.0.442, :presto-main-base)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-queries)
- GitHub Check: prestocpp-linux-build-for-test
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: prestocpp-worker-with-clp-connector-runtime-image
| # 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 && \ | ||
| apt-get clean && \ | ||
| rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
🧹 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.
| - name: "Build and push dependency image" | ||
| if: steps.dependency-changes.outputs.dep-changed == 'true' | ||
| 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}}" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Multi-line tag list passed as DEPENDENCY_IMAGE will break docker build
steps.meta-dependency.outputs.tags is newline-separated. Passing the whole string to --build-arg yields an invalid ARG or pulls the wrong image. Extract the first tag:
build-args: |
DEPENDENCY_IMAGE=${{ steps.meta-dependency.outputs.tags %% $'\n'* }}or capture it in a preceding step:
- run: echo "dep_tag=$(echo "${{ steps.meta-dependency.outputs.tags }}" | head -n1)" >> $GITHUB_OUTPUT
id: select-tag
...
DEPENDENCY_IMAGE=${{ steps.select-tag.outputs.dep_tag }}🤖 Prompt for AI Agents
In .github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml
around lines 56 to 67, the tags output from steps.meta-dependency.outputs.tags
is newline-separated and passing it directly to the tags or build-args causes
docker build to fail or use incorrect images. To fix this, extract only the
first tag from the multi-line string before passing it as DEPENDENCY_IMAGE in
build-args. You can do this by either using shell string manipulation to get the
first line or by adding a preceding step that captures the first tag into a
separate output variable and then referencing that single tag in the build-args.
| on: | ||
| # TODO: specifiy the branch to the release-0.293 when finalize the PR | ||
| pull_request: | ||
| push: | ||
|
|
There was a problem hiding this comment.
🧹 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.
| - name: "Update Metadata for Dependency Image" | ||
| id: "meta-dependency" | ||
| uses: "docker/metadata-action@902fa8ec7d6ecbf8d84d538b9b233a880e428804" | ||
| with: | ||
| images: >- | ||
| ghcr.io/${{github.repository}}/prestissimo-with-clp-connector-dependency | ||
| tags: | | ||
| type=raw,value=ubuntu-22.04 | ||
| - name: "Update Metadata for Runtime Image" | ||
| id: "meta-runtime" | ||
| uses: "docker/metadata-action@902fa8ec7d6ecbf8d84d538b9b233a880e428804" | ||
| with: | ||
| images: >- | ||
| ghcr.io/${{github.repository}}/prestissimo-with-clp-connector-runtime | ||
| tags: | | ||
| type=raw,value=ubuntu-22.04 | ||
There was a problem hiding this comment.
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-dependencyAdd 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 in presto-native-execution | ||
| 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 | ||
| cd presto-native-execution/velox | ||
| if git diff --name-only ${{ github.event.before }} ${{ github.sha }} | grep -q '^scripts'; then | ||
| dep_changed=true | ||
| fi | ||
| echo "dep-changed=${dep_changed}" >> $GITHUB_OUTPUT | ||
There was a problem hiding this comment.
dep-changed is an invalid output name – step expression will fail
Hyphens are not allowed in output names (^[A-Za-z0-9_]+$).
Change both the writer and the consumer:
-echo "dep-changed=${dep_changed}" >> $GITHUB_OUTPUT
+echo "dep_changed=${dep_changed}" >> $GITHUB_OUTPUTand
-if: steps.dependency-changes.outputs.dep-changed == 'true'
+if: steps.dependency-changes.outputs.dep_changed == 'true'Without this, the job errors with “Error: Invalid workflow file”.
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
between lines 42 and 55, the output name "dep-changed" uses a hyphen which is
invalid in GitHub Actions output names. Rename the output to use only
alphanumeric characters or underscores, such as "dep_changed", and update both
the line that sets the output (echo "dep-changed=...") and any references to
this output elsewhere in the workflow to match the new valid name.
…e-publish-workflow
…anlowee/presto into xwei/add-release-image-publish-workflow
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
.github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml (4)
90-90: QuoteEXTRA_CMAKE_FLAGSto avoid word-splittingThe value contains spaces; unquoted, Bash splits it into three args.
- EXTRA_CMAKE_FLAGS=-DPRESTO_ENABLE_TESTING=OFF -DPRESTO_ENABLE_PARQUET=ON -DPRESTO_ENABLE_S3=ON + EXTRA_CMAKE_FLAGS="-DPRESTO_ENABLE_TESTING=OFF -DPRESTO_ENABLE_PARQUET=ON -DPRESTO_ENABLE_S3=ON"
4-7: Restrict triggers & remove stale TODO – current setup will run expensive builds on every branch pushThe workflow still fires for any
pushorpull_request, and the TODO comment is obsolete. This will hammer CI minutes on feature branches.-on: - # TODO: specifiy the branch to the release-0.293 when finalize the PR - pull_request: - push: +on: + push: + branches: [ "release-0.293-clp-connector" ] + pull_request: + branches: [ "release-0.293-clp-connector" ]
28-40: Image repository not forced to lower-case → GHCR push will fail with “invalid reference format”
github.repositorypreserves upper-case characters. GHCR/Docker require lower-case paths, so pushes will fail for maintainers with capital letters in their username or org.+env: + REPO_LOWER: ${{ github.repository }} - images: >- - ghcr.io/${{github.repository}}/prestissimo-with-clp-connector-dependency + images: ghcr.io/${{ env.REPO_LOWER,, }}/prestissimo-with-clp-connector-dependency ... - images: >- - ghcr.io/${{github.repository}}/prestissimo-with-clp-connector-runtime + images: ghcr.io/${{ env.REPO_LOWER,, }}/prestissimo-with-clp-connector-runtime
86-88:DEPENDENCY_IMAGEcan be multi-line – pass only one tag
steps.meta-dependency.outputs.tagsis newline-separated when multiple tag formats are configured. Passing it verbatim todocker buildyieldsunknown flag: –t.- DEPENDENCY_IMAGE=${{ steps.meta-dependency.outputs.tags }} + # Use the first tag only + DEPENDENCY_IMAGE=${{ steps.meta-dependency.outputs.tags %% $'\n'* }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml (1)
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
| 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 | ||
| - name: "Build and push dependency image" | ||
| if: steps.dependency-changes.outputs.dep-changed == 'true' || steps.dependency-changes.outputs.image-exists == 'false' |
There was a problem hiding this comment.
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.
.github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml
Outdated
Show resolved
Hide resolved
| - name: Get number of CPU | ||
| id: get-cores | ||
| run: | | ||
| echo "num-threads=$(nproc)" >> $GITHUB_OUTPUT |
.github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml
Outdated
Show resolved
Hide resolved
| 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 |
There was a problem hiding this comment.
Can we alphabetize these so they're easier to read later?
.github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml
Outdated
Show resolved
Hide resolved
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
presto-native-execution/scripts/dockerfiles/prestissimo-runtime.dockerfile (1)
42-49: Time-zone hot-fix still breaks CentOS buildsThe unconditional
apt-getblock assumes an Ubuntu base image, butBASE_IMAGEcan be CentOS 9 (dnf) by default. The build will fail for anyone not overridingBASE_IMAGE.-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 && \ ln -snf /usr/share/zoneinfo/America/Toronto /etc/localtime && \ echo "America/New_York" > /etc/timezone && \ - apt-get clean && \ + (command -v apt-get >/dev/null && apt-get clean || true) && \ rm -rf /var/lib/apt/lists/*Adds distro-agnostic logic and fulfils Hadolint DL3015.
.github/workflows/maven-checks.yml (2)
49-64: Hard-coded0.293scattered—centralise into one env varThe version appears four times; every bump will require a search-and-replace. Define
env.PRESTO_VERSIONonce and interpolate.
71-71:always() && success()is self-contradictory
needs: maven-checksalready blocks on failure;always()nullifies that. Keep onlysuccess()or drop the condition..github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml (4)
3-7: Broad trigger & stale TODOWorkflow runs on every branch push. Restrict
push.branches/pull_request.branchestorelease-0.293-clp-connectorand delete the TODO.
25-41: Image paths must be lower-case for GHCR
ghcr.io/${{github.repository}}/...may contain uppercase chars → push rejected. Store a lower-cased var and use that in both metadata blocks.
57-63: Hyphens in output names break workflow parsing
dep-changed/image-existsviolate the^[A-Za-z0-9_]+$rule—workflow fails to load. Rename todep_changed,image_existsand update references.
85-91: QuoteEXTRA_CMAKE_FLAGSto preserve spacesWithout quotes, YAML splits the value and Docker sees only the first token.
EXTRA_CMAKE_FLAGS="-DPRESTO_ENABLE_TESTING=OFF -DPRESTO_ENABLE_PARQUET=ON -DPRESTO_ENABLE_S3=ON"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/maven-checks.yml(1 hunks).github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml(1 hunks)presto-native-execution/scripts/dockerfiles/prestissimo-runtime.dockerfile(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/prestocpp-worker-with-clp-connector-runtime-image-build.yml (1)
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/maven-checks.yml (1)
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
🪛 Hadolint (2.12.0)
presto-native-execution/scripts/dockerfiles/prestissimo-runtime.dockerfile
[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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-general)
- GitHub Check: test (17.0.13, :presto-main)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (8.0.442, :presto-main)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-resource-manager)
- GitHub Check: test (8.0.442, :presto-main-base)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: prestocpp-linux-build-for-test
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: prestocpp-worker-with-clp-connector-runtime-image
.github/workflows/maven-checks.yml
Outdated
| images: >- | ||
| ghcr.io/${{github.repository}}/coordinator | ||
| tags: |- | ||
| type=raw,value=dev | ||
| - name: "Build and push" | ||
| uses: "docker/build-push-action@v6" |
There was a problem hiding this comment.
🛠️ 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.
| - name: "Build and push dependency image" | ||
| if: steps.dependency-changes.outputs.dep-changed == 'true' || steps.dependency-changes.outputs.image-exists == 'false' | ||
| 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}}" | ||
|
|
There was a problem hiding this comment.
🛠️ 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.
2481b0b to
fa06d23
Compare
…e-publish-workflow
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
.github/workflows/maven-checks.yml (3)
49-64: Still hard-coding the Presto versionLiteral
0.293survives in both artifact paths and step names despite earlier feedback. Centralising the version in a singlePRESTO_VERSIONenv var keeps bumps to one-line changes.
71-72:always() && success()negates the protectionKeeping
always()makes the job run even when the upstream failed. Drop it or keep onlysuccess().
100-101: GHCR requires lower-case repository paths
${{ github.repository }}may contain capitals → push will fail. Wrap withtoLower()or usegithub.repository_owner.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/maven-checks.yml(1 hunks).github/workflows/prestissimo-worker-images-build.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/maven-checks.yml (1)
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/prestissimo-worker-images-build.yml (1)
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: test (17.0.13, :presto-main)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-resource-manager)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-resource-manager)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-general)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: prestocpp-linux-build-for-test
- GitHub Check: prestissimo-worker-images-build
🔇 Additional comments (1)
.github/workflows/prestissimo-worker-images-build.yml (1)
4-6: Open TODO left in trigger sectionThe branch filter still carries a TODO. Confirm the final branch before merging to avoid accidental runs on every push.
.github/workflows/maven-checks.yml
Outdated
| build-args: |- | ||
| JMX_PROMETHEUS_JAVA_AGENT_VERSION=0.20.0 | ||
| PRESTO_VERSION=0.293 | ||
| context: "./docker/" |
There was a problem hiding this comment.
🛠️ 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.
| 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.
| images: >- | ||
| ghcr.io/${{github.repository}}/prestissimo-worker | ||
| tags: |- | ||
| type=raw,value=dev |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same lowercase issue for the runtime image path
Apply the same fix as above to keep both images consistent and push-able.
🤖 Prompt for AI Agents
In .github/workflows/prestissimo-worker-images-build.yml around lines 48 to 51,
the runtime image path uses uppercase letters which can cause inconsistencies
and push failures. Modify the image path to use all lowercase letters, matching
the fix applied to the other image path, ensuring both image paths are
consistent and pushable.
| images: >- | ||
| ghcr.io/${{github.repository}}/prestissimo-worker-dev-env | ||
| tags: |- | ||
| type=raw,value=dev |
There was a problem hiding this comment.
Upper-case repository names break GHCR pushes
Convert ${{ github.repository }} to lower-case (or just use ${{ github.repository_owner }}) to avoid auth errors.
- ghcr.io/${{github.repository}}/prestissimo-worker-dev-env
+ ghcr.io/${{ github.repository_owner }}/prestissimo-worker-dev-env📝 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.
| images: >- | |
| ghcr.io/${{github.repository}}/prestissimo-worker-dev-env | |
| tags: |- | |
| type=raw,value=dev | |
| images: >- | |
| ghcr.io/${{ github.repository_owner }}/prestissimo-worker-dev-env | |
| tags: |- | |
| type=raw,value=dev |
🤖 Prompt for AI Agents
In .github/workflows/prestissimo-worker-images-build.yml around lines 28 to 31,
the use of `${{ github.repository }}` can cause GHCR push failures if the
repository name contains upper-case letters. To fix this, convert `${{
github.repository }}` to lower-case using a function or replace it with `${{
github.repository_owner }}` which is always lower-case, ensuring the image name
is valid and avoiding authentication errors.
…e-publish-workflow
…anlowee/presto into xwei/add-release-image-publish-workflow
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
.github/workflows/maven-checks.yml (3)
49-64: Hard-coded version string still scattered
The literal0.293is repeated in the upload paths. Please consolidate the version into an environment variable (PRESTO_VERSION) and interpolate it everywhere to avoid silent breakage on upgrades.
100-102: Image path may fail on GHCR because of mixed-case repository name
${{ github.repository }}preserves capital letters; GHCR requires lowercase. Wrap withtoLoweror use${{ github.repository_owner }}.
108-110: PRESTO_VERSION duplicated again in build args
Reference the same workflow-level variable instead of another literal0.293..github/workflows/prestissimo-worker-images-build.yml (2)
12-18: Inconsistent action pinning style vs other workflows
This file pins actions to SHAs whilemaven-checks.ymluses version tags. Pick one strategy (commit SHAs are safer) and apply it repo-wide for predictability.
28-30: Upper-case repository segment breaks GHCR push
Convert${{ github.repository }}to lowercase (or use${{ github.repository_owner }}) before composingghcr.io/...to avoidDENIED: invalid repository nameerrors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/maven-checks.yml(1 hunks).github/workflows/prestissimo-worker-images-build.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/maven-checks.yml (1)
Learnt from: anlowee
PR: #7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/prestissimo-worker-images-build.yml (1)
Learnt from: anlowee
PR: #7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-resource-manager)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (17.0.13, :presto-main)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-general)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (8.0.442, :presto-main-base)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-local-queries)
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: prestocpp-linux-build-for-test
- GitHub Check: prestissimo-worker-images-build
| - name: "Build and push" | ||
| uses: "docker/build-push-action@v6" | ||
| with: | ||
| build-args: |- | ||
| JMX_PROMETHEUS_JAVA_AGENT_VERSION=0.20.0 | ||
| PRESTO_VERSION=0.293 | ||
| 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}}" |
There was a problem hiding this comment.
🧹 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.
| uses: "docker/build-push-action@471d1dc4e07e5cdedd4c2171150001c434f0b7a4" | ||
| with: | ||
| build-args: |- | ||
| BASE_IMAGE=ubuntu:22.04 | ||
| DEPENDENCY_IMAGE=${{steps.metadata-deps-image.outputs.tags}} | ||
| EXTRA_CMAKE_FLAGS=-DPRESTO_ENABLE_TESTING=OFF -DPRESTO_ENABLE_PARQUET=ON -DPRESTO_ENABLE_S3=ON | ||
| NUM_THREADS=${{steps.get-cores.outputs.num_cores}} | ||
| OSNAME=ubuntu | ||
| context: "./presto-native-execution/" |
There was a problem hiding this comment.
DEPENDENCY_IMAGE may contain newlines, confusing BuildKit
docker/metadata-action outputs tags as a newline-separated list. Passing that raw into a single BUILD_ARG can inject stray line breaks and break the build. Grab the first tag explicitly:
- DEPENDENCY_IMAGE=${{steps.metadata-deps-image.outputs.tags}}
+ DEPENDENCY_IMAGE=${{ steps.metadata-deps-image.outputs.tags }}
+ # If multiple tags are present, keep only the first one
+ DEPENDENCY_IMAGE=${DEPENDENCY_IMAGE%%$'\n'*}or parse it in a separate step and set an output.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/prestissimo-worker-images-build.yml around lines 58 to 66,
the DEPENDENCY_IMAGE build argument is set directly from
steps.metadata-deps-image.outputs.tags, which may contain multiple
newline-separated tags causing BuildKit to fail. Fix this by extracting only the
first tag from the tags output before passing it as DEPENDENCY_IMAGE, either by
using shell commands to select the first line or by adding a separate step to
parse and set a single tag output, then reference that sanitized output in the
build-args.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
.github/workflows/maven-checks.yml (3)
49-64: Hard-coded Presto version still duplicated in paths – centralise via env varThe literal
0.293appears in both artifact paths. This was flagged in a prior review and remains unfixed. Introduce a singlePRESTO_VERSIONvariable at workflow level and reference it here to avoid silent breakage on future bumps.- path: "presto-server/target/presto-server-0.293.tar.gz" + path: "presto-server/target/presto-server-${{ env.PRESTO_VERSION }}.tar.gz" ... - path: "presto-cli/target/presto-cli-0.293-executable.jar" + path: "presto-cli/target/presto-cli-${{ env.PRESTO_VERSION }}-executable.jar"
96-103: GHCR path may fail due to uppercase letters
github.repositorypreserves case; GHCR rejects mixed-case names. Convert to lower-case (or usegithub.repository_owner) to guarantee pushes succeed.- images: "ghcr.io/${{github.repository}}/coordinator" + images: "ghcr.io/${{ github.repository }}".toLower()/coordinator
104-109: PRESTO_VERSION duplicated again in build argsKeep the build arg in sync with the artefact upload by reusing
env.PRESTO_VERSION.- PRESTO_VERSION=0.293 + PRESTO_VERSION=${{ env.PRESTO_VERSION }}.github/workflows/prestissimo-worker-images-build.yml (2)
24-29: Upper-case repository segment will break GHCR pushSame issue as coordinator image:
${{ github.repository }}may contain capitals. Lower-case it or switch to${{ github.repository_owner }}.- images: "ghcr.io/${{github.repository}}/prestissimo-worker-dev-env" + images: "ghcr.io/${{ github.repository_owner }}/prestissimo-worker-dev-env"
56-59:DEPENDENCY_IMAGEmay carry newline-separated tags → invalid build-arg
steps.metadata-deps-image.outputs.tagsoften contains multiple lines. BuildKit will choke on embedded newlines. Strip to the first tag before passing.- DEPENDENCY_IMAGE=${{steps.metadata-deps-image.outputs.tags}} + # Sanitise to first tag only + DEPENDENCY_IMAGE=${{ steps.metadata-deps-image.outputs.tags }} + DEPENDENCY_IMAGE=${DEPENDENCY_IMAGE%%$'\n'*}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/maven-checks.yml(1 hunks).github/workflows/prestissimo-worker-images-build.yml(1 hunks)
🧠 Learnings (3)
📓 Common learnings
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/prestissimo-worker-images-build.yml (1)
Learnt from: anlowee
PR: #7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/maven-checks.yml (1)
Learnt from: anlowee
PR: #7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/prestissimo-worker-images-build.yml (1)
Learnt from: anlowee
PR: #7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
.github/workflows/maven-checks.yml (1)
Learnt from: anlowee
PR: #7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (17.0.13, :presto-main-base)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-resource-manager)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (8.0.442, :presto-main)
- GitHub Check: test (8.0.442, :presto-main-base)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-general)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: prestocpp-linux-build-for-test
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: prestissimo-worker-images-build
| - uses: "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" | ||
| with: | ||
| submodules: "recursive" | ||
|
|
||
| - name: "Login to image registry" | ||
| uses: "docker/login-action@74a5d142397b4f367a81961eba4e8cd7edddf772" | ||
| with: |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mixed pinning style: commit SHAs here, version tags elsewhere
This workflow pins actions to SHAs while maven-checks.yml uses version tags (@v4, @v5). Pick one convention (preferably SHA pinning) across all workflows for consistency and supply-chain safety.
🤖 Prompt for AI Agents
In .github/workflows/prestissimo-worker-images-build.yml around lines 12 to 18,
the actions are pinned using commit SHAs while other workflows use version tags.
To maintain consistency and improve supply-chain security, update all action
references in this workflow to use commit SHAs instead of version tags, matching
the preferred SHA pinning style used elsewhere.
kirkrodrigues
merged commit 89ce0f3
into
y-scope:release-0.293-clp-connector
Description
For the coordinator image build, we modified the
build.shto usedocker buildinstead of a builder, otherwise the published image will have unknown manefest. We can probably look into this issue in the future but to make it work for now, we can make this change.Then in the exesiting
maven-checks.ymlwe add a new step which is to download the cached built coordinator and cli tar balls which are used for building the coordinator image. Then just run thebuild.shto build the image and publish it when directly push to the main branch.For the worker image, we added a new workflow, which does similar things as building coordinator image.
For the doc update, we have another PR to do that: #28
Checklist
breaking change.
Validation performed
Tested the coordinator image build on my fork and succeeded (need to modify the
REG_ORGtoghcr.io/anloweeinmaven-checks.yaml): https://github.com/anlowee/presto/actions/runs/16119159394/job/45480798215Then ran the following to test and it printed the coordinator information:
docker pull ghcr.io/anlowee/yscope-presto-with-clp-connector-coordinator docker run -p 8080:8080 ghcr.io/anlowee/yscope-presto-with-clp-connector-coordinator:latest # In another window curl -X GET http://localhost:8080/v1/infoFor worker. we also built the images successfully: https://github.com/y-scope/presto/actions/runs/16131561623/job/45519833846
Summary by CodeRabbit
New Features
Bug Fixes
Chores