Update getPullRequestsMergedBetween logic #6906

roryabraham · 2021-12-27T21:11:58Z

Details

This (hopefully) fixes long-broken logic where duplicate PRs show up in checklists / deploy comments or are excluded from checklists / deploy comments in error.

See this comment for details.

Fixed Issues

$ #4977

Tests

I wrote an automated test script to run these steps on an ongoing basis, but here are the manual testing steps:

Setup steps

If needed, install jq: brew install jq.
Create directory for dummy repo: mkdir DummyRepo && cd DummyRepo
Initialize npm project so that we can test the node script w/ underscore dependency: npm init (use 0.0.1 as the initial version).
Install underscore: npm install underscore && npm i

Create a tiny node script we'll use for testing: touch ./getPullRequestsMergedBetween.js && chmod u+x ./getPullRequestsMergedBetween.js

#!/usr/bin/env node

# Note: you'll need to use your local path to the Expensify/App repo
const GitUtils = require('../Expensidev/App/.github/libs/GitUtils.js');

GitUtils.getPullRequestsMergedBetween(process.argv[2], process.argv[3]);

Initialize DummyRepo as a git repo with main as the default branch.

git init -b main
git add package.json package-lock.json getPullRequestsMergedBetween.js
git commit -m "Initial commit"

Update the version to 1.0.0 and tag the commit. This is only necessary so that we have a "previous tag".

npm --no-git-tag-version version 1.0.0 -m "Update version to 1.0.0"
git add package.json
git commit -m "Update version to 1.0.0"

Create staging and production branches off of main:

git checkout -b staging
git checkout -b production
git checkout main

Tag the version on staging:

git checkout staging
git tag $(cat ./package.json | jq -r .version)
git checkout main

Scenario 1: Merge a pull request when the checklist is unlocked

Create "PR 1", and merge that PR to main.

git checkout main
git checkout -b pr-1
echo "Changes from PR #1" >> CHANGELOG.txt 
git add CHANGELOG.txt
git commit -m "Create changelog"
git checkout main
git merge pr-1 --no-ff -m "Merge pull request #1 from Expensify/pr-1"
git branch -d pr-1

Bump the version to 1.0.1:

git checkout main
git checkout -b version-bump
npm --no-git-tag-version version 1.0.1 -m "Update version to 1.0.1"
git add package.json package-lock.json 
git commit -m "Update version to $(cat package.json | jq -r .version)"
git checkout main
git merge version-bump --no-ff -m "Merge pull request #2 from Expensify/version-bump"
git branch -d version-bump

Merge main into staging:

git checkout staging
git checkout -b update-staging-from-main
git merge -Xtheirs main
git checkout staging
git merge update-staging-from-main --no-ff -m "Merge pull request #3 from Expensify/update-staging-from-main"
git branch -d update-staging-from-main

Tag staging:

git checkout staging
git tag $(cat ./package.json | jq -r .version)
git checkout main

From staging, verify that node getPullRequestsMergedBetween.js 1.0.0 1.0.1 outputs:
```
List of pull requests merged between 1.0.0 and 1.0.1 [ '1', ]
```

Scenario 2: Lock the deploy checklist

Bump the version to 1.1.0 on main

git checkout main
git checkout -b version-bump
npm --no-git-tag-version version 1.1.0 -m "Update version to 1.1.0"
git add package.json package-lock.json
git commit -m "Update version to $(cat package.json | jq -r .version)"
git checkout main
git merge version-bump --no-ff -m "Merge pull request #4 from Expensify/version-bump"
git branch -d version-bump

Merge main into staging

git checkout staging
git checkout -b update-staging-from-main
git merge -Xtheirs main
git checkout staging
git merge update-staging-from-main --no-ff -m "Merge pull request #5 from Expensify/update-staging-from-main"
git branch -d update-staging-from-main

Tag the new version on staging

git checkout staging
git tag $(cat package.json | jq -r .version)

From staging, verify that node getPullRequestsMergedBetween 1.0.0 1.1.0 outputs:
```
List of pull requests merged between 1.0.0 and 1.1.0 [ '1' ]
```

Scenario 3: Merge another pull request, but don't CP it

Create "PR 6", and merge that PR to main:

git checkout main
git checkout -b pr-6
echo "Changes from PR #6" >> CHANGELOG.txt 
git add CHANGELOG.txt
git commit -m "Update changelog for PR #6"
git checkout main
git merge pr-6 --no-ff -m "Merge pull request #6 from Expensify/pr-6"
git branch -d pr-6

Scenario 4: Merge another pull request and CP it to staging

