[No QA] chore: bump rock to 0.11.5#72568
Conversation
|
|
|
I struggle to understand if the failing GH actions validation is something to be fixed by me? Tried syncing with |
|
Typechecks are currently failing in main. Will review this shortly |
|
🚨 Before merging, let's trigger both workflows (android+ios) for remote builds from this branch. |
|
🚧 @chuckdries has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
@chuckdries for these workflow changes you need to create internal copy of the branch so it has access to the secrets, you cannot run the workflow change from fork Running it here #72601 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
Sorry I was wrong, this is just package update so no need for internal branch |
|
I think we shoul dbe good to merge once the actions pass |
|
@adhorodyski Can you run |
|
I'm fine merging this - everything works as it did before - but I'm seeing it does not solve the issue we talked about in slack.
rock.update.is.calling.local.env.when.it.shouldn.t.mp4 |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
|
Let's wait with merging this until we resolve remote builds on |
|
@chuckdries ok I think the problem is within the fingerprint itself for Hybrid. I know it's not perfect (and the hash should reflect a state of the file system) so with the changed root (compared to ND only) it's missing the root .env. Let me try to fix this one. btw no need for running edit: as far as I see it's being used here so the variable becomes a part of the build process. I'm looking into this fingerprinting now. edit2: forget edit1 😅 I'm on the right solution now! fingerprint: {
env: ['USE_NGROK'],
}is the missing part (to the upgrade itself which is crucial because it enabled it) and this is what we should bring in for every env var we really care about. I'll make sure it doesn't break thing with USE_NGROK and it's safe to merge with my next commit. For other ones, we can do followups only after they become problematic. |
|
We need to figure out what we care about from env to resolve this ngrok issue, using Here are the actions we can take:
|
|
@chuckdries if you can please confirm this sample change fixes it for you, I'd propose merging this PR and making a followup with the env setup - we'll be able to avoid conflicts coming up and separating between the upgrade itself and a config fix. |
|
Adding this to fingerprint: {
env: ['USE_NGROK', 'NGROK_URL', 'SECURE_NGROK_URL'],
},I think we should apply this change so that users' intended configuration actually works. I think passing |
|
✋ 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/chuckdries in version: 9.2.32-0 🚀
|
|
Is it common for people to leave some values in their local |
|
You need at least |
|
Yeah this is my concern. By adding these to the config, if people have them exposed locally to the process via You'd have to remember to comment them out completely (so they're different than the default values), I'm worried it'll result in lots of silent cache misses. It makes sense to have a different hash with different env values for these options, but might be bad DX. I'm working on some improvements to making this |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.32-6 🚀
|
@chuckdries @mountiny
Explanation of Change
Following up on this Slack conversation, Rock in 0.9.0 (our current version) does not handle environmental variables correctly. This pull request bumps the version of Rock in the project to the latest release (11.5) to ensure we account for the content of
.envin a secure manner. This was fixed in 11.4 here.At the same time, this makes for a regular chore to stay up to date with latest fixes and performance improvements to the framework.
The new fingerprinting mechanism is the most impactful change in this PR.
Releases page: https://github.com/callstackincubator/rock/releases
Fixed Issues
$
PROPOSAL: https://expensify.slack.com/archives/C08CZDJFJ77/p1759444673926729
Tests
Offline tests
QA Steps
Same as tests.
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