CI: detect-changes workflow to only check the affected components on the CI side#5843
CI: detect-changes workflow to only check the affected components on the CI side#5843
Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
…orm gates The step-level platform-check in sample-application, sample-application-expo, and e2e-v2 workflows gated on needs_ios/needs_android, which only track SDK source and native code changes. When only sample app or performance test files changed, the job-level condition correctly triggered (via needs_sample_react_native, needs_sample_expo), but every platform step was skipped — runners allocated, zero meaningful work done, false green reported. Fix by checking sample_react_native/sample_expo/perf_tests as the first condition in each platform-check step. If the relevant files changed, all platforms proceed without skipping. Affected workflows: - sample-application.yml: build and test jobs (sample_react_native) - sample-application-expo.yml: build job (sample_expo) - e2e-v2.yml: metrics job (perf_tests)
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…r Expo sample
The working-directory value had a leading space (' samples/expo') due to
an unnecessary ternary expression. Since the step is already guarded by
an if condition checking matrix.platform == 'ios', the ternary was
redundant. Simplified to the plain path 'samples/expo'.
| platform: 'macos' | ||
| steps: | ||
| - name: Check if platform is needed | ||
| id: platform-check |
There was a problem hiding this comment.
nit: Wdyt of extracting the platform-check in a separate workflow and reuse it? (this can be part of a separate improvement PR)
There was a problem hiding this comment.
Yes, let's better handle that separately
| needs: [diff_check] | ||
| if: ${{ needs.diff_check.outputs.skip_ci != 'true' }} | ||
| needs: [diff_check, detect-changes] | ||
| if: ${{ needs.diff_check.outputs.skip_ci != 'true' && needs.detect-changes.outputs.needs_android == 'true' }} |
There was a problem hiding this comment.
Suggestion/idea for a future improvement:
It's nice that no runner is allocated here when the if is false here since each job is a single platform.
The other workflows use platform as a matrix dimension alongside others. We could apply the native-tests pattern on the other workflows too by splitting into separate build-ios / build-android / build-macos jobs, each with their own job-level if and a reduced matrix (without platform). That would avoid runner allocation entirely.
There was a problem hiding this comment.
I would also rather do it in another PR but that's a great suggestion. Will create an issue out of it!
antonis
left a comment
antonis
left a comment
There was a problem hiding this comment.
Awesome work @alwx 🎸
I think it would be nice to fix https://github.com/getsentry/sentry-react-native/pull/5843/changes#r2965213616 and the cursor comment for the web before merging but none is blocking imo.
I've also added a couple of suggestions which I think we could handle in separate PRs later.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| echo "Skipping Android — no relevant changes detected." | ||
| elif [[ "${{ matrix.platform }}" == "web" && "${{ needs.detect-changes.outputs.js_source }}" != "true" ]]; then | ||
| echo "skip=true" >> "$GITHUB_OUTPUT" | ||
| echo "Skipping Web — no relevant JS changes detected." |
There was a problem hiding this comment.
Expo web build skipped during CI-only or main/release changes
Medium Severity
The web platform-check in sample-application-expo.yml only considers js_source to decide whether to skip, while the needs_ios/needs_android convenience outputs also account for ci changes and IS_MAIN_OR_RELEASE. When only CI files change (or on a push to main), needs_sample_expo is true at the job level, and iOS/Android correctly run via their needs_* flags, but the web build is incorrectly skipped because js_source is false. This violates the stated intent that CI changes "should trigger everything" and that main/release pushes always run all jobs.
Additional Locations (1)
2 participants
Fixes #5786
Fixes #5785
📢 Type of change
📜 Description
.github/workflows/detect-changes.yml— New reusable workflow that usesdorny/paths-filterv3 with the file filters to produceneeds_iosandneeds_androidboolean outputs.That makes it all skip certain checks — for example, if the changes are in sample apps then there is no need to re-run tests for the whole SDK.
During the review I would love you to carefully look at
detect-changes.yml, as well as the conditions specified inplatform-checksteps.💡 Motivation and Context
We want our CI to be fast and don't do redundant jobs.
📝 Checklist
sendDefaultPIIis enabled