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

Unexpected shas for Merge Queues #140

Open
Fitzbert-Fitz opened this issue Jan 31, 2024 · 1 comment
Open

Unexpected shas for Merge Queues #140

Fitzbert-Fitz opened this issue Jan 31, 2024 · 1 comment

Comments

@Fitzbert-Fitz
Copy link

Hi there,
as also explained here #100 we encounter massive building overhead due to the sha calculation.
What we would expect:
assume you have two PRs queued in the merge queue. Tha latest main build is successful. We would assume that Queue positon '#1' would derive latest main(successful) as base sha. Position '#2' in the queue would derive position '#1' as base sha since it assumes that position '#1' will pass the queue as well.
image

But what we see is that the base sha is always the git merge-base / the common ancestor. Meaning Position '1#' in the queue builds all changes since the branch was created not taking into account that builds on main might have been successful in the meantime. The same for position '#1' in the queue. See below:
image

Looking at the code we do not understand why git merge-base is used here. It might be a good fall back in case you do not find successful commits on main. Maybe miss ordered inside the if else statement?

if (
(["pull_request", "pull_request_target"].includes(eventName) &&
!github.context.payload.pull_request.merged) ||
eventName == "merge_group"
) {
try {
const mergeBaseRef = await findMergeBaseRef();
const baseResult = spawnSync(
"git",
["merge-base", `origin/${mainBranchName}`, mergeBaseRef],
{ encoding: "utf-8" }
);
BASE_SHA = baseResult.stdout;
} catch (e) {
core.setFailed(e.message);
return;
}
} else {
try {
BASE_SHA = await findSuccessfulCommit(
workflowId,

@mandarini
Copy link
Member

Hi there @Fitzbert-Fitz ! Thanks for filing an issue! Do you think the changes suggested by @paulo9mv in the PR linked above would solve your issue? Do have a suggested implementation that would make things smoother?

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

No branches or pull requests

2 participants