Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: fix caching in some CI jobs #2479

Merged
merged 3 commits into from
Dec 2, 2022

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Dec 1, 2022

We were trying to compute a cache key on an invalid path, which did not invalidate the cached app builds when they should have been to test changes to the SDK.

I actually also found a build error on the default branch in trending movies indicating that it hasn't been built since the change that made that code invalid: #2480

#skip-changelog

@armcknight armcknight changed the title Armcknight/ci/disable trending movies caching ci: disable trending movies caching Dec 1, 2022
@armcknight armcknight changed the base branch from master to 8.0.0 December 1, 2022 19:25
@codecov
Copy link

codecov bot commented Dec 1, 2022

Codecov Report

Merging #2479 (8a09831) into 8.0.0 (5da2d48) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 8a09831 differs from pull request most recent head 961dd8a. Consider uploading reports for the commit 961dd8a to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##            8.0.0    #2479      +/-   ##
==========================================
+ Coverage   88.32%   88.36%   +0.03%     
==========================================
  Files         178      178              
  Lines       15201    15206       +5     
  Branches     5280     5279       -1     
==========================================
+ Hits        13427    13437      +10     
- Misses       1584     1593       +9     
+ Partials      190      176      -14     
Impacted Files Coverage Δ
Sources/Sentry/NSDictionary+SentrySanitize.m 48.27% <0.00%> (-32.98%) ⬇️
Sources/Sentry/SentryThreadInspector.m 96.87% <0.00%> (-1.57%) ⬇️
Sources/Sentry/SentryBacktrace.cpp 90.85% <0.00%> (-1.22%) ⬇️
Sources/Sentry/SentryTime.mm 40.00% <0.00%> (ø)
Sources/Sentry/SentryTracer.m 94.80% <0.00%> (ø)
Sources/Sentry/SentryDevice.mm 50.40% <0.00%> (ø)
Sources/Sentry/SentryScreenshot.m 65.85% <0.00%> (ø)
Sources/Sentry/SentryCrashWrapper.m 75.00% <0.00%> (ø)
Sources/Sentry/SentryUIApplication.m 0.00% <0.00%> (ø)
Sources/Sentry/SentryThreadHandle.cpp 68.22% <0.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd2e101...961dd8a. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Dec 1, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1242.98 ms 1267.91 ms 24.93 ms
Size 20.75 KiB 383.77 KiB 363.02 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
9cb6c52 1201.08 ms 1211.37 ms 10.29 ms
5da2d48 1209.52 ms 1232.16 ms 22.64 ms
3936dd2 1214.98 ms 1242.24 ms 27.26 ms
bbf5334 1216.84 ms 1238.82 ms 21.98 ms
c9129b6 1231.86 ms 1270.11 ms 38.25 ms
d914052 1236.52 ms 1251.56 ms 15.04 ms
d8c347f 1240.53 ms 1257.70 ms 17.17 ms
034ff5e 1231.22 ms 1255.37 ms 24.14 ms
1ef804d 1243.26 ms 1255.90 ms 12.64 ms
d446105 1237.06 ms 1261.34 ms 24.28 ms

App size

Revision Plain With Sentry Diff
9cb6c52 20.75 KiB 382.12 KiB 361.36 KiB
5da2d48 20.75 KiB 383.77 KiB 363.01 KiB
3936dd2 20.75 KiB 383.29 KiB 362.53 KiB
bbf5334 20.75 KiB 383.37 KiB 362.62 KiB
c9129b6 20.75 KiB 381.81 KiB 361.06 KiB
d914052 20.75 KiB 383.89 KiB 363.13 KiB
d8c347f 20.75 KiB 383.77 KiB 363.02 KiB
034ff5e 20.75 KiB 383.83 KiB 363.07 KiB
1ef804d 20.75 KiB 383.40 KiB 362.65 KiB
d446105 20.75 KiB 383.37 KiB 362.62 KiB

Previous results on branch: armcknight/ci/disable-trending-movies-caching

Startup times

Revision Plain With Sentry Diff
1048f84 1232.39 ms 1252.12 ms 19.73 ms
8a39b3b 1212.61 ms 1235.56 ms 22.95 ms

App size

Revision Plain With Sentry Diff
1048f84 20.75 KiB 383.89 KiB 363.14 KiB
8a39b3b 20.75 KiB 383.77 KiB 363.01 KiB

@armcknight armcknight changed the title ci: disable trending movies caching ci: fix caching in some CI jobs Dec 1, 2022
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 🙏

@armcknight armcknight force-pushed the armcknight/ci/disable-trending-movies-caching branch from 70edf68 to 961dd8a Compare December 2, 2022 22:52
@armcknight armcknight merged commit b9c9598 into 8.0.0 Dec 2, 2022
@armcknight armcknight deleted the armcknight/ci/disable-trending-movies-caching branch December 2, 2022 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants