Skip to content

fix: return correct NX_BASE SHA for merge_group event#145

Merged
meeroslav merged 4 commits intonrwl:mainfrom
paulo9mv:patch-1
Feb 28, 2025
Merged

fix: return correct NX_BASE SHA for merge_group event#145
meeroslav merged 4 commits intonrwl:mainfrom
paulo9mv:patch-1

Conversation

@paulo9mv
Copy link
Copy Markdown
Contributor

@paulo9mv paulo9mv commented Mar 14, 2024

Context

As reported in #140, the current solution for merge-queue works only when the queue size 1. When more commits are added to the queue, the action keeps setting the same NX_BASE for all of them.

Problem
When new commits are added to the queue, we expect the NX_BASE commit to be the last commit in the queue. However, it sets the last commit in master. This image from #140 (comment) represents well the problem.

Solution proposed
The NX_BASE commit should be main branch HEAD commit when merge-queue has size 1. When size > 1, NX_BASE should be the HEAD commit of the last merge branch that current commit is on top of.

Fixes #140

@paulo9mv paulo9mv closed this Mar 14, 2024
@paulo9mv
Copy link
Copy Markdown
Contributor Author

paulo9mv commented Oct 16, 2024

Closed since I couldn't test enough scenarios

return result ? result.at(1) : undefined;
}

async function findMergeQueueBranch(): Promise<string> {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In merge-queue scenario, base will always be the previous commit in the queue. That's why we can remove these functions here.

@paulo9mv paulo9mv marked this pull request as ready for review December 19, 2024 12:32
Comment thread find-successful-workflow.ts Outdated
const baseResult = spawnSync('git', ['merge-base', `origin/${mainBranchName}`, 'HEAD'], { encoding: 'utf-8' });
BASE_SHA = baseResult.stdout;
} else if (eventName == "merge_group") {
const baseResult = spawnSync('git', ['rev-parse', 'HEAD^1'], { encoding: 'utf-8' });
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In merge-queue scenario, base will always be the previous commit in the queue.

@paulo9mv
Copy link
Copy Markdown
Contributor Author

@mandarini would appreciate your review

@jakemhiller
Copy link
Copy Markdown

We had to fork to pull in this patch (it's working great for us), it would be nice to get it integrated into the real action at some point soon.

@jordanpowell88
Copy link
Copy Markdown

We had to fork to pull in this patch (it's working great for us), it would be nice to get it integrated into the real action at some point soon.

Hey everyone! Sorry for the delay in getting a review on this. We've all been super swamped on other initiatives. Luckily I'm hoping to take a look at this soon

@meeroslav meeroslav self-assigned this Feb 28, 2025
@meeroslav
Copy link
Copy Markdown
Contributor

Looks good to me, thank you!

@meeroslav meeroslav changed the title fix: return correct base_sha when merge_group event happens fix: return correct NX_BASE SHA for merge_group event Feb 28, 2025
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.

Unexpected shas for Merge Queues

4 participants