-
Notifications
You must be signed in to change notification settings - Fork 77
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
feature: add fingerprint integration #231
Conversation
5d1a6b6
to
03e53dd
Compare
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.
It looks good so far, but have some reservations about using SQLite through github caches 😄 We'll see how that goes in testing.
Can you add two readmes next to the action.yml
that describes these actions? E.g. copy https://github.com/expo/expo-github-action/blob/main/preview/README.md and add a warning, like this, that mentions it's still experimental?
sure thing! plan to add READMEs in a separated pr. will open the pr later. |
f933408
to
189ba6c
Compare
48c654a
to
700c651
Compare
following up with #231 (review) to add READMEs --------- Co-authored-by: Cedric van Putten <[email protected]>
## [8.1.0](8.0.0...8.1.0) (2023-11-24) ### New features * add fingerprint integration ([#231](#231)) ([e6622e0](e6622e0)) * add readme for preview-build and fingerprint action ([#232](#232)) ([acc171b](acc171b)), closes [/github.com//pull/231#pullrequestreview-1628540314](https://github.com/expo//github.com/expo/expo-github-action/pull/231/issues/pullrequestreview-1628540314) * **preview:** add update manifest permalinks to the output ([#245](#245)) ([331a5ab](331a5ab)) ### Bug fixes * **preview:** allow multiple `app.json` schemes ([#244](#244)) ([42d64cf](42d64cf)) ### Code changes * update error handling to node 20 standards ([#243](#243)) ([65ee055](65ee055)) * update workflows and repository to use Bun ([#241](#241)) ([7f1c170](7f1c170)) * upgrade all actions to run on node 20 ([#242](#242)) ([b3e5355](b3e5355)) ### Other chores * **ci:** update release flow with new service account ([#238](#238)) ([e289d93](e289d93)) * update release workflow actions ([#239](#239)) ([57dcabe](57dcabe)) ### Documentation changes * add link to breaking change pull request ([9e1a8c8](9e1a8c8)) * fix changelog link in readme ([185932d](185932d)) * update example to use `eas-version` ([#222](#222)) ([7bc946c](7bc946c))
🎉 This PR is included in version 8.1.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Why
to dogfood
@expo/fingerprint
, this pr tries to the fingerprint integration.close ENG-8599
for updating doc, i will do it in a separated pr.
How
this pr introduces two new actions, mainly designed for pull requests.
fingerprint
the action will check the fingerprint from the pr head commit against base commit and just output the fingerprint diff. this could be used for something like pr labeler:
fingerprint not changed
fingerprint changed
preview-build
the action will check the fingerprint from the pr head commit against base commit and create eas builds if necessary. example workflow
fingerprint not changed
fingerprint not changed and found compatible EAS Build
fingerprint changed
Implementations
check the high-level design first: https://www.notion.so/expo/expo-github-action-preview-build-92a444c9e4b84726a85b0bf4ef619f6c
to check whether the pr has fingerprint changes, we keep track
gitCommitSha <-> fingerprint <-> easBuildId
in a sqlite database and keep it in the github actions cache. the remaining part are just github integration like commenting or eas build command executions.some challenges i had in this pr:
ncc
i want to import the @expo/fingerprint from the version user specified in yaml, not the version prebundled when we use ncc to build. this is more like a dynamic require for ncc. other than that, sqlite3 has some prebuilt native node-pre-gyp
binaries. if i trigger ncc build on macos, the darwin binary cannot run on github linux or windows runners. to do this, mainly have two points here:
process.env['NODE_PATH']
to let nodejs further search the github tool directory when usingrequire('sqlite3')
.github actions cache mutation
github actions cache is designed to be immutable. as long as the cache is changed, you should have a new cache key. i don't want to pollute user's cache and have many
fingerprint-db-v1
,fingerprint-db-v2
,fingerprint-db-v3
and so on. i just have to use the github api to remove the cache key before saving new database. the downside is that we needactions: write
permission when integrating the action.github actions cache matching scope
from the design of github actions cache, when saving cache in a branch, they will actually save the cache key in its branch's namespace. when updating database in a pr branch, saving the fingerprint-db cache will not update the fingerprint-db cache on main branch. as the result, we have to add
on.push.branches: [main]
from the workflow and update fingerprint database when prs merged back and push to main branch. that make the main branch's database updated when new prs coming.to further update
easBuildId
from pr branches in the database, when a pr merged back to main branch. we should use github api to find the associated pr from the merge commit, find the pr comment we added, and find theeasBuildId
in the hidden metadata in the comment.Risks
what if concurrent github actions workflows pushed back to main branch, the database may be overwritten in unexpected state. in this case, currently i have to use the github actions concurrency to limit the concurrency group when pushing on main:
Test Plan