fix(pr): ensure PR update after branch commit

[PhilipAbed]

Changes

i stopped going inside the IF case, if we have a PR created already,
the code was getting inside the IF case also when there's a branch update, but that doesn't make sense to me.

@rarkins @viceice this solution works, but i'm not sure if i impact any other flow that i dont know about,
it seems logical to me,
but you know better.

Context

Closes #17388

Documentation (please check one with an [x])

• I have updated the documentation, or
• No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

• Code inspection only, or
• Newly added/modified unit tests, or
• No unit tests but ran on a real repository, or
• Both unit tests + ran on a real repository

[rarkins]

We need to be careful about unintended side effects.

Here's the if which now won't be entered:

renovate/lib/workers/repository/update/branch/index.ts

Line 556 in 8ddc944

// break if we pushed a new commit because status check are pretty sure pending but maybe not reported yet

Here's the next if which now might be entered:

renovate/lib/workers/repository/update/branch/index.ts

Line 574 in 8ddc944

// Try to automerge branch and finish if successful, but only if branch already existed before this run

So we might want to skip that second one if commitSha exists?

We also need to consider what the return {} value will be

[viceice]

this will cause early automerge again, check the comment above the if condition.

we added this to suppress automerge, because status checks can be missing and renovate has probably at green stability check.

on some platforms you can workaround with required status checks, but not always.

[PhilipAbed]

@rarkins yes i dont want to enter in case there's no PR for that branch on, this is supposed to take care of PR CREATION only

renovate/lib/workers/repository/update/branch/index.ts

Line 556 in 8ddc944

// break if we pushed a new commit because status check are pretty sure pending but maybe not reported yet

that leaves the PR UPDATE flow that i don't want it to enter there,
this should pass the above IF statement, since its not PR creation, but it is a new commit, a new update on the branch.

@viceice let's say we have Automerge, and we did an Update flow, why do u step into an IF statement that belongs to the PR Creation?

shouldn't the check for automerge be after it? at

renovate/lib/workers/repository/update/branch/index.ts

Line 574 in 8ddc944

// Try to automerge branch and finish if successful, but only if branch already existed before this run

