chore: Add action to warn about potentially risky PR changes #2315
Conversation
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
.github/file-filters.yml
Outdated
| # This is used by the action https://github.com/dorny/paths-filter | ||
|
|
||
| high_risk_code: &high_risk_code | ||
| - ".github/file-filters.yml" |
There was a problem hiding this comment.
this is for testing and will be removed before merging
need to align on set of potentially higher-risk files before merging as well
kahest
requested review from
buenaflor,
krystofwoldrich,
martinhaintz and
stefanosiano
as code owners
.github/file-filters.yml
Outdated
| # This is used by the action https://github.com/dorny/paths-filter | ||
|
|
||
| high_risk_code: &high_risk_code | ||
| - ".github/file-filters.yml" |
There was a problem hiding this comment.
cool!
is this how we reference file paths?
high_risk_code: &high_risk_code
- "dart/lib/src/risky_file.dart"
- "flutter/lib/src/risky_file.dart"There was a problem hiding this comment.
yep, just full repo paths, you can check https://github.com/getsentry/sentry-cocoa/blob/main/.github/file-filters.yml for inspiration
There was a problem hiding this comment.
Okay, I'll think about the code paths that would benefit from this
There was a problem hiding this comment.
What do we define as risky? Is this strictly defined as code paths that are prone to crashing?
There was a problem hiding this comment.
the way to think about this is - what can break the users in significant ways which is not currently automatically ensured/validated/tested/etc. in a different way - also try to strike a good balance between missing things and over-reporting (I wouldn't expect this warning to need to be shown in every other PR, for example)
Android Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 90a08ea | 477.25 ms | 534.10 ms | 56.85 ms |
| c57d3b7 | 413.56 ms | 508.80 ms | 95.24 ms |
| 4c78360 | 402.29 ms | 462.04 ms | 59.76 ms |
| dd76eef | 461.37 ms | 540.55 ms | 79.18 ms |
| 21562c5 | 304.31 ms | 348.91 ms | 44.61 ms |
| a609134 | 350.12 ms | 404.12 ms | 54.00 ms |
| 294b7f0 | 367.83 ms | 430.00 ms | 62.17 ms |
| 256df44 | 447.58 ms | 485.84 ms | 38.25 ms |
| 103eb14 | 405.50 ms | 444.30 ms | 38.80 ms |
| 8fa3934 | 340.64 ms | 407.92 ms | 67.28 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 90a08ea | 6.49 MiB | 7.55 MiB | 1.06 MiB |
| c57d3b7 | 6.33 MiB | 7.30 MiB | 992.08 KiB |
| 4c78360 | 6.33 MiB | 7.27 MiB | 954.08 KiB |
| dd76eef | 6.35 MiB | 7.40 MiB | 1.05 MiB |
| 21562c5 | 6.16 MiB | 7.13 MiB | 1001.31 KiB |
| a609134 | 5.94 MiB | 6.95 MiB | 1.01 MiB |
| 294b7f0 | 6.33 MiB | 7.26 MiB | 950.21 KiB |
| 256df44 | 6.52 MiB | 7.59 MiB | 1.06 MiB |
| 103eb14 | 6.52 MiB | 7.59 MiB | 1.06 MiB |
| 8fa3934 | 6.06 MiB | 7.09 MiB | 1.03 MiB |
iOS Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 24b6e60 | 1250.69 ms | 1268.63 ms | 17.94 ms |
| 2e1e4ae | 1254.41 ms | 1278.55 ms | 24.14 ms |
| 6a5a65d | 1237.22 ms | 1250.29 ms | 13.07 ms |
| 73d70bf | 1249.08 ms | 1268.41 ms | 19.33 ms |
| 7faee57 | 1232.65 ms | 1246.10 ms | 13.45 ms |
| 43760f9 | 1216.07 ms | 1238.51 ms | 22.44 ms |
| 6daa837 | 1250.42 ms | 1265.60 ms | 15.18 ms |
| d4120ac | 1260.61 ms | 1274.09 ms | 13.47 ms |
| d883d62 | 1221.39 ms | 1230.18 ms | 8.80 ms |
| 43abc4f | 1248.33 ms | 1272.86 ms | 24.52 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 24b6e60 | 8.32 MiB | 9.38 MiB | 1.06 MiB |
| 2e1e4ae | 8.34 MiB | 9.65 MiB | 1.31 MiB |
| 6a5a65d | 8.34 MiB | 9.65 MiB | 1.31 MiB |
| 73d70bf | 8.38 MiB | 9.70 MiB | 1.33 MiB |
| 7faee57 | 8.33 MiB | 9.64 MiB | 1.31 MiB |
| 43760f9 | 8.29 MiB | 9.36 MiB | 1.07 MiB |
| 6daa837 | 8.33 MiB | 9.40 MiB | 1.07 MiB |
| d4120ac | 8.28 MiB | 9.34 MiB | 1.06 MiB |
| d883d62 | 8.29 MiB | 9.36 MiB | 1.07 MiB |
| 43abc4f | 8.33 MiB | 9.54 MiB | 1.21 MiB |
|
@buenaflor do you have more additions in mind? I wanna wrap this up soon-ish :) |
|
No, I was about to write that this is good now :D If I see something over the next few days that's alarming I'll add it |
cool 👍 this is meant to be a living list anyway, whenever something makes sense to add, we do it. just need a ✅ now 😅 |
📜 Description
Implement a mechanism that automatically adds an "alert" comment to PRs if they include changes to files we think have higher potential to break things than others.
💡 Motivation and Context
same as getsentry/sentry-java#3726
💚 How did you test it?
✅ by adding the file-filter.yml introduced in this PR to the list of potentially risky changes and ensuring that an alert is shown
📝 Checklist
I added tests to verify changesNo new PII added or SDK only sends newly added PII ifsendDefaultPiiis enabledI updated the docs if neededAll tests passingNo breaking changes🔮 Next steps
#skip-changelog