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}
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." | ||
| else | ||
| echo "skip=false" >> "$GITHUB_OUTPUT" | ||
| fi |
There was a problem hiding this comment.
Expo web platform never skipped by platform-check
Low Severity
The platform-check in sample-application-expo.yml only handles ios and android platforms explicitly. The web platform (added via matrix include) falls through to the else branch and always gets skip=false. This means when the job runs due to ios_native or android_native changes alone (without any JS source or expo sample changes), the web build runs unnecessarily — web doesn't depend on native code at all. A web-specific branch could check needs_ios, needs_android, or js_source to skip appropriately.
There was a problem hiding this comment.
This looks valid. Something like the following might work
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."
…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)
| 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.
| - 'packages/core/plugin/**' | ||
| - 'packages/core/scripts/**' | ||
| - 'packages/core/package.json' | ||
| - 'packages/core/tsconfig.json' |
There was a problem hiding this comment.
No filter matches yarn.lock, root package.json, or .yarnrc.yml. A lockfile-only PR would have all detect-changes outputs as false, skipping all jobs.
| - 'packages/core/tsconfig.json' | |
| - 'packages/core/tsconfig.json' | |
| - 'yarn.lock' | |
| - 'package.json' | |
| - '.yarnrc.yml' |
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.
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