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

[roryabraham]

Problem

When a new deploy checklist is created, PRs that were cherry-picked in the previous deploy cycle are included in the new checklist. For example, the last two PRs in this deploy checklist were CP'd in the previous checklist:

Solution

Not sure, fix it!

[roryabraham]

No update here

[roryabraham]

No update here

[roryabraham]

Again no update here. And still don't really have a plan for how to fix this.

[roryabraham]

No update from last week 😬

[roryabraham]

Again no update. Removing N6 hold

[mountiny]

I stumbled upon this issue the other day and was intrigued by it so started to investigate.

What do you think of this as a why it is happening?

App/.github/actions/createOrUpdateStagingDeploy/createOrUpdateStagingDeploy.js

Lines 53 to 73 in c247f82

	// Parse the data from the previous StagingDeployCash
	// (newest if there are none open, otherwise second-newest)
	const previousStagingDeployCashData = shouldCreateNewStagingDeployCash
	? GithubUtils.getStagingDeployCashData(stagingDeployResponse.data[0])
	: GithubUtils.getStagingDeployCashData(stagingDeployResponse.data[1]);
	console.log('Found tag of previous StagingDeployCash:', previousStagingDeployCashData.tag);
	if (shouldCreateNewStagingDeployCash) {
	// Find the list of PRs merged between the last StagingDeployCash and the new version
	const mergedPRs = GitUtils.getPullRequestsMergedBetween(previousStagingDeployCashData.tag, newVersion);
	// eslint-disable-next-line max-len
	console.log(`The following PRs have been merged between the previous StagingDeployCash (${previousStagingDeployCashData.tag}) and new version (${newVersion}):`, mergedPRs);
	// TODO: if there are open DeployBlockers and we are opening a new checklist,
	// then we should close / remove the DeployBlockerCash label from those
	return GithubUtils.generateStagingDeployCashBody(
	newVersion,
	_.map(mergedPRs, GithubUtils.getPullRequestURLFromNumber),
	);
	}

Here, this is where we get data for the new deploy checklist. Since shouldCreateNewStagingDeployCash is true, the previousStagingDeployCashData.tag is tag of the latest release. This release is however created when we create new staging deployment and therefore, the CP'ed pull requests (the merge commits) happened after the moment this tag was created. As a result, the CP'ed PRs are mentioned in this issue as well as in the previous one.

In the previous one, they are included, because the refs we use for the existing one is new ref, which does not give the set upper boundary.

App/.github/actions/createOrUpdateStagingDeploy/createOrUpdateStagingDeploy.js

Lines 85 to 92 in c247f82

	// If we aren't sent a tag, then use the existing tag
	const tag = newVersion || currentStagingDeployCashData.tag;
	// Find the list of PRs merged between the last StagingDeployCash and the new version
	const mergedPRs = GitUtils.getPullRequestsMergedBetween(previousStagingDeployCashData.tag, tag);
	// eslint-disable-next-line max-len
	console.log(`The following PRs have been merged between the previous StagingDeployCash (${previousStagingDeployCashData.tag}) and new version (${tag}):`, mergedPRs);

What do you think of this? 👀 Honestly, I am not sure if I completely understand, when the new tags are created, but if it works as I imagine then what I described could be the problem. I would like to learn more about the Github Actions and I am not sure how these can be tested.

[roryabraham]

This release is however created when we create new staging deployment and therefore, the CP'ed pull requests (the merge commits) happened after the moment this tag was created.

In the previous one, they are included, because the refs we use for the existing one is new ref, which does not give the set upper boundary.

Sorry, I had difficulty following these sentences ^

[mountiny]

Sorry, it was kind of clumsy indeed.

What I am trying to say is that when we are creating new checklist, the previous tag is pointing to a moment in time (some commit) before those CP Staging PR's were merged. Then the getPullRequestsMergedBetween will include them in its response.

Then for the existing checklists, on this line:

App/.github/actions/createOrUpdateStagingDeploy/createOrUpdateStagingDeploy.js

Line 87 in c247f82

const tag = newVersion || currentStagingDeployCashData.tag;

When is the newVersion passed? If there is newVersion included, that is what I would imagine enables adding the cherry-picked PRs to the existent staging checklist.

I am not sure if I understand how we create and maintain the tags. I think we need to make sure that once the Staging deploy checklist is closed, its tag will point to the last cherry-picked merge commit, so then the new checklist will start searching for PRs from that moment in time.

[roryabraham]

What I am trying to say is that when we are creating new checklist, the previous tag is pointing to a moment in time (some commit) before those CP Staging PR's were merged.

