ci: dont do CI when it's not needed #2774
Conversation
WalkthroughAdds direct-package path filters for JS and Rust, exposes their outputs in the main tests workflow, and extends reusable JS and Rust package test workflows with a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant GH as GitHub Actions
participant CH as changes job
participant PF_RS as paths-filter (rs-packages-direct)
participant PF_JS as paths-filter (js-packages-direct)
participant RP_RS as rs-packages job (reusable)
participant RP_JS as js-packages job (reusable)
participant RW_RS as tests-rs-package.yml
participant RW_JS as tests-js-package.yml
participant Jobs as Gated Jobs
Dev->>GH: Open PR / push
GH->>CH: Run "changes" job
CH->>PF_RS: Apply .github/package-filters/rs-packages-direct.yml
CH->>PF_JS: Apply .github/package-filters/js-packages-direct.yml
PF_RS-->>CH: Output rs-packages-direct (JSON)
PF_JS-->>CH: Output js-packages-direct (JSON)
CH-->>RP_RS: Provide direct-packages = rs-packages-direct
CH-->>RP_JS: Provide direct-packages = js-packages-direct
RP_RS->>RW_RS: Call reusable RS workflow with inputs
RP_JS->>RW_JS: Call reusable JS workflow with inputs
alt Package in direct-packages
RW_RS->>Jobs: Run gated RS jobs (lint/format/...)
RW_JS->>Jobs: Run gated JS jobs (lint/...)
else Package not in direct-packages
RW_RS--x Jobs: Skip gated RS jobs
RW_JS--x Jobs: Skip gated JS jobs
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/package-filters/rs-packages-direct.yml (1)
1-45: Align rs-packages-direct.yml keys with the rs-packages matrixrs-packages-direct.yml contains keys that are not present in .github/package-filters/rs-packages.yml, which makes the contains(fromJSON(inputs.direct-packages), inputs.package) checks always false and will gate jobs too aggressively. Keys only in rs-packages-direct.yml: dapi-grpc, dashpay-contract, dpns-contract, dpp, drive, json-schema-compatibility-validator, masternode-reward-shares-contract, rs-dapi-client, token-history-contract, wallet-utils-contract, withdrawals-contract. Fix by adding matching keys to .github/package-filters/rs-packages.yml or renaming/removing entries in .github/package-filters/rs-packages-direct.yml so names exactly match the matrix. Files: .github/package-filters/rs-packages-direct.yml, .github/package-filters/rs-packages.yml.
.github/workflows/tests-rs-package.yml (1)
160-166: Bug: contradictory grep removes all candidates, making the check always passIn the append_only check, you pipe through grep "^-", then immediately grep -v "^-", which eliminates all deletions. This can mask forbidden deletions.
Apply:
- # Check append_only structures - if ! diff -u "${{ steps.base_structures.outputs.base_dir }}/$(basename $file).append_only.base" "$(basename $file).append_only.pr" | grep "^-" | grep -v "@append_only" | grep -v "^-" | grep -v "^///" | grep -v "^//" | grep -v "^-$"; then - echo "No deletions detected in @append_only structures in $file. Test passed." - else - echo "Deletions detected in @append_only structures in $file. Test failed." - exit 1 - fi + # Check append_only structures: fail if we find non-comment deletions (excluding the annotation itself) + if diff -u "${{ steps.base_structures.outputs.base_dir }}/$(basename $file).append_only.base" "$(basename $file).append_only.pr" \ + | grep -E '^-([^-/]|$)' \ + | grep -v '@append_only' \ + | grep -v '^\-\s*///' \ + | grep -v '^\-\s*//' \ + | grep -v '^\-$' ; then + echo "Deletions detected in @append_only structures in $file. Test failed." + exit 1 + else + echo "No deletions detected in @append_only structures in $file. Test passed." + fi
🧹 Nitpick comments (3)
.github/package-filters/rs-packages-direct.yml (1)
1-45: Optional: add a “global-rs” trigger for workspace-level Rust config changesRight now, if only workspace-level files change (e.g., Cargo.toml, .cargo/config.toml, rustfmt.toml), direct-packages will be empty and lint/format/unused_deps won’t run for any crate. Consider introducing a global-rs filter (matching those files) and wiring a boolean input to the reusable workflow so you can OR it with the contains(...) checks. Happy to draft the wiring if you want.
.github/workflows/tests-rs-package.yml (2)
123-123: Prefer base SHA over base ref for deterministic diffingUsing github.event.pull_request.base.ref checks out the moving branch head. To avoid races, use the immutable base SHA.
Apply:
- with: - ref: ${{ github.event.pull_request.base.ref }} + with: + ref: ${{ github.event.pull_request.base.sha }}
214-214: Style: unify if expression styleElsewhere you use plain if: condition without ${{ }}. For consistency, drop ${{ }} here as well.
Apply:
- if: ${{ inputs.check-each-feature && contains(fromJSON(inputs.direct-packages), inputs.package) }} + if: inputs.check-each-feature && contains(fromJSON(inputs.direct-packages), inputs.package)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/package-filters/rs-packages-direct.yml(1 hunks).github/workflows/tests-rs-package.yml(5 hunks).github/workflows/tests.yml(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
📚 Learning: 2025-09-07T22:18:50.883Z
Learnt from: CR
PR: dashpay/platform#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T22:18:50.883Z
Learning: Applies to packages/rs-sdk-ffi/**/*.{h,rs} : Follow unified SDK function prefixes: dash_core_sdk_* for Core, dash_sdk_* for Platform, dash_unified_sdk_* for unified APIs
Applied to files:
.github/package-filters/rs-packages-direct.yml
🪛 actionlint (1.7.7)
.github/workflows/tests.yml
46-46: the runner of "dorny/paths-filter@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (dashpay-contract) / Tests
- GitHub Check: Rust packages (withdrawals-contract) / Tests
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (json-schema-compatibility-validator) / Tests
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Tests
- GitHub Check: Rust packages (token-history-contract) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (wallet-utils-contract) / Tests
- GitHub Check: Rust packages (dpns-contract) / Tests
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (3)
.github/workflows/tests-rs-package.yml (2)
12-15: New input looks good; JSON default prevents fromJSON errorsdirect-packages with default '[]' is the right shape for fromJSON() and keeps callers backward compatible.
21-21: Gating will skip these jobs when no direct packages are detected — confirm intentcontains(fromJSON(inputs.direct-packages), inputs.package) means Linting/Formatting/Unused deps won’t run unless the package changed directly. If that’s the intended CI reduction, great; otherwise consider a fallback (e.g., run for all when direct-packages == []).
Also applies to: 59-59, 80-80
.github/workflows/tests.yml (1)
93-94: Wiring LGTM: direct-packages propagationPassing needs.changes.outputs.rs-packages-direct into the reusable workflow matches the new input; no quoting issues since it’s a JSON string already.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/package-filters/js-packages-direct.yml (1)
1-48: Add missing workspace packages to .github/package-filters/js-packages-direct.ymlThe filter is missing mappings for:
- @dashevo/bench-suite
- @dashevo/feature-flags-contract
- @dashevo/keyword-search-contract
- dash
- dashmate
- wasm-drive-verify
- wasm-sdk
- wasm-sdk-tests
- wasm-sdk-ui-automation
Add entries for these (or confirm intentional omission). Consider excluding generated output dirs (e.g., wasm-dpp/pkg) to avoid false positives.
♻️ Duplicate comments (4)
.github/workflows/tests.yml (4)
37-41: Also bump the existing JS filter to v3This is consistent with the new steps and avoids Node runtime deprecations.
- - uses: dorny/paths-filter@v2 + - uses: dorny/paths-filter@v3 id: filter-js with: filters: .github/package-filters/js-packages.yml
47-50: Also bump the existing RS filter to v3Same deprecation applies here.
- - uses: dorny/paths-filter@v2 + - uses: dorny/paths-filter@v3 id: filter-rs with: filters: .github/package-filters/rs-packages.yml
42-46: Upgrade paths-filter to v3 for Node20 runnersNew step still uses dorny/paths-filter@v2, which is flagged by actionlint as too old on current runners.
Apply:
- - uses: dorny/paths-filter@v2 + - uses: dorny/paths-filter@v3 id: filter-js-direct with: filters: .github/package-filters/js-packages-direct.yml
52-56: Same here: bump to v3Mirror the fix for RS direct filter.
- - uses: dorny/paths-filter@v2 + - uses: dorny/paths-filter@v3 id: filter-rs-direct with: filters: .github/package-filters/rs-packages-direct.yml
🧹 Nitpick comments (3)
.github/package-filters/js-packages-direct.yml (1)
22-24: Consider ignoring generated wasm artifactsIf packages/wasm-dpp/pkg/** is committed, filter will treat rebuilt artifacts as “direct changes” and re-trigger CI. Optionally add a negation rule.
Apply:
'@dashevo/wasm-dpp': - packages/wasm-dpp/** + - '!packages/wasm-dpp/pkg/**'.github/workflows/tests-js-package.yml (1)
46-75: Optional: consider gating tests, or document why notIf the goal is “don’t redo CI,” you might optionally gate tests similarly. If transitive-change coverage is desired, leave as-is but add a note in PR description.
Example (only if desired):
- test: + test: name: Tests runs-on: ubuntu-24.04 timeout-minutes: 15 permissions: id-token: write contents: read - if: ${{ !inputs.skip-tests }} + if: ${{ !inputs.skip-tests && contains(fromJSON(inputs.direct-packages), inputs.package) }}.github/workflows/tests.yml (1)
21-25: Minor: skip changes job for PR drafts only where applicableCondition already handles drafts. Optionally document the rationale in the workflow header for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/package-filters/js-packages-direct.yml(1 hunks).github/workflows/tests-js-package.yml(2 hunks).github/workflows/tests.yml(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: dashpay/platform#0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T22:20:55.344Z
Learning: Applies to packages/** : Place all source packages under packages/* (JS/TS packages and Rust crates)
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
📚 Learning: 2025-09-07T22:18:50.883Z
Learnt from: CR
PR: dashpay/platform#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T22:18:50.883Z
Learning: Applies to packages/rs-sdk-ffi/**/*.{h,rs} : Follow unified SDK function prefixes: dash_core_sdk_* for Core, dash_sdk_* for Platform, dash_unified_sdk_* for unified APIs
Applied to files:
.github/package-filters/js-packages-direct.yml
🪛 actionlint (1.7.7)
.github/workflows/tests.yml
42-42: the runner of "dorny/paths-filter@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
47-47: the runner of "dorny/paths-filter@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
52-52: the runner of "dorny/paths-filter@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (withdrawals-contract) / Tests
- GitHub Check: Rust packages (dashpay-contract) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (masternode-reward-shares-contract) / Tests
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (6)
.github/package-filters/js-packages-direct.yml (1)
19-21: Confirm package dir namesDouble-check these map to actual dirs:
- @dashevo/grpc-common -> packages/js-grpc-common/**
- @dashevo/dapi-client -> packages/js-dapi-client/**
If repo naming differs, update to avoid silent non-matches.
Also applies to: 35-36
.github/workflows/tests-js-package.yml (2)
8-11: Good: input for direct packages with safe defaultThe new workflow_call input enables gating without breaking callers. Looks correct.
26-26: Gating lint to direct changes is appropriatecontains(fromJSON(...), inputs.package) is the right pattern for arrays. Keep.
.github/workflows/tests.yml (3)
28-30: Outputs for direct package sets wired correctlyjs-packages-direct and rs-packages-direct are exposed from changes job as expected.
96-100: Direct package propagation to RS workflow looks rightInput wiring matches changes.outputs.rs-packages-direct. Good.
134-139: Direct package propagation to JS workflow looks rightCorrectly forwards js-packages-direct to the called workflow.
QuantumExplorer
changed the title
QuantumExplorer
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/tests.yml (1)
38-63: Paths-filter upgraded to v3 across change-detection stepsNode20-compatible and matches prior suggestion.
🧹 Nitpick comments (2)
.github/package-filters/rs-packages-no-workflows.yml (1)
50-58: Intentional mix of RS and proto/JS paths under dapi-grpc?dapi-grpc includes rs-platform-version, rs-dapi-grpc-macros, and packages/dapi-grpc/**. If the proto definitions live in packages/dapi-grpc, this is correct; otherwise consider switching to an rs- prefixed path.
.github/workflows/tests.yml (1)
21-26: Optional: declare minimal permissions for the change-detection jobpaths-filter can work without checkout on PRs using the API and requires pull-requests: read. You already check out, but declaring explicit permissions reduces footguns later.
Apply at job level:
jobs: changes: name: Determine changed packages + permissions: + contents: read + pull-requests: read
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/package-filters/js-packages-no-workflows.yml(1 hunks).github/package-filters/rs-packages-no-workflows.yml(1 hunks).github/package-filters/test-suite-triggers.yml(1 hunks).github/workflows/tests.yml(5 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
PR: dashpay/platform#0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T22:20:55.344Z
Learning: Applies to packages/** : Place all source packages under packages/* (JS/TS packages and Rust crates)
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
📚 Learning: 2025-09-07T22:18:50.883Z
Learnt from: CR
PR: dashpay/platform#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T22:18:50.883Z
Learning: Applies to packages/rs-sdk-ffi/**/*.{h,rs} : Follow unified SDK function prefixes: dash_core_sdk_* for Core, dash_sdk_* for Platform, dash_unified_sdk_* for unified APIs
Applied to files:
.github/package-filters/js-packages-no-workflows.yml.github/package-filters/rs-packages-no-workflows.yml.github/package-filters/test-suite-triggers.yml
📚 Learning: 2025-09-03T16:37:11.605Z
Learnt from: QuantumExplorer
PR: dashpay/platform#2756
File: packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs:11-11
Timestamp: 2025-09-03T16:37:11.605Z
Learning: In packages/rs-dpp/Cargo.toml, the abci feature already includes core_rpc_client, and core_rpc_client is defined as ["dep:dashcore-rpc"]. This means rs-drive-abci can access dashcore-rpc types through dpp when using the abci feature.
Applied to files:
.github/package-filters/rs-packages-no-workflows.yml
📚 Learning: 2025-09-07T22:20:55.344Z
Learnt from: CR
PR: dashpay/platform#0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T22:20:55.344Z
Learning: Applies to packages/*/tests/**/*.rs : Rust unit/integration tests must not perform network calls; mock dependencies
Applied to files:
.github/package-filters/test-suite-triggers.yml
📚 Learning: 2025-09-07T22:20:55.344Z
Learnt from: CR
PR: dashpay/platform#0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T22:20:55.344Z
Learning: Applies to packages/!(platform-test-suite)/*/tests/**/*.{js,jsx,ts,tsx} : JS/TS unit/integration tests must not perform network calls; mock dependencies
Applied to files:
.github/package-filters/test-suite-triggers.yml
📚 Learning: 2025-09-07T22:20:55.344Z
Learnt from: CR
PR: dashpay/platform#0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T22:20:55.344Z
Learning: Applies to packages/platform-test-suite/** : Keep end-to-end tests and helpers in packages/platform-test-suite
Applied to files:
.github/package-filters/test-suite-triggers.yml
📚 Learning: 2025-09-07T22:20:55.344Z
Learnt from: CR
PR: dashpay/platform#0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T22:20:55.344Z
Learning: Applies to packages/platform-test-suite/** : Keep all E2E tests exclusively in packages/platform-test-suite
Applied to files:
.github/package-filters/test-suite-triggers.yml
📚 Learning: 2024-11-13T10:31:30.891Z
Learnt from: lklimek
PR: dashpay/platform#2318
File: .github/workflows/tests-build-image.yml:45-45
Timestamp: 2024-11-13T10:31:30.891Z
Learning: In the dashpay/platform repository, changes to `.github/workflows/tests-build-image.yml` that switch the Docker image platform from `linux/arm64` to `linux/amd64` for testing purposes are acceptable when required to run on GitHub-hosted runners. ARM64 testing is covered on the testnet.
Applied to files:
.github/package-filters/test-suite-triggers.yml
⏰ 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). (2)
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (10)
.github/package-filters/js-packages-no-workflows.yml (2)
22-37: Anchors and transitive deps for wasm-dpp look solidGood use of YAML anchors/aliases to pull in dependent contracts and RS crates; paths-filter v3 supports this pattern. No issues spotted.
62-80: Aggregate group composition for dash/platform-test-suitedash aggregates dapi-client and wallet-lib and is reused by platform-test-suite. This matches the repo layout under packages/* and should keep CI tight.
If helpful, I can generate a quick script to diff keys here against js-packages-direct.yml to ensure 1:1 coverage of package names.
.github/package-filters/rs-packages-no-workflows.yml (2)
22-41: dpp/wasm-dpp dependency fan-in captured correctlydpp includes contracts and RS platform libs; wasm-dpp builds on dpp. This mirrors crate relationships and should prevent unnecessary CI re-runs elsewhere.
59-71: dash-sdk / rs-sdk-ffi coverageBoth include rs-drive-proof-verifier and reuse dapi_client + drive anchors. Looks consistent and should gate downstream crates as intended.
.github/package-filters/test-suite-triggers.yml (1)
1-33: Reasonable, conservative triggers for the full Test SuiteThe run list focuses on components that influence local-network/E2E behavior plus system contracts and helper scripts. This aligns with “don’t redo CI.”
Do we want changes to packages/rs-dapi-grpc-macros/** to trigger the suite as well, or is coverage via rs-dapi-client sufficient?
.github/workflows/tests.yml (5)
27-32: Expose direct and suite outputs from change detectionAdding js-packages-direct, rs-packages-direct, and test-suite outputs keeps downstream gating explicit. Looks good.
93-107: Wire rs direct-packages through to the reusable workflowPassing direct-packages enables job-level gating inside tests-rs-package.yml. Assuming the called workflow consumes this input, this is good to go.
I can auto-check that .github/workflows/tests-rs-package.yml defines an input named direct-packages and uses it for gating. Want a quick repo scan script?
130-146: Wire JS direct-packages to the reusable workflowSame note as RS: ensure tests-js-package.yml defines and uses the direct-packages input.
Happy to generate a short script to validate the input contract in tests-js-package.yml and tests-rs-package.yml.
213-243: Suite gating via needs.changes.outputs.test-suiteThe boolean output from filter-test-suite maps cleanly to the job if condition. Matrix is unchanged; this should avoid running the suite when unrelated files change.
244-253: Functional packages job gated by same suite flagConsistent with the new suite gating.
shumkov
changed the title
According to existing code I don't think this statement is correct: ABCI changes will not mark dpp as changed. It's not in paths: And then bellow we run rs-package tests only for changed packages: So I don't think the problem you're pointing to could exist. Could you provide a link to a real life example of such CI run? |
CI was being run when it was not needed. For example we would do rs-dpp tests if only a small change happened in rs-drive-abci. There's no point for this as it would not have changed. Waiting for CI takes forever as well. So with this PR we try to reduce things.
Summary by CodeRabbit