Copilot commented on code in PR #478:
URL: https://github.com/apache/hudi-rs/pull/478#discussion_r2471624399
##########
.github/workflows/ci.yml:
##########
@@ -134,10 +157,35 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v5
+ - name: Set up Docker Buildx
+ uses: docker/setup-buildx-action@v3
+ - name: Pre-pull base images
+ run: |
+ docker pull quay.io/minio/minio:latest
+ - name: Prepare cache directories with correct permissions
+ run: |
+ mkdir -p .cargo/registry .cargo/git target python/target .uv-cache
+ sudo chown -R $(id -u):$(id -g) .cargo target python/target .uv-cache
Review Comment:
The `.cargo` and `.uv-cache` directories created here are not listed in
`.gitignore`. This could lead to unintentional commits of cache files to the
repository. Add `.cargo/` and `.uv-cache/` to the `.gitignore` file to prevent
cache directories from being tracked by git.
##########
demo/ci_run.sh:
##########
@@ -75,3 +82,6 @@ else
echo "Unknown app path: $app_path"
exit 1
fi
+
+# Always tear down the compose stack to release file locks and avoid cache
save issues
+docker compose down -v || true
Review Comment:
The `|| true` pattern silences all errors from `docker compose down -v`,
which could hide legitimate failures. Consider checking if containers are
running before attempting teardown, or at minimum log a warning if the teardown
fails: `docker compose down -v || echo 'Warning: Failed to tear down compose
stack'`.
```suggestion
docker compose down -v || echo 'Warning: Failed to tear down compose stack'
>&2
```
##########
.github/workflows/ci.yml:
##########
@@ -94,31 +94,54 @@ jobs:
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v5
+
+ # Cache Rust dependencies and build artifacts
+ - name: Cache Rust dependencies
+ uses: Swatinem/rust-cache@v2
+ with:
+ # Cache based on Python version and OS for better hit rate
+ shared-key: "python-binding-${{ matrix.os }}-py${{
matrix.python-version }}"
+ # Save cache even on failure to speed up retries
+ save-if: ${{ github.ref == 'refs/heads/main' || github.event_name ==
'pull_request' }}
- name: install uv and set the python version
uses: astral-sh/setup-uv@v7
with:
version: "0.7.19"
python-version: ${{ matrix.python-version }}
enable-cache: true
- - name: Setup Python venv
+ cache-dependency-glob: "python/pyproject.toml"
+
+ - name: Setup Python venv and build
run: |
make setup-venv
source .venv/bin/activate
make build
make develop
+
+ - name: Python unit tests
+ # Only compute coverage for one matrix combination to save time
+ if: ${{ !((matrix.os == 'ubuntu-24.04') && (matrix.python-version ==
'3.13')) }}
Review Comment:
[nitpick] The condition uses double negation `!((matrix.os ==
'ubuntu-24.04') && (matrix.python-version == '3.13'))` which is harder to read.
Consider using De Morgan's law to simplify this to `${{ matrix.os !=
'ubuntu-24.04' || matrix.python-version != '3.13' }}` for better clarity.
```suggestion
if: ${{ matrix.os != 'ubuntu-24.04' || matrix.python-version !=
'3.13' }}
```
##########
demo/compose.yaml:
##########
@@ -57,9 +57,13 @@ services:
build:
context: ./infra/runner
container_name: runner
Review Comment:
[nitpick] The `user` field references environment variables `HOST_UID` and
`HOST_GID` that are set in `ci_run.sh`, but there's no fallback or validation
if these variables are unset. Consider adding a comment explaining that these
variables must be set before running `docker compose up`, or documenting the
expected usage pattern.
```suggestion
container_name: runner
# NOTE: HOST_UID and HOST_GID must be set in your environment before
running `docker compose up`.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