That might be true, but it might not. After all, the actual thing that triggers a staging deploy is:

1. New code (CP'd or otherwise) is merged to the staging branch.
2. We tag staging
3. Then we deploy the code at that tag.

So the last tag on a deploy checklist should be the last PR that was CP'd (I think). But clearly it's not working correctly so I must be wrong somehow 🤷

One question I have ... does the commit history between two tags (i.e: the output of git log --format="%s" 1.1.8-9...1.1.8-22, for example), change depending on what branch you're on when you run the command? i.e: is the history between those two tags different depending on whether you're looking from the perspective of main or staging? Because if so, the correct history for the checklists should really be from the perspective of staging, and the solution might be to checkout the staging branch instead of main before running the createOrUpdateStagingDeploy action.

I am not sure if I understand how we create and maintain the tags.

Search for git tag in App/.github/

[mountiny]

New theory on the problem here, which stems from the duplicate PR issue you have resolved and the problem mentioned by Matt today. Essentially, I believe this is caused because when we cherry-pick there is a new Merge pull request commit created with the same PR number, therefore we then have two same commits in git log, one is dated in the old deploy and the CP one is the new deploy.

For instance, take this compare and look up this #5812 #5812 PR and you can see there are two cherry pick PRs.

As a solution, could we change the cherry pick merge commit message to be slightly different so it is not caught by the regex for the other merge commits? I assume we do not have any regex to look for Cherry pick commits as they are same as the normal merge commits.

[roryabraham]

If you're right, then I think the line we'll need to edit is right here, and we'll want to edit the commit message. It looks like the git cherry-pick command has a -e flag that allows you to edit the commit message prior to committing docs, but I've never used that and so we'll need to do lots of local testing and be careful not to break anything. I'm worried that option will open a text editor and allow the user to interactively input a commit message, because that would likely break our cherry pick workflow.

[mountiny]

@roryabraham Seems like the -e would open some editor indeed. What about adding the -x option which should automatically add certain message to the end of the commit message and we can then update the regex in here to not match those with the cherry picked from substring.

[mountiny]

I looked into this and I think we will need to make more changes. The command we are currently using has the --format="%s" flag, which means it only returns the commit subject. However, the (cherry picked from message is added to end of commit body. So currently, we are not even getting that message in.

If we would have the entire commit (ie subject + body) we would probably need to strap the new lines out.

[mountiny]

I would do following: use --format="{[%B]}" flag with the curly braces around the commit message to give us clear way to delimiter the commit messages in the git log (as combination of {[]} characters which should be unique to commit messages).

This returns following format:

{[Update version to 1.1.11-6
]}
{[Merge pull request #5745 from parasharrajat/fix-back

Fix: back handler for Drawer]}
{[comment improved
]}
{[Merge pull request #6141 from Expensify/version-BUILD-0c44613c92514b29f734b24d2c736704d31718e0

]}
{[Update version to 1.1.11-5
]}
{[Merge pull request #6061 from kakajann/add-tooltip-for-user-avatar

Add a tooltip for profile avatar and online status icon]}
{[Removed header from dropdown on the Send Money page.
]}
{[Remove the header from payment Options
]}
{[Merge pull request #6135 from Expensify/update-staging-from-main

]}
{[Merge branch 'main' into update-staging-from-main
]}
{[Merge pull request #6129 from Expensify/update-staging-from-main

]}
{[Merge branch 'main' into update-staging-from-main
]}
{[Merge pull request #6126 from Expensify/update-staging-from-main

]}
{[Merge branch 'main' into update-staging-from-main
]}
{[Merge pull request #6118 from Expensify/OSBotify-cherry-pick-staging-6114

]}
{[Merge pull request #6114 from Expensify/chirag-pronoun-fix

(cherry picked from commit 17b4b052a68678a4b05b376b0b04c1b52fa4c52f)
]}
{[Merge pull request #6117 from Expensify/version-BUILD-17b4b052a68678a4b05b376b0b04c1b52fa4c52f

(cherry picked from commit 7fde34f9b2f22266162e50585167ebaa6b0688ab)
]}
{[Changes
]}
{[fix: Routing issue
]}
{[Merge pull request #6108 from Expensify/update-staging-from-main

]}
{[Merge branch 'main' into update-staging-from-main
]}
{[Merge pull request #6105 from Expensify/update-staging-from-main

]}
{[Merge branch 'main' into update-staging-from-main
]}
{[Merge pull request #6101 from Expensify/OSBotify-cherry-pick-staging-6099

]}

Inside of these brackets we can more easily check for the CP commit.

[roryabraham]

Seems like a clever approach. If you wanted to you could create some even more explicit format for each commit that would never conflict with an actual commit message, such as --format="<commit>%b</commit>", resulting in XML-style tags wrapping each commit like so:

<commit>Update version to 1.1.11-6</commit>
<commit>Merge pull request #5745 from parasharrajat/fix-back

Fix: back handler for Drawer</commit>
<commit>Merge pull request #6114 from Expensify/chirag-pronoun-fix

(cherry picked from commit 17b4b052a68678a4b05b376b0b04c1b52fa4c52f)
</commit>

Then again, {[%B]} is more readable and commit messages are very unlikely to contain {[ or ]}, so your existing proposal might be fine too.

[MelvinBot]

This issue has not been updated in over 15 days. @mountiny, @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!

[mountiny]

Excuse us Melvin, we just merged 😡

[roryabraham]

@mountiny I believe I was able to confirm that the fix for this unfortunately did not work.

[mountiny]

@roryabraham are the actions supposed to work from main? I will try to explore once my coursework is over after this week. We added some more logs if I remember correctly.

[mountiny]

Interesting how this turned out, the doubled messages about the deploy for PRs have stopped, but the messages are for the original commits and not for the cherry pick commits. Thus on the CP'ed PR, we get some delay since the actual effect has already been deployed in the CP commits. Here is such a PR for example: #6741

I guess the problem is that now, we are filtering out the CP'ed commits and interact with those from original PR (for the purposes of the deploy checklist).

I think what we might have to do is to approach the problem from the other direction. Consider the CP'ed commits and then ignore the original PR commits. At least if I am understanding correctly what is going on here.

[roryabraham]

Okay, I'm going to attempt to summarize all the cases we have here, state the expected behavior, and try to trace the git logs. Following along with a simplified/minimal example:

1. We have a repo with 3 branches: main, staging, production, and an initial commit. Initial version 1.0.0-0 is tagged from staging

   

2. Pull request 123 is merged when the checklist is unlocked:

   1. main version is bumped to 1.0.0-1
   2. main is merged to staging
   3. staging is tagged to create 1.0.0-1
   4. The diff between 1.0.0-0 and 1.0.0-1 should include the merge commit for 123.

   

3. Then the checklist is locked

   1. main version is bumped to 1.0.1-0
   2. main is merged to staging
   3. staging is tagged to create 1.0.1-0
   4. The only diff between 1.0.0-1 and 1.0.1-0 should be the version bump commit.

   

4. Pull request 456 is merged to main but not CP'd

   

5. Pull request 789 is merged to main and CP'd to staging.

   1. main version is bumped to 1.0.1-1
   2. PR 789 is CP'd to staging
   3. Logs between 1.0.0-0 and 1.0.1-1 should include the changes from 123, checklist locked, and 789, but not 456

   
   
   

   Important note: In this case, we should not ignore the Merge pull request #789 (cherry picked from abcdefg). Ideally it should be added to the deploy checklist only after it has been merged to staging, not when it was merged to main.

6. Pull request 135 is merged to main but not CP'd.

   

   • Logs on main:

     

   • Logs on staging:

   

7. Checklist is closed with version 1.0.1-1:

   • Prod deploy:

     1. staging is merged into production.
     2. List of "deployed to production" PRs should be [123, 789], but not include [456, 135]

     

     Note: Again, the CP'd commit for #789 should be included

   • Staging deploy:

     1. main version is bumped to 1.0.1-2
     2. main is merged into staging
     3. List of PR's on new checklist should be [456, 135], and not include [123, 789].

     

     Note: Here, we see the original non-CP'd commit for #789 and it should be ignored. Furthermore, when this new checklist is closed, #789 should continue to be ignored for the sake of deploy comments.

[roryabraham]

So one thing I'm noticing is that, after all this is done, on production there is a single commit in the git log for PR #789:

And on staging, the full git log now includes two commits: one for the original PR and one for the CP PR:

But the diff between the two tags in question includes only the original PR (which at this point has already been CP'd to staging and deployed to production).

So maybe the logic we want is something like this:

1. Run the git log between the two tags in question to get a preliminary list of PRs
2. Search the git log on the branch in question (staging when creating a checklist or posting "deployed to staging" comments, or production when creating a release or posting "deployed to production" comments), and exclude any PR's that appear in the log more than once with both an "original merge commit" and a "CP merge commit".

[roryabraham]

Got a draft PR up here, but I'm still working on testing it.

[roryabraham]

PR is ready for review.

[roryabraham]

Completed QA of #6906, and with that I declare this issue officially solved 😄