-
Notifications
You must be signed in to change notification settings - Fork 85
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
[eas-cli] create fingerprint on each update #2687
[eas-cli] create fingerprint on each update #2687
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
63d5f34
to
6d5494c
Compare
Size Change: +711 B (0%) Total Size: 52.9 MB
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2687 +/- ##
==========================================
+ Coverage 52.77% 52.79% +0.03%
==========================================
Files 579 580 +1
Lines 22194 22260 +66
Branches 4343 4352 +9
==========================================
+ Hits 11710 11750 +40
- Misses 10448 10474 +26
Partials 36 36 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Subscribed to pull request
Generated by CodeMention |
); | ||
} | ||
|
||
export async function maybeCalculateFingerprintForRuntimeVersionInfoObjectsWithoutExpoUpdatesAsync({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the logic of this function is kind of complicated. would be good to split as smaller functions and add some unit test.
but fine if that requires too much effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we also need to add this to republish. Probably just pull in the fingerprint from the previously-published update assuming we are sure it won't change between publish and republish (don't think we have another option since we can't re-calculate the fingerprint on republish since republish can't use any info from the current project state, code signing suffers from this fwiw)
(if you want to do republish in a follow-up, request re-review on this one. this looks good otherwise) |
Also, should roll-back-to-embedded updates have this as well? (every command that calls |
@wschurman here is the PR for republishing (made separate because of extra dependencies on www changes): #2708 I dont think we need to pass on fingerprint info for rollback to embedded. A 'rollback to embedded' directive would always be compatible with whatever build that is targeted. Also, it would technically have the fingerprint of the receiving build (ideally all builds of the same runtime have the same fingerprint, but that's not necessarily the case) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think we need to pass on fingerprint info for rollback to embedded.
My guess is that it might be simpler product-wise to include them since they are just normal UpdateEntities (as are rollouts and republishes). Otherwise we'd need to special case the UI to explain why they don't have them for those updates?
A 'rollback to embedded' directive would always be compatible with whatever build that is targeted. Also, it would technically have the fingerprint of the receiving build (ideally all builds of the same runtime have the same fingerprint, but that's not necessarily the case)
I believe they're only compatible with builds with the same code signing key (and therefore theoretically runtime version as we advise people to change when they do a key change). I think since the code signing configuration is part of the fingerprint we should probably attach the fingerprint to directives.
1149b96
to
2527a31
Compare
2527a31
to
9eb89a7
Compare
✅ Thank you for adding the changelog entry! |
Why
This PR computes a fingerprint for each update.
expo
package for projects SDK 52+.expo-updates
.How
fingerprintHash
instead of the full fingerprint objectTest Plan