Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a new Cloudberry-backed GitHub Actions workflow for kafka_fdw regression testing and updates the Kafka test script to use mirror fallback downloads, detached daemon startup, and broker readiness checks. ChangesKafka FDW CI Pipeline
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (2)
.github/workflows/cloudberry-kafka-fdw-ci.yml (2)
45-51: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
editedtrigger fires on non-code PR edits.Including
editedinpull_request.types(Line 50) re-runs this expensive CI pipeline (up to 180+120 minutes) whenever the PR title/body/labels change, with no code diff involved.♻️ Proposed fix
pull_request: branches: [ main, master ] - types: [ opened, synchronize, reopened, edited, ready_for_review ] + types: [ opened, synchronize, reopened, ready_for_review ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cloudberry-kafka-fdw-ci.yml around lines 45 - 51, The pull_request trigger in the cloudberry-kafka-fdw CI workflow is too broad because it includes the edited event, which reruns the pipeline for non-code PR metadata changes. Update the workflow trigger definition to remove edited from the pull_request.types list while keeping the code-relevant events such as opened, synchronize, reopened, and ready_for_review.
75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnpinned
:latestcontainer tag hurts reproducibility.Both jobs use
apache/incubator-cloudberry:cbdb-build-rocky9-latest. If the tag is updated upstream between the build and test job runs (or between re-runs), the toolchain used to build Cloudberry can silently drift, causing flaky/irreproducible CI results.Also applies to: 188-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/cloudberry-kafka-fdw-ci.yml at line 75, The workflow jobs use an unpinned Cloudberry container tag, which can drift and make CI non-reproducible. Update the image reference in the affected job definitions in cloudberry-kafka-fdw-ci so it points to a pinned, immutable Apache/incubator-cloudberry tag or digest instead of cbdb-build-rocky9-latest, and apply the same change to the matching job that also uses this image.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/cloudberry-kafka-fdw-ci.yml:
- Around line 105-112: Set the Cloudberry checkout in the workflow to avoid
persisting GitHub credentials, since the checked-out `${CLOUDBERRY_DIR}` is
later archived and uploaded as an artifact. Update the `actions/checkout@v4`
step for “Checkout Cloudberry Source” to disable credential persistence, and
verify the tarball/artifact creation steps that package `cloudberry-source` do
not include `.git/config` with token data. Use the `Checkout Cloudberry Source`
job step and the `cloudberry-source` archive/upload flow to locate the fix.
- Around line 218-222: The kafka_fdw checkout in the workflow still leaves the
GitHub token persisted in .git/config, so update the actions/checkout step under
“Checkout kafka_fdw” to disable credential persistence. Keep the existing
checkout settings in place, and add persist-credentials: false to the
actions/checkout@v4 configuration so later steps cannot reuse the token.
---
Nitpick comments:
In @.github/workflows/cloudberry-kafka-fdw-ci.yml:
- Around line 45-51: The pull_request trigger in the cloudberry-kafka-fdw CI
workflow is too broad because it includes the edited event, which reruns the
pipeline for non-code PR metadata changes. Update the workflow trigger
definition to remove edited from the pull_request.types list while keeping the
code-relevant events such as opened, synchronize, reopened, and
ready_for_review.
- Line 75: The workflow jobs use an unpinned Cloudberry container tag, which can
drift and make CI non-reproducible. Update the image reference in the affected
job definitions in cloudberry-kafka-fdw-ci so it points to a pinned, immutable
Apache/incubator-cloudberry tag or digest instead of cbdb-build-rocky9-latest,
and apply the same change to the matching job that also uses this image.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 40c02321-f2d8-4a2e-ae29-033ec203d97a
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/cloudberry-kafka-fdw-ci.yml
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
| - name: Checkout Cloudberry Source | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: ${{ env.CLOUDBERRY_REPO }} | ||
| ref: ${{ env.CLOUDBERRY_REF }} | ||
| fetch-depth: 1 | ||
| submodules: true | ||
| path: ${{ env.CLOUDBERRY_DIR }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Cloudberry source artifact can leak the job's GITHUB_TOKEN.
actions/checkout at Line 105-112 doesn't set persist-credentials: false, so the injected token remains in .git/config inside ${CLOUDBERRY_DIR}. That whole directory is then tarred (Line 159) and uploaded as the cloudberry-source artifact (Line 174-180), packaging the credential into a downloadable artifact.
🔒 Proposed fix
- name: Checkout Cloudberry Source
uses: actions/checkout@v4
with:
repository: ${{ env.CLOUDBERRY_REPO }}
ref: ${{ env.CLOUDBERRY_REF }}
fetch-depth: 1
submodules: true
path: ${{ env.CLOUDBERRY_DIR }}
+ persist-credentials: falseAs per static analysis hints, zizmor flagged "credential persistence through GitHub Actions artifacts (artipacked)" at this checkout step.
Also applies to: 154-180
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 105-112: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/cloudberry-kafka-fdw-ci.yml around lines 105 - 112, Set
the Cloudberry checkout in the workflow to avoid persisting GitHub credentials,
since the checked-out `${CLOUDBERRY_DIR}` is later archived and uploaded as an
artifact. Update the `actions/checkout@v4` step for “Checkout Cloudberry Source”
to disable credential persistence, and verify the tarball/artifact creation
steps that package `cloudberry-source` do not include `.git/config` with token
data. Use the `Checkout Cloudberry Source` job step and the `cloudberry-source`
archive/upload flow to locate the fix.
Source: Linters/SAST tools
| - name: Checkout kafka_fdw | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
| path: ${{ env.KAFKA_FDW_DIR }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Same missing persist-credentials: false on the kafka_fdw checkout.
Not packaged into an artifact in this cohort, but the token still persists in .git/config for the remainder of the job, reachable by any later step/script.
🔒 Proposed fix
- name: Checkout kafka_fdw
uses: actions/checkout@v4
with:
fetch-depth: 0
path: ${{ env.KAFKA_FDW_DIR }}
+ persist-credentials: falseAs per static analysis hints, zizmor flagged this checkout with the same artipacked warning.
📝 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.
| - name: Checkout kafka_fdw | |
| uses: actions/checkout@v4 | |
| with: | |
| fetch-depth: 0 | |
| path: ${{ env.KAFKA_FDW_DIR }} | |
| - name: Checkout kafka_fdw | |
| uses: actions/checkout@v4 | |
| with: | |
| fetch-depth: 0 | |
| path: ${{ env.KAFKA_FDW_DIR }} | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 218-222: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/cloudberry-kafka-fdw-ci.yml around lines 218 - 222, The
kafka_fdw checkout in the workflow still leaves the GitHub token persisted in
.git/config, so update the actions/checkout step under “Checkout kafka_fdw” to
disable credential persistence. Keep the existing checkout settings in place,
and add persist-credentials: false to the actions/checkout@v4 configuration so
later steps cannot reuse the token.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/run_kafka.sh`:
- Around line 20-27: The Kafka download fallback in run_kafka.sh can leave a
partial KAFKA_ARCHIVE from the first wget, causing the retry to save a second
copy while the corrupted file still remains. Update the download logic around
the wget calls so the target archive path is always overwritten or cleaned up
before retrying, and ensure the later tar extraction uses a valid complete
archive from the KAFKA_ARCHIVE variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b59a2e77-a323-4949-a148-57350781de18
📒 Files selected for processing (2)
.github/workflows/cloudberry-kafka-fdw-ci.ymltest/run_kafka.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/cloudberry-kafka-fdw-ci.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/cloudberry-kafka-fdw-ci.yml:
- Around line 326-349: In the CI workflow step that runs `make installcheck` for
planner and ORCA, capture each run’s exit status instead of letting `set -euo
pipefail` abort after the first failure. Update the shell block around the
planner and ORCA `make installcheck` calls so both runs always execute, and
store their statuses for the final summary/exit decision. Also preserve separate
diff artifacts for each mode by copying or renaming `regression.diffs`/related
outputs after each run before the next one overwrites them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f36da68a-13b7-443a-b0f8-185a0469e8f0
📒 Files selected for processing (2)
.github/workflows/cloudberry-kafka-fdw-ci.ymltest/run_kafka.sh
Summary by CodeRabbit
kafka_fdwintegration/regression tests on a demo cluster.