Create "PR 7" and merge that PR to main:

git checkout main
git checkout -b pr-7
echo "\nChanges from PR #7\n" >> CHANGELOG.txt 
git add CHANGELOG.txt
git commit -m "Update changelog for PR #7"
git checkout main
git merge pr-7 --no-ff -m "Merge pull request #7 from Expensify/pr-7"
git branch -d pr-7

Update version to 1.1.1 on main:

git checkout main
git checkout -b version-bump
npm --no-git-tag-version version 1.1.1 -m "Update version to 1.1.1"
git add package.json package-lock.json
git commit -m "Update version to $(cat package.json | jq -r .version)"
git checkout main
git merge version-bump --no-ff -m "Merge pull request #8 from Expensify/version-bump"
git branch -d version-bump

Cherry pick PR 7 (and the version-bump) to staging:

First, find the merge commit hash for PR 7 using git log. Save it to a local variable called MERGE_7
Next, find the merge commit hash for PR 8 using git log. Save it to a local variable called MERGE_8

Next, begin using the cherry-pick command to CP our changes:

 git checkout staging
 git checkout -b cherry-pick-staging-7
 git cherry-pick -S -x --mainline 1 --strategy=recursive -Xtheirs $MERGE_8
 git cherry-pick -S -x --mainline 1 $MERGE_7

At this point, there will be conflicts in CHANGELOG.txt. Fix those conflicts to include only the ones to CP (i.e.: "changes from PR 7").

# manually resolve conflicts
 vim CHANGELOG.txt
 git add CHANGELOG.txt
 
 # Then continue the CP
 git cherry-pick --continue
 
 # And merge the pretend CP-PR
 git checkout staging
 git merge cherry-pick-staging-7 --no-ff -m "Merge pull request #9 from Expensify/cherry-pick-staging-7"
 git branch -d cherry-pick-staging-7

Tag the new version on staging:

git checkout staging
git tag $(cat package.json | jq -r .version)

From staging, verify that node getPullRequestsMergedBetween 1.0.0 1.1.1 outputs:
```
List of pull requests merged between 1.0.0 and 1.1.1 [ '9', '7', '1' ]
```
Note: It's fine that PR 9 is included here. It will be filtered out of the deploy checklist at a later step because it's created by OSBotify.

Scenario 5: Close the checklist

Scenario 5A: Run the production deploy

Update production from staging

git checkout production
git checkout -b update-production-from-staging
git merge -Xtheirs staging
git checkout production
git merge update-production-from-staging --no-ff -m "Merge pull request #10 from Expensify/update-production-from-staging"
git branch -d update-production-from-staging

Verify that node getPullRequestsMergedBetween 1.0.0 1.1.1 outputs:

List of pull requests merged between 1.0.0 and 1.1.1 [ '9', '7', '1' ]

Scenario 5B: Run the staging deploy and create a new checklist

Bump version to 1.1.2 on main:

git checkout main
git checkout -b version-bump
npm --no-git-tag-version version 1.1.2 -m "Update version to 1.1.2"
git add package.json package-lock.json
git commit -m "Update version to $(cat package.json | jq -r .version)"
git checkout main
git merge version-bump --no-ff -m "Merge pull request #11 from Expensify/version-bump"
git branch -d version-bump

Update staging from main:

git checkout staging
git checkout -b update-staging-from-main
git merge -Xtheirs main
git checkout staging
git merge update-staging-from-main --no-ff -m "Merge pull request #12 from Expensify/update-staging-from-main"
git branch -d update-staging-from-main

Tag new version on staging:

git checkout staging
git tag $(cat package.json | jq -r .version)

Verify that node getPullRequestsMergedBetween 1.1.1 1.1.2 outputs:

List of pull requests merged between 1.1.1 and 1.1.2 [ '9', '6' ]

Scenario 6: Merge another pull request with the checklist unlocked

Create "PR 13", and merge that PR to main.

git checkout main
git checkout -b pr-13
echo "Changes from PR #13" >> CHANGELOG.txt 
git add CHANGELOG.txt
git commit -m "Update changelog in PR 13"
git checkout main
git merge pr-13 --no-ff -m "Merge pull request #13 from Expensify/pr-13"
git branch -d pr-13

Bump the version to 1.1.3:

git checkout main
git checkout -b version-bump
npm --no-git-tag-version version 1.1.3 -m "Update version to 1.1.3"
git add package.json package-lock.json 
git commit -m "Update version to $(cat package.json | jq -r .version)"
git checkout main
git merge version-bump --no-ff -m "Merge pull request #14 from Expensify/version-bump"
git branch -d version-bump

