Set up remote ad hoc builds#70396
Conversation
| - name: Convert keystore to base64 | ||
| id: keystore-base64 | ||
| run: | | ||
| echo "keystore_content=$(base64 -w 0 Mobile-Expensify/Android/upload-key.keystore)" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Let's not do this. I'm not exactly sure if this will cause security issues, but it at minimum doesn't feel right.
We can do one of two things:
- Update
rockso that it doesn't need this parameter, can it just do the base64 inside? - We can store the base64 version in 1Password and you can access it like all our other secrets.
There was a problem hiding this comment.
I agree with Andrew - we normally keep this file in 1Password, and before that we would GPG-encrypt it
There was a problem hiding this comment.
Yes it makes sense, I will ask the team if is possible to change it. If not, would it be a big effort to store it in the 1Password?
There was a problem hiding this comment.
Currently, using Base64 encoding for the release keystore is necessary in our setup.
The common pattern is to store the Base64 string in GitHub Secrets and decode it during the workflow.
So in our case we could use the 1Password for it.
We can apply the same pattern for both Android and iOS:
| Platform | Encode Command | GitHub / 1Password Secret Name |
|---|---|---|
| Android | base64 -i release.keystore | pbcopy |
ROCK_ANDROID_KEYSTORE_BASE64 |
| iOS Certificate | base64 -i BUILD_CERTIFICATE.p12 | pbcopy |
ROCK_APPLE_CERTIFICATE_BASE64 |
| iOS Profile | base64 -i OldApp_AdHoc.mobileprovision | pbcopy |
ROCK_APPLE_PROFILE_BASE64 |
What are your thoughts? Do you know who can help with setting it up?
There was a problem hiding this comment.
Use 1password/load-secrets-action to grab these all:
op://${{ vars.OP_VAULT }}/upload-key.keystore/base64op://${{ vars.OP_VAULT }}/New Expensify Distribution Certificate/base64op://${{ vars.OP_VAULT }}/OldApp_AdHoc/OldApp_AdHoc.mobileprovisionop://${{ vars.OP_VAULT }}/OldApp_AdHoc_Share_Extension/OldApp_AdHoc_Share_Extension.mobileprovisionop://${{ vars.OP_VAULT }}/OldApp_AdHoc_Notification_Service/OldApp_AdHoc_Notification_Service.mobileprovision
There was a problem hiding this comment.
Thanks you for that, I’ve updated the PR.
For Android, it looks like everything is covered ✅
For iOS, we’ll also need the Base64 version of the provisioning profile (similar to how we have the certificate -
op://${{ vars.OP_VAULT }}/New Expensify Distribution Certificate/base64)
Could you provide that as well?
Additionally, we also need the certificate-password and keychain-password. We’re currently pulling them from secrets like this:
certificate-password: ${{ secrets.CERTIFICATE_PASSWORD || '' }}
keychain-password: ${{ secrets.KEYCHAIN_PASSWORD || '' }}
Could you verify that these are properly set up?
There was a problem hiding this comment.
Provisioning profiles are the last three bullet points. You can leave the certificate password and keychain passwords blank. They are not set.
There was a problem hiding this comment.
@AndrewGable can you help with adding the base64 for the provisioning profile, share profile and for notification profile?
There was a problem hiding this comment.
I've provided them here. Please DM me on Slack if you need more help.
AndrewGable
left a comment
There was a problem hiding this comment.
Small style suggestions
| # rock v3 | ||
| uses: callstackincubator/android@1a7d52dfe3ca195ccbe5ad2f06c15f2fc3835115 |
There was a problem hiding this comment.
| # rock v3 | |
| uses: callstackincubator/android@1a7d52dfe3ca195ccbe5ad2f06c15f2fc3835115 | |
| uses: callstackincubator/android@1a7d52dfe3ca195ccbe5ad2f06c15f2fc3835115 # v3.0.0 |
| # v2 | ||
| uses: 1password/load-secrets-action@581a835fb51b8e7ec56b71cf2ffddd7e68bb25e0 |
There was a problem hiding this comment.
| # v2 | |
| uses: 1password/load-secrets-action@581a835fb51b8e7ec56b71cf2ffddd7e68bb25e0 | |
| uses: 1password/load-secrets-action@581a835fb51b8e7ec56b71cf2ffddd7e68bb25e0 # v2 |
| # rock v3 | ||
| uses: callstackincubator/ios@08a533dbeda6adec39f94d08d820091514d1f7af |
There was a problem hiding this comment.
| # rock v3 | |
| uses: callstackincubator/ios@08a533dbeda6adec39f94d08d820091514d1f7af | |
| uses: callstackincubator/ios@08a533dbeda6adec39f94d08d820091514d1f7af # v3.0.0 |
| keystore-key-alias: ${{ steps.load-credentials.outputs.ANDROID_UPLOAD_KEYSTORE_ALIAS }} | ||
| keystore-key-password: ${{ steps.load-credentials.outputs.ANDROID_UPLOAD_KEY_PASSWORD }} | ||
| comment-bot: true | ||
| rock-build-extra-params: '--extra-params -PreactNativeArchitectures=arm64-v8a,x86_64' |
There was a problem hiding this comment.
Can we make sure these are still required? As far as I remember the default value is now the same.
There was a problem hiding this comment.
In the project’s Gradle setup, we’ve got all four architectures selected.
If we don’t want to build for the legacy one, I think we’ll still need to keep it. Do you know how we can verify that further?
There was a problem hiding this comment.
No I think you're right, thanks for looking into it! Let's leave it as is.
| keystore-store-password: ${{ steps.load-credentials.outputs.ANDROID_UPLOAD_KEYSTORE_PASSWORD }} | ||
| keystore-key-alias: ${{ steps.load-credentials.outputs.ANDROID_UPLOAD_KEYSTORE_ALIAS }} | ||
| keystore-key-password: ${{ steps.load-credentials.outputs.ANDROID_UPLOAD_KEY_PASSWORD }} | ||
| comment-bot: true |
There was a problem hiding this comment.
Do we want the comment bot to post as-is? I'd expect we can integrate with the existing system. Maybe producing build metadata (url) artifacts from this step would play better?
There was a problem hiding this comment.
we are saving ROCK_IOS_PATH and ROCK_APK_PATH as output in our build step and we print it postGithubComment. So you are right we don't need the comment-bot. I will disable it
| "file": "./OldApp_AdHoc_Notification_Service.mobileprovision" | ||
| } | ||
| ] | ||
| comment-bot: true |
There was a problem hiding this comment.
same comment about the bot posting
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@rinej will leave his 🟢 light for merging after some cleanup (some actions can be a followup, we can trim down fastlane stuff) |
|
Let's sync up with main before merging to test again with the latest Rock version. @rinej |
|
Merged the main, we can do the final test and final review. |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
The last build was successful, we can proceed and merge the changes ✅ |
mountiny
left a comment
There was a problem hiding this comment.
Builds work and changes look good to me, lets move it ahead
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.33-0 🚀
|
|
@mountiny @AndrewGable @roryabraham Can you pls validate this PR internally as it needs a new build trigger |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.33-4 🚀
|
|
@mountiny @AndrewGable Seems it was tested last deploy, no need testing it now with |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.35-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.2.35-4 🚀
|
…et-up-remote-ad-hoc-builds" This reverts commit 0c2ed1a.
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.89-1 🚀
|
|
@mountiny @AndrewGable @roryabraham Can you pls validate this PR internally as it needs a new build trigger |
|
@m-natarajan We can check this off, this was deployed a while ago, some gremlins |
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.2.89-6 🚀
|
Explanation of Change
The primary goal of this change is to speed up the process of creating ad-hoc builds on request. We will use the Rock framework, following the same approach already in place for dev remote builds.
Proposed Plan:
After each merge to the branch, we will trigger an ad-hoc-remote-build, similar to the dev workflow. However, this job will generate a signed build. So we need to provide all necessary credentials.Notes:
For now, we will maintain separate workflows: ad-hoc-remote-build-android and ad-hoc-remote-build-ios. This makes testing and isolation easier.Once testing is complete, we will integrate them into the shared workflows (remote-build-android.yml and remote-build-ios.yml) using a matrix strategy.Fixed Issues
$ #62296
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop