CI: Build presto dependency container image by default instead of fetching it from s3#207
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the CI workflow to build Presto dependency container images locally by default rather than downloading them from S3. This change aims to reduce dependency on external S3 storage and provides more flexibility in CI execution.
Changes:
- Added a new
build_dependency_imageinput parameter (defaulting totrue) to control whether dependency images are built locally or fetched from S3 - Updated workflow jobs to pass the new parameter through the workflow chain
- Added conditional logic to build or download the dependency image based on the parameter value
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| .github/workflows/presto-test.yml | Adds build_dependency_image input parameter and passes it to all dependent jobs (java, native-cpu, native-gpu) |
| .github/workflows/presto-test-composite.yml | Implements the build/download logic with conditional steps based on the new parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| description: 'Set VELOX_ENABLE_BACKWARD_COMPATIBLE in Velox build' | ||
| type: boolean | ||
| default: false | ||
| build_dependency_image: &build_dependency_image |
There was a problem hiding this comment.
Using YAML anchors for workflow inputs is unconventional and may not work as expected across workflow_dispatch and workflow_call trigger types. GitHub Actions typically requires explicit input definitions for each trigger type. Consider defining the input separately for each trigger or verifying this works correctly in your GitHub Actions environment.
There was a problem hiding this comment.
@Avinash-Raj can we assume this concern is unfounded, as we are already using the same semantics for the other parameters?
There was a problem hiding this comment.
Yes, we don't need to worry about it. Since we already have our workflows working with those anchors.
simoneves
left a comment
There was a problem hiding this comment.
LGTM assuming the Copilot concern is unfounded
| description: 'Set VELOX_ENABLE_BACKWARD_COMPATIBLE in Velox build' | ||
| type: boolean | ||
| default: false | ||
| build_dependency_image: &build_dependency_image |
There was a problem hiding this comment.
@Avinash-Raj can we assume this concern is unfounded, as we are already using the same semantics for the other parameters?
| description: 'Set VELOX_ENABLE_BACKWARD_COMPATIBLE in Velox build' | ||
| type: boolean | ||
| default: false | ||
| build_dependency_image: &build_dependency_image |
No description provided.