i see if (branchExists || config.ignoreTests) {
but i can see that this is not enough,
what if we change it?
shouldn't we check status of branch before ?

[viceice]

@rarkins yes, i think we need to check commitSha and skip trying automerge then

@PhilipAbed #5594 is the original introduction of that condition. it was added to supress automerge when a new commit was made. The code was very different than now and it was easiest solution. You can now try to refactor but you need to make sure automerge is skipped when a new commit was made. The skipped PR update was a trade of not doing automerge.

[PhilipAbed]

there are too many flows going on

1. artifact error flow
2. pr creation immediate
3. pr creation not immediate
4. pr update immediate
5. pr update not immediate
6. automerge immediate
7. automerge not immediate

There's no way to do this without a refactor, ill see what i can do

not to mention that prCreation !== immediate is also affecting a case where there's an update that is not immediate
are you saying that prCreation !== immediate means that checks are required before every change?

[PhilipAbed]

@rarkins @viceice i believe we should split the Immediate flow from the non-immediate flow
the functionalities of branch update/ Auto merge / ensuring pr should be in functions but i can't refactor there are over 30 returns in the middle of the code, it's not that easy

i just added double condition
now
what changed:

before:
pr update not immediate flow - exit early skip auto merge and ensure PR

after:
pr update not immediate flow - will now ensure PR but skip automerge

i check if branchPr exists, and that is how i acknowledge if its an update flow or a creation flow.

[PhilipAbed]

it's working according to my test at:

reproduction:
StinkyLord/prcreationimmediate#2
( i changed name but see contents it says lodash to 4.17.20 while the branch is updated to 4.17.21)

after fix:
StinkyLord/prcreationimmediate#6

[PhilipAbed]

@rarkins a question that has been bothering me,
why is Automerge before ensuring PR?

doesn't make sense to me,
so you are saying some branch could change, the title/content aren't updated, yet we merge it?

if Automerge is before EnsurePR, then it should have a condition saying: there are no new commits
in the processBranch the automerge IF should have commitSha=null right? but i dont see it

[rarkins]

It's branch automerge

[rarkins]

We need to make sure that we don't branch or PR automerge after a fresh commit unless ignoreTests=true

[PhilipAbed]

you say we should not even try to auto merge branch when there's a fresh commit
but if you look at the current code without any changes:

renovate/lib/workers/repository/update/branch/index.ts

Lines 558 to 563 in 6f051f3

	if (
	!config.artifactErrors?.length &&
	!userRebaseRequested &&
	commitSha &&
	config.prCreation !== 'immediate'
	) {

this IF condition only quits if prCreation !== immediate

so in case the prCreation flag is not used, it would just skip to the automerge where

renovate/lib/workers/repository/update/branch/index.ts

Line 575 in 6f051f3

if (branchExists || config.ignoreTests) {

and enter the loop in case we have an update on the branch..
the update is a Fresh commit, where commitSha is not null. but it is not checked.

so if i add another check to it to make sure i dont do auto merge that would change logic right? is that ok?

that condition also doesnt makes sense to me why BranchExists || config.IgnoreTests
branch exists is a MUST, can we try to automerge without having a branch that exists??

[rarkins]

In the current implementation it would have already returned before line 558

[PhilipAbed]

i will test and see

[viceice]

That's why i've doen it that way long ago. the early return was the easiest solution to not automerge 😉

[PhilipAbed]

it actually reaches the condition even if i dont use the config.automerge flag
it doesn't return early as you mentioned
but inside the function tryBranchAutomerge(config) it would return 'no automerge';

so i'm not sure what you mean by the early return as it reaches that line in the branch update case,
i believe if i add automergeType='branch' it should work normally

[PhilipAbed]

Thanks a lot @rarkins @viceice i learned a lot from this, there are certainly lots of flows and cases

I tried to refactor, but i see more than 30 exits to processBranch function with 700+ rows .
the impact is too large so i will not do that unless its necessary.

hope i covered everything in my changes

[PhilipAbed]

renovate/lib/workers/repository/update/branch/index.spec.ts

Lines 511 to 528 in 6f051f3

	it('returns if branch automerged and no checks', async () => {
	getUpdated.getUpdatedPackageFiles.mockResolvedValueOnce({
	updatedPackageFiles: [{}],
	} as PackageFilesResult);
	npmPostExtract.getAdditionalFiles.mockResolvedValueOnce({
	artifactErrors: [],
	updatedArtifacts: [partial<FileChange>({})],
	} as WriteExistingFilesResult);
	git.branchExists.mockReturnValue(false);
	git.getBranchCommit.mockReturnValue('123test');
	automerge.tryBranchAutomerge.mockResolvedValueOnce('automerged');
	await branchWorker.processBranch({
	...config,
	ignoreTests: true,
	});
	expect(automerge.tryBranchAutomerge).toHaveBeenCalledTimes(1);
	expect(prWorker.ensurePr).toHaveBeenCalledTimes(0);
	});

this test is failing because i added the !commitSha check condition on the IF of the automerge branch section, so it's skipping the call to tryBranchAutomerge,
and failing on row expect(automerge.tryBranchAutomerge).toHaveBeenCalledTimes(1);

in this test:
Branch Exists = false
IgnoreTests = true
commitSha = 123test

i still don't understand which cases have BranchExists=false? how can we process a branch that doesn't exist?
because of this missing knowledge i'm not sure what to do with this test
please advise

[viceice]

branch exists false probably means we created it in this run.

[viceice]

otherwise LGTM

[PhilipAbed]

@viceice @rarkins looks great now :D can we merge? or are we waiting for anything?

[renovate-release]

🎉 This PR is included in version 34.26.2 🎉

The release is available on:

• GitHub release
• 34.26.2

Your semantic-release bot 📦🚀