Merge main into staging:

git checkout staging
git checkout -b update-staging-from-main
git merge -Xtheirs main
git checkout staging
git merge update-staging-from-main --no-ff -m "Merge pull request #15 from Expensify/update-staging-from-main"
git branch -d update-staging-from-main

Tag staging:

git checkout staging
git tag $(cat ./package.json | jq -r .version)

From staging, verify that node getPullRequestsMergedBetween.js 1.1.1 1.1.3 outputs:
```
List of pull requests merged between 1.1.1 and 1.1.3 [ '13', '9', '6' ]
```

In addition to the above, I updated some automated unit tests.

QA Steps

Here are the live-testing steps for this PR... We'll need 5 PRs A-E.

With the deploy checklist unlocked, merge PR A. Then:
- PR A should be deployed to staging
- PR A should be added to the deploy checklist
- PR A should receive a "deployed to staging" comment
With the deploy checklist unlocked, merge PR B. Then:
- PR B should be deployed to staging
- PR B should be added to the deploy checklist
- PR B should receive a "deployed to staging" comment
- PR A should not receive another "deployed to staging" comment.
Lock the checklist.
- PR A should not receive another "deployed to staging" comment
- PR B should not receive another "deployed to staging" comment
Merge PR C without CP Staging label
- Deploy should not happen
- PR C should receive "not yet deployed" comment
Merge PR D with CP Staging label
- PR D should be deployed to staging
- PR D should be added to the deploy checklist
- Applauseleads should be tagged to be informed of the newly-CP'd PR.
- PR D should receive a "CP'd to staging" comment
Merge PR E without CP Staging label, then manually CP it.
- PR E should be deployed to staging
- PR E should be added to the deploy checklist
- Applauseleads should be tagged to be informed of the newly-CP'd PR.
- PR E should receive a "CP'd to staging" comment
Close the checklist
- Production deploy should happen
- PRs A, B, D, E should all receive "deployed to production" comment
- PR C should not receive "deployed to production" comment.
- Staging deploy should also happen
- PR C should be included in the new checklist
- PRs A, B, D, E should all not be included in the new checklist.
- PR C should receive "deployed to staging" comment.
- PRs A, B, D, E should all not receive "deployed to staging" comment.

Tested On

GitHub only

…ging branches

roryabraham · 2021-12-28T16:12:43Z

Question: Is it valuable to aggregate those many long testing steps into an automated script? Should be pretty easy... The only tricky part is the CP conflicts, but those aren't super critical and I can change the steps a bit so there's no conflict.

mountiny

Wohoo 🎉 This is absolutely great work and investigation here. I haven't found any typos or any changes I would suggest. I believed you have tested this thoroughly so going to approve it now.

There is one thing I can think of and that is some more extensive description of this entire process and why we need to do it (parse and filter the commits in this way). Since it took us a long time to figure it out and hopefully, this will work and solve our issues, would you write some SO to cover this for future engineers to understand better?

Thank you for working on this!

ctkochan22

NAB, we repeat some code in all of these instances that could probably be dry'd up, but it was like that before.

MelvinBot · 2022-01-07T18:37:06Z

Triggered auto assignment to @robertjchen (InternalQA), see https://stackoverflow.com/c/expensify/questions/5042 for more details.

botify · 2022-01-07T18:37:08Z

@ctkochan22 looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2022-01-07T18:37:53Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

ctkochan22 · 2022-01-07T18:40:41Z

Double checked with @Jag96 , this sounds like https://github.com/Expensify/Expensify/issues/189984

Removing emegency label

OSBotify · 2022-01-10T19:45:22Z

🚀 Deployed to staging by @ctkochan22 in version: 1.1.26-2 🚀

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

OSBotify · 2022-01-10T20:01:57Z

🚀 Deployed to staging by @ctkochan22 in version: 1.1.27-0 🚀

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

OSBotify · 2022-01-12T05:04:20Z

🚀 Deployed to production by @roryabraham in version: 1.1.27-1 🚀

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

roryabraham · 2022-01-12T12:07:51Z

Okay, I've gathered together some relatively low-risk PRs we can use to QA this:

A: #7072
B: #7137
C: #7154
D: #7149

roryabraham · 2022-01-12T17:56:56Z

Updated the QA steps to account for #7161 – so the new 5 PRs will be:

A: #7161
B: #7072
C: #7137
D: #7154
E: #7149

roryabraham · 2022-01-12T19:21:03Z

Okay, starting QA using these 6 PRs:

A: #7161
B: #7072
C: #7137
D: #7154
E: #7149
F: #7126

