Do not include CP merge commits in the list of PR merge commits #6165

mountiny · 2021-11-01T23:25:28Z

Details

We are having several issues which are caused by the fact that when PR is CP'ed to staging, this creates a new commit in git history, which starts the same as every other merge commit ie: Merge pull request #4444 .

This duplicate then causes multiple problems, see linked issue for details.

Fixed Issues

$ #4977

Tests

Updated the GitUtilsTest.js to have unit tests

QA Steps

Once deployed to production, we will need to monitor for example staging deploy message to see, if the pull requests, which have been cherry-picked to staging will appear in two deploy checklists in a row or if the PRs will get deploy messaged for two consequent deploys. They should not, the PR should only be considered as part of the deploy into thich it was CP'ed.

Tested On

Unit tests.

mountiny

However, since this is potentially very problematic change if this would fail for whatever reason, I would try to test this on couple of latest releases, which I have not had a time to do so yet, so I am putting this on HOLD for now.

mountiny · 2021-11-01T23:34:37Z

.github/actions/createOrUpdateStagingDeploy/index.js

    return _.map(
-        [...localGitLogs.matchAll(/Merge pull request #(\d{1,6}) from (?!Expensify\/(?:master|main|version-))/g)],
+        [...localGitLogs.matchAll(/{\[Merge pull request #(\d{1,6}) from (?!(?:Expensify\/(?:master|main|version-))|(?:[^(\]})]*\(cherry picked from commit .*\)\s*\]}))/g)],


So this has been a bit of magic and fiddling around with the regex, but this is what worked for the cases I could think of.

roryabraham · 2021-11-16T00:04:52Z

@mountiny What's your testing plan for this PR? Any way I can help?

mountiny · 2021-11-16T00:51:32Z

@roryabraham Got stuck with other issues and university. I have some coursework to focus on currently, but gave it a some quick testing now as I realize this is quite annoying bug.

Testing plan

Firstly, I went to App repository Actions and sound the deployBlocker job, which is run to update the comment.

Then I have found the latest one which has successfully completed: https://github.com/Expensify/App/runs/4214468887

Then I opened the Update StagingDeployCash with new deploy blocker section and since this run does not create new deploy issue, I searched for Getting pull requests merged between the following refs string in the logs to get the appropriate tags which the GH action used.

Next, I make sure my local tags are synced with remote using: git fetch --tags

Now, I run git log --format="{[%B]}" 1.1.13-2...1.1.14-2 > log.txt locally to get the same output string the GH action would get and save it to log.txt file. Here I am using the tags which have been used to create the most up to date staging issue and the logs are following:

{[Merge pull request #6255 from Expensify/OSBotify-cherry-pick-staging-6252

]}
{[Merge pull request #6252 from Expensify/chirag-removing-draftComments-param

Removing draft comments param

(cherry picked from commit 35283908f5936724d00ac6a718884a73daf1db50)
]}
{[Merge pull request #6254 from Expensify/version-BUILD-35283908f5936724d00ac6a718884a73daf1db50

(cherry picked from commit 8b477cbb902a6a0f0e3e554ef49e009ad622890f)
]}
{[Merge pull request #6244 from Expensify/OSBotify-cherry-pick-staging-6240

]}
{[Merge pull request #6240 from Expensify/vit-fixDeployBlockerBlankPage

[CP Staging] Fix blank page upon signing out

(cherry picked from commit f6e5fb5db11c0e93d48a93215f7879d51dd476a5)
]}
{[Merge pull request #6243 from Expensify/version-BUILD-f6e5fb5db11c0e93d48a93215f7879d51dd476a5

(cherry picked from commit 097134481f83ae0debc78bd06e0e1c7566d635c1)
]}
{[Merge pull request #6226 from Expensify/update-staging-from-main

]}
{[Merge branch 'main' into update-staging-from-main
]}
{[Merge pull request #6225 from Expensify/version-PATCH-1c4ff1733456e1ddbea985b50b1dd67ed1c070e4

]}
{[Update version to 1.1.14-0
]}
{[Merge pull request #6224 from Expensify/update-staging-from-main

]}
{[Merge branch 'main' into update-staging-from-main
]}
{[Merge pull request #6223 from Expensify/version-BUILD-9b9844e4e3907ba878817ee9c95202600bb3a0f2

]}
{[Update version to 1.1.13-4
]}
{[Merge pull request #6221 from Expensify/update-staging-from-main

]}
{[Merge branch 'main' into update-staging-from-main
]}
{[Merge pull request #6197 from AlfredoAlc/alfredo-modify-codestyle

Modifies code wrapper style iOS]}
{[Merge pull request #6220 from Expensify/version-BUILD-18a7153db75fd1729cdd7ac474e7ce8da958fc2a

]}
{[Update version to 1.1.13-3
]}
{[Merge pull request #6214 from Expensify/version-BUILD-9b9f6940157242e053bc9aad10d9e976f8291fd1

]}
{[Update version to 1.1.13-2
]}
{[Merge pull request #6201 from Expensify/joe-fix-menu-crash

]}
{[Merge pull request #6190 from PrashantMangukiya/prashant-5992

Added Spanish translation for example check image and Company types]}
{[Merge pull request #6205 from parasharrajat/keyboard-close

close Keyboard when opening drawer]}
{[Merge pull request #6080 from Expensify/marcaaron-fixUpPolicyMethods

]}
{[close Keyboard when opening drawer
]}
{[Update popover menu check to use isEmpty
]}
{[Merge pull request #6196 from Santhosh-Sellavel/Improve_Payment_Options_Floating_Button

Improvement Payment options Padding]}
{[Merge pull request #6084 from marktoman/issue-6050

LHN: Hide the pencil icon when message is sent or draft is deleted and show the icon when user started to type #6050]}
{[Modifies code wrapper style iOS
]}
{[Improve code style
]}
{[Merge branch 'main' of github.com:expensify/App into pr/marktoman/6084
]}
{[Merge branch 'main' of github.com:expensify/App into pr/marktoman/6084
]}
{[Merge pull request #6172 from Expensify/cmartins-removeStepCounter

Remove the step counter from "Let's finish in chat" step of VBA flow]}
{[Merge pull request #6149 from Expensify/Rory-AddValidationLinkRouteBack

]}
{[Improvement Payment options Padding
]}
{[Merge pull request #6083 from sidferreira/sidferreira-5972-mini-action-menu

Ensures mini actions menu will disappear when attachment modal opens up]}
{[Merge pull request #6193 from Expensify/version-BUILD-9fae7502f5565d631a5f5e71a16bc1ad5aaef0db

]}
{[Update version to 1.1.13-1
]}
{[Merge pull request #6191 from Expensify/yuwen-apos

]}
{[Fix an apostrophe to trigger a CP Build
]}
{[Merge pull request #6189 from PrashantMangukiya/prashant-5593

]}
{[Merge branch 'main' into Rory-AddValidationLinkRouteBack

# Conflicts:
#	src/pages/SetPasswordPage.js
]}
{[Removed trailing space to solve es lint problem.
]}
{[Refactored code within .then() block
]}
{[Update translation in es.js
]}
{[Merge pull request #6140 from akshayasalvi/tooltip-for-name

]}
{[Added Spanish translation for example check image and Company types
]}
{[Corrected infinite loading problem if non-image file uploaded
]}
{[Comply with the guidelines, minor fixes
]}
{[add propType
]}
{[add prop
]}
{[add shoulShowStepCounter prop
]}
{[change es copy
]}
{[change comment
]}
{[remove step counter
]}
{[Moved tooltip to the Text component
]}
{[make sure to return false if there is no url
]}
{[Merge remote-tracking branch 'origin' into marcaaron-fixUpPolicyMethods
]}
{[Fix SetPassword default props
]}
{[Import react
]}
{[Add login validation route back in NewDot
]}
{[Added tooltip for the communications link
]}
{[Update the tests with the new draft detection
]}
{[Replace the remaining use of the old draft detection
]}
{[Prevent saving the same value in Onyx
]}
{[Fix new comments not re-ordering, replace deprecated componentWillReceiveProps
]}
{[solution
]}
{[only update policies once
]}
{[Ensures mini actions menu will disappear when attachment modal opens up
]}
{[fix docs
]}
{[refactor shouldCreateFreePolicy method to AuthScreens class method
]}
{[Improve methods
]}
{[add createAndNavigate method
]}

I have tested this in the https://regex101.com/ tool but unfortunately I have came across a regression and that is PR #6240 which has been cherry picked is matched. The corresponding part of the log is this:

{[Merge pull request #6240 from Expensify/vit-fixDeployBlockerBlankPage

[CP Staging] Fix blank page upon signing out

(cherry picked from commit f6e5fb5db11c0e93d48a93215f7879d51dd476a5)
]}

The reason why it is matched is that there is the [CP Staging] in the name and the regex I am using currently gets mistaken by the closing ] and does not look further to see the cherry picked section.

I need to update the regex currently used {\[Merge pull request #(\d{1,6}) from (?!(?:Expensify\/(?:master|main|version-))|(?:[^(\]})]*$cherry picked from commit .*$\s*\]}) so the [^(\]})] does not treat ]} as single characters but one substring.

mountiny · 2021-11-16T01:06:03Z

We can't really use the character class there as this will always be prone to error in case the developer would use the character in the commit message or in this case PR title.

Unless, just got the idea now, we could update the cherry pick script to strip the [, ], { and } characters from the PR title when inserting it to the CP commit message. However, this seems to me like backup solution.

Do you have any ideas how we could do this with plain regex and the structure we have now (or using the <commit> tags instead, we would still not be able to simply use the character class in the regex)?

roryabraham · 2021-11-16T22:47:28Z

I spent some time trying to get this to work using a single regex with the example you were testing against, but didn't have any success. I believe it might be possible using a nested negative lookahead to match "anything except ]}" instead of "any characters except ] or }", but I spent like an hour on it and couldn't get it working.

An alternative would be to use two regex in a row. So we'd:

First take the full git log --format="{[%B]}" firstTag...secondTag output

Then use a regex like this: {\[([\s\S]*?)\]} to get an array where each item is a commit message (format spit out by %B)

[
    'Merge pull request #6255 from Expensify/OSBotify-cherry-pick-staging-6252',
    'Merge pull request #6252 from Expensify/chirag-removing-draftComments-param\nRemoving draft comments param\n(cherry picked from commit 35283908f5936724d00ac6a718884a73daf1db50)',
    ...
    ...
    ...
]

Then use a second regex, similar to the one you're already working with, to reduce that list to a list of pull request numbers. I think that might work better. At that point you would not need to worry about matching these characters anymore: {[]}

mountiny · 2021-11-22T17:39:24Z

@roryabraham I think going the way to split it into two regex is a good way... no reason to spend more time trying to find out some complex regex solving this. I will try to update the code in coming days. Having lots of coursework these weeks, so apologies for slower progress.

mountiny · 2021-11-25T00:08:24Z

.github/actions/createOrUpdateStagingDeploy/index.js

+    // Parse the git log into an array of commit messages between the two refs
+    const commitMessages = _.map(
+        [...localGitLogs.matchAll(/{\[([\s\S]*?)\]}/gm)],


I have used your advice and split it up to two regexps. Here, I get the individual commit messages which are all inside of the {[]}.

mountiny · 2021-11-25T00:09:58Z

.github/actions/createOrUpdateStagingDeploy/index.js

+    const pullRequestIDs = _.reduce(commitMessages, (mergedPRs, commitMessage) => {
+        const mergeCommits = [...commitMessage.matchAll(/Merge pull request #(\d{1,6}) from (?!(?:Expensify\/(?:master|main|version-))|(?:([\s\S]*?)\(cherry picked from commit .*\)\s*))/gm)];
+
+        // Get the PR number of the first match (there should not be multiple matches in one commit message)
+        if (_.size(mergeCommits)) {
+            mergedPRs.push(mergeCommits[0][1]);
+        }
+        return mergedPRs;
+    }, []);


And then I decided to use reduce on the list of commit messages. I use the regexp we have developed (slightly adjusted) to match the PR numbers. Then if some match found, I push the group to the accumulator array.

mountiny

@roryabraham Updated the parsing logic as we have discussed in this PR. The unit tests all pass well. Not sure if the matching logic inside of the reduce is the most elegant way to do this. Please, let me know if you can think of some nicer way!

Thank you for your and @tylerkaraszewski review on this one!

tylerkaraszewski · 2021-11-25T16:33:08Z

Lol, I got picked as reviewer but I basically have no idea how any of this works? Does someone want to walk me through the review, or should I let @roryabraham handle it?

roryabraham · 2021-11-26T19:08:16Z

@tylerkaraszewski I can review 😁 But in case you're curious, the TL;DR is:

Given two git refs, we want to figure out the list of pull requests which were merged between those refs.
When a PR is CP'd, it creates a duplicate commit that resembles the original merge commit, suffixed with cherry-picked from .... This causes us to falsely include pull requests which were merged outside the range of those two refs.
Oh yeah, and all the index.js files in the diff are compiled, and so you should just ignore them in a review.

roryabraham

I think this looks good to me 👍

mountiny · 2021-11-29T18:05:57Z

Thank you @roryabraham 🙇

roryabraham · 2021-11-29T19:00:56Z

Let's test this out!

roryabraham · 2021-11-29T19:26:45Z

Well, it didn't break the checklist, which is a good sign 👍

We should keep an eye on this PR, as an example. When we lock the checklist it should not get a comment saying it was deployed to staging, because it's already been CP'd to staging and deployed to production.

mountiny · 2021-11-29T19:29:52Z

Great news!

OSBotify · 2021-11-29T20:22:47Z

🚀 Deployed to staging by @roryabraham in version: 1.1.16-13 🚀

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

mvtglobally · 2021-11-30T19:47:37Z

@roryabraham @mountiny Can you validate and check this off?

roryabraham · 2021-12-06T23:24:11Z

@mountiny It seems like this didn't work. Check out this checklist ... #6535 was CP'd to staging then deployed to production but is present on a new checklist.

OSBotify · 2021-12-06T23:25:19Z

🚀 Deployed to production by @roryabraham in version: 1.1.17-7 🚀

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

mountiny added 6 commits November 1, 2021 19:29

Filter out the commits introduced by cherry picking

2550687

Add comment

4664a8a

Update the regex and format of log

62799cf

Update tests

11e7580

Update gitutils git log tests

d283a83

Update unit test and regex for cherry pick commit message

b911b88

mountiny added the Internal Requires API changes or must be handled by Expensify staff label Nov 1, 2021

mountiny requested a review from roryabraham November 1, 2021 23:25

mountiny requested a review from a team as a code owner November 1, 2021 23:25

mountiny self-assigned this Nov 1, 2021

MelvinBot requested review from tylerkaraszewski and removed request for a team November 1, 2021 23:25

Build the Github Actions

4bda84c

mountiny changed the title ~~Do not include CP merge commits in the list of PR merge commits~~ [HOLD] Do not include CP merge commits in the list of PR merge commits Nov 1, 2021

mountiny commented Nov 1, 2021

View reviewed changes

mountiny added 5 commits November 24, 2021 23:00

Merge branch 'main' into vit-updateMergeCommitRegex

76d2ab6

Use two regexp to parse the pr numbers

f839e13

Remove whitespace

aa1b134

Build the actions

b1fa8c6

Use arrow function

69a8816

mountiny changed the title ~~[HOLD] Do not include CP merge commits in the list of PR merge commits~~ Do not include CP merge commits in the list of PR merge commits Nov 25, 2021

Update logs

6106845

mountiny commented Nov 25, 2021

View reviewed changes

mountiny added 3 commits November 25, 2021 00:13

Adjust the array to not overdo the max char per line limit

7529734

Build the actions

c94553e

Fix linter

3c6d0dd

roryabraham approved these changes Nov 26, 2021

View reviewed changes

roryabraham merged commit 0393507 into main Nov 29, 2021

roryabraham deleted the vit-updateMergeCommitRegex branch November 29, 2021 19:01

OSBotify mentioned this pull request Nov 29, 2021

Deploy Checklist: New Expensify 2021-11-29 #6505

Closed

69 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not include CP merge commits in the list of PR merge commits #6165

Do not include CP merge commits in the list of PR merge commits #6165

mountiny commented Nov 1, 2021

mountiny left a comment

mountiny Nov 1, 2021

roryabraham commented Nov 16, 2021 •

edited

Loading

mountiny commented Nov 16, 2021

mountiny commented Nov 16, 2021

roryabraham commented Nov 16, 2021 •

edited

Loading

mountiny commented Nov 22, 2021

mountiny Nov 25, 2021 •

edited

Loading

mountiny Nov 25, 2021

mountiny left a comment

tylerkaraszewski commented Nov 25, 2021

roryabraham commented Nov 26, 2021

roryabraham left a comment

mountiny commented Nov 29, 2021

roryabraham commented Nov 29, 2021

roryabraham commented Nov 29, 2021

mountiny commented Nov 29, 2021

OSBotify commented Nov 29, 2021

mvtglobally commented Nov 30, 2021

roryabraham commented Dec 6, 2021

OSBotify commented Dec 6, 2021

Do not include CP merge commits in the list of PR merge commits #6165

Do not include CP merge commits in the list of PR merge commits #6165

Conversation

mountiny commented Nov 1, 2021

Details

Fixed Issues

Tests

QA Steps

Tested On

mountiny left a comment

Choose a reason for hiding this comment

mountiny Nov 1, 2021

Choose a reason for hiding this comment

roryabraham commented Nov 16, 2021 • edited Loading

mountiny commented Nov 16, 2021

Testing plan

mountiny commented Nov 16, 2021

roryabraham commented Nov 16, 2021 • edited Loading

mountiny commented Nov 22, 2021

mountiny Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

mountiny Nov 25, 2021

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

tylerkaraszewski commented Nov 25, 2021

roryabraham commented Nov 26, 2021

roryabraham left a comment

Choose a reason for hiding this comment

mountiny commented Nov 29, 2021

roryabraham commented Nov 29, 2021

roryabraham commented Nov 29, 2021

mountiny commented Nov 29, 2021

OSBotify commented Nov 29, 2021

mvtglobally commented Nov 30, 2021

roryabraham commented Dec 6, 2021

OSBotify commented Dec 6, 2021

roryabraham commented Nov 16, 2021 •

edited

Loading

roryabraham commented Nov 16, 2021 •

edited

Loading

mountiny Nov 25, 2021 •

edited

Loading