-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[No QA] Use full Git history to populate StagingDeployCash #3173
Conversation
HOLD for live testing |
@Dal-Papa All the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asking this question now even though I'm not done with the review to avoid losing too much time.
.github/actions/createOrUpdateStagingDeploy/createOrUpdateStagingDeploy.js
Outdated
Show resolved
Hide resolved
Just to make sure, when you say compiled, that means that there are multiple source files that end up being the various |
Yeah, that's correct. A more accurate word than "compiled" would be "bundled with dependencies". Basically any dependencies are bundled into a single Node.js script that has everything it needs to run. There's some more information here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good but there's a problem with that validate action ?
Ah yes, forgot to rebuild the GHA's 🤦 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
Okay, making a note here that during our testing of CP's over here we discovered another race condition:
Fortunately, the output of this command: git log --format="%s" 1.0.56-4...1.0.57-0 Includes that last-minute PR. So after the changes from this PR, it would be included in the The last-minute PR should also get the "deployed to staging" comment, so that's good. The only thing that's sort of broken here is that this comment is a bit misleading: But that's not a severe enough problem to worry about for such a rare edge-case. |
Updated the testing steps of this PR to verify that it fixes this edge case 👍 |
@AndrewGable : Do you still want to review this or should we get it merged ? |
Looks good, @roryabraham feel free to self merge when ready to test. |
Thanks, I have to wait to test this until after the next production deploy. |
Going to test this now |
🚀 Deployed to staging in version: 1.0.70-1🚀
|
This did not work. First follow-up here just adds better logs to help me diagnose the failure |
Think I found the problem and have a solution here |
🚀 Deployed to production in version: 1.0.73-3🚀
|
Details
The main motivation behind this pull request is to use the full Git / GitHub history to populate the StagingDeployCash, rather than relying on one-off changes made in actions. This makes the whole process more robust: any time the StagingDeployCash is updated, it should have all the correct pull requests and deploy blockers. That means if there is a GH outage, then when the service is restored the StagingDeployCash should self-resolve to its correct state.
Here's a more detailed list of the changes made in this PR (in broad strokes)
GitUtils.getPullRequestsMergedBetween
to be synchronous instead of async. Before it was unnecessarily async, and an arbitrary source of extra complexity.createNewStagingDeployCash
andupdateStagingDeployCash
from theGithubUtils
class. This is now handled entirely in thecreateOrUpdateStagingDeploy
GitHub Action (located at.github/actions/createOrUpdateStagingDeploy/createOrUpdateStagingDeploy.js
). Accordingly, I stripped some tests fromGitHubUtilsTest
and created a new automated test for thecreateOrUpdateStagingDeploy
GH Action.NEW_PULL_REQUESTS
andNEW_DEPLOY_BLOCKERS
inputs from thecreateOrUpdateStagingDeploy
action. Instead, the action finds pull requests by looking at the tag on the previous StagingDeployCash, and parsing the git history between that tag and the latest tag to determine which PRs were merged since the last prod deploy. It finds deploy blockers by just querying the GH api for any open issues with theDeployBlockerCash
label.createOrUpdateStagingDeploy
action.Fixed Issues
Fixes https://github.com/Expensify/Expensify/issues/164517
Tests
StagingDeployCash
is updated with this pull request and the newBUILD
version.StagingDeployCash
StagingDeployCash
, then immediately after merge a pull request.StagingDeployCash
is updated to include that pull request.StagingDeployCash
DeployBlockerCash
StagingDeployCash
is updated to include the new deploy blockerDeployBlockerCash
StagingDeployCash
is updated to list this pull request as a deploy blocker.StagingDeployCash
.StagingDeployCash
is update to include the deferred pull request.StagingDeployCash
that were checked off remain checked off.StagingDeployCash
.StagingDeployCash
to trigger a production deploy. Verify that the prod deploy begins.StagingDeployCash
is created, including the second deferred PR.StagingDeployCash
gets the "deployed to prod" comment.