Skip to content
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

Merged
merged 9 commits into from
Jun 17, 2021

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented May 27, 2021

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)

  1. Changes GitUtils.getPullRequestsMergedBetween to be synchronous instead of async. Before it was unnecessarily async, and an arbitrary source of extra complexity.
  2. Removes createNewStagingDeployCash and updateStagingDeployCash from the GithubUtils class. This is now handled entirely in the createOrUpdateStagingDeploy GitHub Action (located at .github/actions/createOrUpdateStagingDeploy/createOrUpdateStagingDeploy.js). Accordingly, I stripped some tests from GitHubUtilsTest and created a new automated test for the createOrUpdateStagingDeploy GH Action.
  3. Removes the NEW_PULL_REQUESTS and NEW_DEPLOY_BLOCKERS inputs from the createOrUpdateStagingDeploy 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 the DeployBlockerCash label.
  4. Updates the YML workflows to remove those unused inputs from usages of the createOrUpdateStagingDeploy action.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/164517

Tests

  1. Merge this pull request.
  2. Verify that a staging deploy happens and that the StagingDeployCash is updated with this pull request and the new BUILD version.
  3. Check off this pull request on the StagingDeployCash
  4. Lock the StagingDeployCash, then immediately after merge a pull request.
  5. Verify that the StagingDeployCash is updated to include that pull request.
  6. Merge another pull request.
  7. Verify that the the pull request gets the "deploy deferred" comment.
  8. Verify that the pull request is not deployed to staging and is not added to the StagingDeployCash
  9. Create a new issue labelled DeployBlockerCash
  10. Verify that the StagingDeployCash is updated to include the new deploy blocker
  11. Label this pull request with DeployBlockerCash
  12. Verify that the StagingDeployCash is updated to list this pull request as a deploy blocker.
  13. Unlock and relock the StagingDeployCash.
  14. Verify that the StagingDeployCash is update to include the deferred pull request.
  15. Verify that the items on the StagingDeployCash that were checked off remain checked off.
  16. Merge another pull request. Verify that it gets the "deploy deferred" comment and is not deployed to staging / added to the StagingDeployCash.
  17. Close the StagingDeployCash to trigger a production deploy. Verify that the prod deploy begins.
  18. Verify that a new StagingDeployCash is created, including the second deferred PR.
  19. Once the prod deploy completes, verify that every PR on the previous StagingDeployCash gets the "deployed to prod" comment.

@roryabraham roryabraham self-assigned this May 27, 2021
@roryabraham roryabraham changed the title Use full Git history to populate StagingDeployCash [No QA] Use full Git history to populate StagingDeployCash May 27, 2021
@roryabraham roryabraham changed the title [No QA] Use full Git history to populate StagingDeployCash [HOLD] [No QA] Use full Git history to populate StagingDeployCash May 27, 2021
@roryabraham
Copy link
Contributor Author

HOLD for live testing

@roryabraham roryabraham marked this pull request as ready for review May 27, 2021 21:25
@roryabraham roryabraham requested a review from a team as a code owner May 27, 2021 21:25
@roryabraham roryabraham requested a review from AndrewGable May 27, 2021 21:26
@MelvinBot MelvinBot requested review from Dal-Papa and removed request for a team May 27, 2021 21:26
@roryabraham
Copy link
Contributor Author

@Dal-Papa All the index.js files here are compiled, so when you review this it's best just to hide them, as they'll be look funky and it will appear to be a lot of duplicate code

Copy link

@Dal-Papa Dal-Papa left a 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.

@Dal-Papa
Copy link

@Dal-Papa All the index.js files here are compiled, so when you review this it's best just to hide them, as they'll be look funky and it will appear to be a lot of duplicate code

Just to make sure, when you say compiled, that means that there are multiple source files that end up being the various index.js files right ? I see a lot of duplicate code but it's because of that, right ?

@roryabraham
Copy link
Contributor Author

Just to make sure, when you say compiled, that means that there are multiple source files that end up being the various index.js files right

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

Dal-Papa
Dal-Papa previously approved these changes May 28, 2021
Copy link

@Dal-Papa Dal-Papa left a 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 ?

@roryabraham
Copy link
Contributor Author

Ah yes, forgot to rebuild the GHA's 🤦

Copy link

@Dal-Papa Dal-Papa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roryabraham
Copy link
Contributor Author

Okay, making a note here that during our testing of CP's over here we discovered another race condition:

  1. The StagingDeployCash is locked, and starts making a new PATCH version./
  2. Another PR is merged to main. Since the StagingDeployCash is locked, it does not create a BUILD version or start a staging deploy. It gets the "deploy deferred" comment.
  3. The PATCH version finishes and is merged to main.
  4. staging is updated from main 💥 – this includes the last-minute PR that was merged while the StagingDeployCash was locked.
  5. So the last-minute PR is deployed, but isn't included in the StagingDeployCash for QA.

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 StagingDeployCash, and not skipped for QA 👍

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:

image

But that's not a severe enough problem to worry about for such a rare edge-case.

@roryabraham
Copy link
Contributor Author

Updated the testing steps of this PR to verify that it fixes this edge case 👍

@roryabraham roryabraham changed the title [HOLD] [No QA] Use full Git history to populate StagingDeployCash [No QA] Use full Git history to populate StagingDeployCash Jun 2, 2021
@Dal-Papa
Copy link

@AndrewGable : Do you still want to review this or should we get it merged ?

@AndrewGable
Copy link
Contributor

Looks good, @roryabraham feel free to self merge when ready to test.

@roryabraham
Copy link
Contributor Author

Thanks, I have to wait to test this until after the next production deploy.

@roryabraham
Copy link
Contributor Author

Going to test this now

@roryabraham roryabraham merged commit 3fcf04f into main Jun 17, 2021
@roryabraham roryabraham deleted the Rory-FullDeployChecklist branch June 17, 2021 16:51
@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.70-1🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@roryabraham
Copy link
Contributor Author

This did not work. First follow-up here just adds better logs to help me diagnose the failure

@roryabraham
Copy link
Contributor Author

Think I found the problem and have a solution here

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.73-3🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants