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

feature: add fingerprint integration #231

Merged
merged 8 commits into from
Sep 20, 2023
Merged

feature: add fingerprint integration #231

merged 8 commits into from
Sep 20, 2023

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Sep 14, 2023

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
    Screenshot 2023-09-14 at 7 19 25 PM

  • fingerprint changed
    Screenshot 2023-09-14 at 7 19 31 PM

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
    Screenshot 2023-09-14 at 7 24 42 PM

  • fingerprint not changed and found compatible EAS Build
    Screenshot 2023-09-19 at 2 02 00 AM

  • fingerprint changed
    Screenshot 2023-09-14 at 7 24 32 PM

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:

  • making external require from ncc
  • we don't want to install sqlite3 or @expo/fingerprint into user's project root. rather than that we install to github tool directory. we should further mutate process.env['NODE_PATH'] to let nodejs further search the github tool directory when using require('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 need actions: 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 the easBuildId 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:

concurrency: fingerprint-${{ github.event_name != 'pull_request' && 'main' || github.run_id }}

Test Plan

@Kudo Kudo marked this pull request as ready for review September 14, 2023 13:41
Copy link
Member

@byCedric byCedric left a 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?

ncc.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Kudo
Copy link
Contributor Author

Kudo commented Sep 15, 2023

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.

@Kudo
Copy link
Contributor Author

Kudo commented Sep 18, 2023

further added a feature to show a link to latest compatible eas build
Screenshot 2023-09-19 at 2 02 00 AM

@Kudo Kudo merged commit e6622e0 into main Sep 20, 2023
12 checks passed
@Kudo Kudo deleted the kudo/preview-build branch September 20, 2023 16:40
Kudo added a commit that referenced this pull request Sep 20, 2023
following up with #231 (review) to add READMEs

---------

Co-authored-by: Cedric van Putten <me@bycedric.com>
expo-bot pushed a commit that referenced this pull request Nov 24, 2023
## [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))
@expo-bot
Copy link

🎉 This PR is included in version 8.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants