-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Race condition can cause big checklists #27123
Comments
No update right now |
No update |
Looking into this one now Noting that the createNewDeployChecklist was moved to the platformDeploy.yml, which feels like the race condition is not happening anymore and the bug might be somewhere else. When the Looking more into why this is failing |
Created a PR which should hopefully help with this |
It looks like the last checklist that was created without being affected by this error is this one, so we should try to figure out what was different between |
This issue has not been updated in over 15 days. @roryabraham eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
FYI I wrote a handy little JS script to mark off items in the new checklist that were on the old one: #30560 (comment) - just a temporary workaround |
Moving this back to weekly since it's a PITA |
Planning to investigate this further this week |
I have a hunch that this has something to do with force-pushes. |
Spent several hours trying to dig into this, but so far no success in finding a minimal reproduction. I tried something like this:
That worked as expected, so I think the next variation should be something like this:
My thinking is that by rebasing onto a commit from the previous release then force-pushing we'll somehow include the commits from PR B in the new checklist |
The other thing maybe worth looking into is that our tests always checkout 1 commit on main before fetching the other tags used in the Not sure if that matters, but it's a difference between our local testing setup and the production environment (this may change if/when I create a new callable workflow to wrap |
This should be fixed by #31458. I hope so 😅 |
That didn't work perfectly: https://github.com/Expensify/App/actions/runs/6910552568/job/18803863115. Going to retry |
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:
|
📣 @github-actions[bot]! 📣
|
Nice, that worked as expected (well, maybe not the upwork bit, that's weird). Anyways, for context I'm marking this as a deploy blocker not because we actually need to block deploy on it, but because I think we should merge #31458 before the next deploy, because it should fix this big checklist issue |
Problem
Sometimes we get big checklists like this one that include all the PRs from the previous checklist.
The root cause is a race condition between:
Both are triggered at the same time after updateStaging finishes. If deployStaging finishes (i.e: the tag for the new staging version is pushed to the remote) before the checklist is created, then it works. If not, then we get a huge checklist like this will all the PRs that were deployed in the previous version.
Solution
TBD – probably change the deploy system a bit such that staging deploys are triggered by the creation of a prerelease and production deploys by the creation of a "full" release.
That way we can push the tag as soon as it's created without triggering a deploy
The text was updated successfully, but these errors were encountered: