[No QA] Ensure bundle version strings are compatible before auto-merging CP PR #6199
Conversation
.github/actions/checkBundleVersionStringMatch/checkBundleVersionStringMatch.js
Outdated
Show resolved
Hide resolved
Jag96
changed the title
| const bundleVersion = execSync(`/usr/libexec/PlistBuddy -c "Print :CFBundleVersion" ${PLIST_PATH}`); | ||
| const shortBundleVersion = execSync(`/usr/libexec/PlistBuddy -c "Print :CFBundleShortVersionString" ${PLIST_PATH}`); | ||
|
|
||
| if (shortBundleVersion !== (bundleVersion.split('-') || [''])[0]) { |
There was a problem hiding this comment.
NAB: This comment is more for me to learn. I see in nativeVersionUpdater.js that the shortBundleVersion is obtained as following:
App/.github/libs/nativeVersionUpdater.js
Line 79 in fa2e8a9
That means in terms of your example from the PR body: 1-1-1-0 this would be 1 and then on the next line we add another zero to the version:
App/.github/libs/nativeVersionUpdater.js
Line 80 in fa2e8a9
What I do not understand is taking the first - [0] element of the split would not check for the case you have described in the PR body:
- Compatible bundle version strings:
1.1.1and1.1.1.0 - Incompatible bundle version strings:
1.1.1and1.1.0.0
Thank you for explanation of this.
There was a problem hiding this comment.
Ah, no worries! So in this case we're grabbing the version number from the info.plist, which has the versions formatted as x.x.x-x. So if the bundleVersion is 1.1.1-0 and the shortBundleVersion is 1.1.1, then they would match. Let me know if that makes sense!
There was a problem hiding this comment.
Ahaa! That makes a lot of sense, did not catch that, thank you!
|
Since we're testing this with a live CP, added some logging in to make it easier to debug in case things don't work properly |
There was a problem hiding this comment.
The logs will help for sure, great idea! Approving and all yours @roryabraham.
|
@Jag96 I'll let you merge when you're ready to test with a CP to fix one of our several deploy blockers. |
|
Going to merge this and use #6341 to test there is no CP regression |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Testing this here |
|
From https://expensify.slack.com/archives/C03V9A4TB/p1637167714322500, PListBuddy only runs on macos, but repo-sync/pull-request needs to run on linux. Looks like we'll need to figure out a different way to support both, so going to revert this PR because CPs are currently broken and I'll look into how to make this work. |
|
🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀
|
Details
This PR updates our CP workflow to ensure that auto-merge PRs won't merge if the bundleVersion string and shortBundleVersion strings aren't compatible.
Compatible bundle version strings:
1.1.1and1.1.1.0Incompatible bundle version strings:
1.1.1and1.1.0.0Fixed Issues
$ https://github.com/Expensify/Expensify/issues/178225
Tests
To test this we'll merge this PR and then CP a dummy change to ensure that there are no CP flow regressions.
QA Steps
None
Tested On
N/A