-
Notifications
You must be signed in to change notification settings - Fork 4
ci: Add GitHub workflows to build and publish coordinator runtime, worker runtime, and worker development environment container images. #26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 17 commits
4f23c7b
48dff13
9bd9207
4997a02
d74b885
4815351
e7fe20c
fc2cc18
cc2c510
60dd19d
a534bec
9e17bc0
aeb4e48
dbb28fa
1e69681
4ca014c
fec665a
20a7054
283b751
64983f0
a6ebbaa
5ca0420
fa06d23
d120672
4c90aad
3b76c12
b55aeda
74787c7
313a648
47b9c10
ceaa809
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| - name: Upload presto-cli executable | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| 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 | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| - name: Clean Maven Output | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Hard-coding The tarball/jar paths and the later Introduce a single environment variable at the top of the workflow: env:
PRESTO_VERSION: "0.293"and replace the literals with 🤖 Prompt for AI Agents |
||
| run: ./mvnw clean -pl '!:presto-server,!:presto-cli,!presto-test-coverage' | ||
|
|
||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
anlowee marked this conversation as resolved.
Outdated
|
||
| presto-coordinator-with-clp-connector-runtime-image: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() }} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| steps: | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| - uses: "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| 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" | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| with: | ||
| registry: ghcr.io | ||
| username: ${{github.actor}} | ||
| password: ${{secrets.GITHUB_TOKEN}} | ||
|
|
||
| - name: "Update Metadata" | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| id: "meta" | ||
| uses: "docker/metadata-action@902fa8ec7d6ecbf8d84d538b9b233a880e428804" | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| with: | ||
| images: >- | ||
| ghcr.io/${{github.repository}}/coordinator-with-clp-connector-runtime | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| tags: | | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| type=raw,value=centos9 | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
|
|
||
| - name: "Build and push" | ||
| uses: "docker/build-push-action@471d1dc4e07e5cdedd4c2171150001c434f0b7a4" | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| with: | ||
| build-args: |- | ||
| PRESTO_VERSION=0.293 | ||
| JMX_PROMETHEUS_JAVA_AGENT_VERSION=0.20.0 | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| 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}}" | ||
|
Comment on lines
+104
to
+114
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Consider skipping the heavy Docker build on pull-requests 🤖 Prompt for AI Agents |
||
| labels: "${{steps.meta.outputs.labels}}" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| name: prestocpp-worker-with-clp-connector-runtime-image-build | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. -# 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
🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 |
||
| jobs: | ||
| prestocpp-worker-with-clp-connector-runtime-image: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| uses: "docker/login-action@74a5d142397b4f367a81961eba4e8cd7edddf772" | ||
| with: | ||
| registry: ghcr.io | ||
| username: ${{github.actor}} | ||
| password: ${{secrets.GITHUB_TOKEN}} | ||
|
|
||
| - name: "Update Metadata for Dependency Image" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we reorder these steps so the order is:
anlowee marked this conversation as resolved.
Outdated
|
||
| id: "meta-dependency" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| tags: | | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| type=raw,value=ubuntu-22.04 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Image repository is not lower-cased – GHCR rejects mixed-case paths - 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.) 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
anlowee marked this conversation as resolved.
Outdated
|
||
|
|
||
| - name: "Update Metadata for Runtime Image" | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| id: "meta-runtime" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| tags: | | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| type=raw,value=ubuntu-22.04 | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Repository path may be rejected by GHCR – force lowercase
-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 🤖 Prompt for AI Agents |
||
| - 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hyphens are not allowed in output names ( -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”.
🤖 Prompt for AI Agents |
||
| - 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Multi-line tag list passed as
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 |
||
| - name: Get number of CPU | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| id: get-cores | ||
| run: | | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| echo "num-threads=$(nproc)" >> $GITHUB_OUTPUT | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about |
||
|
|
||
| - 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 }} | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| 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. Choose a reason for hiding this commentThe 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="-DPRESTO_ENABLE_TESTING=OFF -DPRESTO_ENABLE_PARQUET=ON -DPRESTO_ENABLE_S3=ON"Also ensure downstream 🤖 Prompt for AI Agents
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
|---|---|---|
|
|
@@ -39,6 +39,15 @@ FROM ${BASE_IMAGE} | |
| 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. | ||
|
anlowee marked this conversation as resolved.
Outdated
|
||
| 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 && \ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What error do we get if we don't have these lines?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prestodb#25531 |
||
| apt-get clean && \ | ||
| rm -rf /var/lib/apt/lists/* | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Timezone hot-fix assumes an Ubuntu base – breaks the default CentOS build
-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. Alternatively, split into two dedicated Dockerfiles (centos vs ubuntu) to avoid conditional logic.
🧰 Tools🪛 Hadolint (2.12.0)[info] 44-44: Avoid additional packages by specifying (DL3015) [warning] 44-44: Pin versions in apt get install. Instead of (DL3008) 🤖 Prompt for AI Agents |
||
|
|
||
| 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 | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.