With the deploy checklist unlocked, merge PR A. 🟢 Then:
- PR A should be deployed to staging (deployed in 1.1.28-1)
- PR A should be added to the deploy checklist
- PR A should receive a "deployed to staging" comment
With the deploy checklist unlocked, merge PR B. 🟢 Then:
- PR B should be deployed to staging (deployed in 1.1.28-1)
- PR B should be added to the deploy checklist
- PR B should receive a "deployed to staging" comment
- PR A should not receive another "deployed to staging" comment.
Lock the checklist. 🟢 (1.1.29-0)
- PR A should not receive another "deployed to staging" comment
- PR B should not receive another "deployed to staging" comment
Merge PR C without CP Staging label 🟢
- Deploy should not happen (https://github.com/Expensify/App/actions/runs/1689350558) ❌
  - Note: Something weird happened here and this PR was deployed to staging
  - Edit: Figured this out! It's a race condition that can happen if a PR is merged right after the checklist is locked, and it has been around since the beginning. It will be fixed in Bump patch version when a new checklist is created, not when it's locked #7166, but is not related to this PR. We'll follow-up with these same QA steps with PR F.
- PR C should not be added to the deploy checklist. ❌
  - Note: this was added when the lockDeploys.yml workflow finished. It's a race condition that needs to be fixed.
- PR C should receive "not yet deployed" comment
Merge PR D with CP Staging label 🟢 https://github.com/Expensify/App/actions/runs/1689365390
- PR D should be deployed to staging https://github.com/Expensify/App/actions/runs/1689381671
- PR D should be added to the deploy checklist
- ~~Applauseleads should be tagged to be informed of the newly-CP'd PR.~~ ❌
- PR D should receive a "CP'd to staging" comment
Merge PR E without CP Staging label, then manually CP it. 🟢 (https://github.com/Expensify/App/actions/runs/1689400812)
- PR E should be deployed to staging
- PR E should be added to the deploy checklist ❌
- ~~Applauseleads should be tagged to be informed of the newly-CP'd PR.~~ ❌
- PR E should receive a "CP'd to staging" comment
Note: This step is a repeat of step 4, which didn't go as expected for an unrelated reason. ↩️
- Merge PR F (Block Virtual keyboard on Request Money Page #7126) without CP Staging label.
- PR F should not be added to the deploy checklist.
- PR F should receive "not yet deployed" comment
Close the checklist
- Production deploy should happen
- PRs A, B, D, E should all receive "deployed to production" comment
  - Due to the race condition that occurred, PR C should receive "deployed to production" comment.
- ~~PR C should not receive "deployed to production" comment.~~
- PR F should not receive "deployed to production comment.
- Staging deploy should also happen
- ~~PR C should be included in the new checklist~~
  - Due to the race condition that occurred, PR C should not be included in the new checklist.
- PR F should be included in the new checklist
- PRs A, B, D, E should all not be included in the new checklist.
- ~~PR C should receive "deployed to staging" comment.~~
  - Due to the race condition, PR C most likely should not receive a "deployed to staging" comment. We'll treat this as a separate issue (i.e: the race condition) if it does.
- PR F should receive "deployed to staging" comment
- PRs A, B, D, E should all not receive "deployed to staging" comment.

roryabraham · 2022-01-12T20:55:55Z

Oh, and Applauseleads should be tagged to be informed of the newly CP'd PR was aspirational – I wrote the QA steps before completing the pull request and never implemented this requirement. We can handle it in #7178

roryabraham · 2022-01-14T21:29:29Z

Finished QA here 🎉

AndrewGable · 2022-01-14T21:33:02Z

mountiny · 2022-01-15T13:06:58Z

@roryabraham Amazing job on this one, Rory!

Update getPullRequestsMergedBetween logic

95a1967

roryabraham added the InternalQA This pull request required internal QA label Dec 27, 2021

roryabraham self-assigned this Dec 27, 2021

Trim output before wrapping with brackets

3be3312

roryabraham mentioned this pull request Dec 27, 2021

Pull requests CP'd in the previous checklist show up in the next one #4977

Closed

roryabraham added 3 commits December 28, 2021 09:08

Ignore PRs on update-staging-from-main and update-production-from-sta…

18f4319

…ging branches

Get rid of --merges flag because it does not work for CP'd merges

97f1a26

Rebuild GH actions

a1d27a2

roryabraham marked this pull request as ready for review December 28, 2021 16:10

roryabraham requested a review from a team as a code owner December 28, 2021 16:10

MelvinBot requested review from ctkochan22 and removed request for a team December 28, 2021 16:10

roryabraham requested review from AndrewGable and mountiny December 28, 2021 16:10

roryabraham added 15 commits January 5, 2022 10:19

Start setting up automated test

924c87a

Sanitize subjects to remove double quotes

b59d28d

Simplify output to only include pull request numbers

2aa5460

Complete scenario 1

ed15396

Add scenario 2

26a56a1

Save draft state with CP not quite working

1b32190

Fix CP commit hashes

822b5a6

Finish writing tests

0682b01

Add tests to GitHub actions

9fab999

Add git configuration

7ec7a51

Rebuild GH actions

343f860

Make git config global

d254711

Get rid of local abbreviated git commands

7bb8b5b

Add echo for dummy dir

97c9825

Use HOME environment variable to make dummy repo

dc57dd2

Add comment for sanitized output

824d952

mountiny approved these changes Jan 7, 2022

View reviewed changes

Jag96 approved these changes Jan 7, 2022

View reviewed changes

ctkochan22 approved these changes Jan 7, 2022

View reviewed changes

ctkochan22 merged commit 0e5afb7 into main Jan 7, 2022

ctkochan22 deleted the Rory-FixCPDeployComment branch January 7, 2022 18:37

MelvinBot assigned robertjchen Jan 7, 2022

botify added the Emergency label Jan 7, 2022

ctkochan22 removed the Emergency label Jan 7, 2022

OSBotify mentioned this pull request Jan 10, 2022

Deploy Checklist: New Expensify 2022-01-10 #7108

Closed

24 tasks

roryabraham mentioned this pull request Jan 12, 2022

Deploy Checklist: New Expensify 2022-01-12 #7144

Closed

54 tasks

This was referenced Jan 12, 2022

[No QA] Convert ReportActionItemFragment into function component #7072

Merged

[No QA] Don't rely on non-portable realpath #7137

Merged

Add stories for TextInput #7149

Merged

Give Storybook a facelift #7154

Merged

roryabraham mentioned this pull request Jan 12, 2022

[No QA] Add separate assertions for deploy comments, fix bug #7161

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update getPullRequestsMergedBetween logic #6906

Update getPullRequestsMergedBetween logic #6906

roryabraham commented Dec 27, 2021 •

edited

Loading

roryabraham commented Dec 28, 2021

mountiny left a comment

ctkochan22 left a comment

MelvinBot commented Jan 7, 2022

botify commented Jan 7, 2022

OSBotify commented Jan 7, 2022

ctkochan22 commented Jan 7, 2022

OSBotify commented Jan 10, 2022

OSBotify commented Jan 10, 2022

OSBotify commented Jan 12, 2022

roryabraham commented Jan 12, 2022

roryabraham commented Jan 12, 2022

roryabraham commented Jan 12, 2022 •

edited

Loading

roryabraham commented Jan 12, 2022

roryabraham commented Jan 14, 2022

AndrewGable commented Jan 14, 2022

mountiny commented Jan 15, 2022 •

edited

Loading

Update getPullRequestsMergedBetween logic #6906

Update getPullRequestsMergedBetween logic #6906

Conversation

roryabraham commented Dec 27, 2021 • edited Loading

Details

Fixed Issues

Tests

Setup steps

Scenario 1: Merge a pull request when the checklist is unlocked

Scenario 2: Lock the deploy checklist

Scenario 3: Merge another pull request, but don't CP it

Scenario 4: Merge another pull request and CP it to staging

Scenario 5: Close the checklist

Scenario 5A: Run the production deploy

Scenario 5B: Run the staging deploy and create a new checklist

Scenario 6: Merge another pull request with the checklist unlocked

QA Steps

Tested On

roryabraham commented Dec 28, 2021

mountiny left a comment

Choose a reason for hiding this comment

ctkochan22 left a comment

Choose a reason for hiding this comment

MelvinBot commented Jan 7, 2022

botify commented Jan 7, 2022

OSBotify commented Jan 7, 2022

ctkochan22 commented Jan 7, 2022

OSBotify commented Jan 10, 2022

OSBotify commented Jan 10, 2022

OSBotify commented Jan 12, 2022

roryabraham commented Jan 12, 2022

roryabraham commented Jan 12, 2022

roryabraham commented Jan 12, 2022 • edited Loading

roryabraham commented Jan 12, 2022

roryabraham commented Jan 14, 2022

AndrewGable commented Jan 14, 2022

mountiny commented Jan 15, 2022 • edited Loading

roryabraham commented Dec 27, 2021 •

edited

Loading

roryabraham commented Jan 12, 2022 •

edited

Loading

mountiny commented Jan 15, 2022 •

edited

Loading