Don't treat renovate/stability-days as passing status check

[rarkins]

If the only passing status check is renovate/stability-days then we should consider that as zero passing status checks and therefore still "pending". Original:

@andrewgies17 in my instance I use an external CI pipeline (Jenkins) which reports to GitLab. The Jenkins jobs sometimes take a while to start and if the stability days status check gets applied first, the MR can be merged before the Jenkins job has started. It would be useful if there was an option to only apply the stability days status check if there is already a passing pipeline.

Originally posted by @grit96 in #5743 (comment)

[rarkins]

This may be platform-specific though, possibly we cannot access the details to perform this type of filtering per-platform.

Also it might be worthwhile to move pass/pending/fail logic up from the platform layer (where we duplicate a lot of logic) to the worker layer.

[viceice]

Maybe a better option is to remove the stabillity check status instead of add it as passing?

[rarkins]

Maybe, but we're probably going to end up with a few renovate/* checks before too long - stability, confidence, automerge. So best if we learn to exclude them from counting

[geraintwhite]

The trouble with having the stability status check is that using GitLab's "Pipelines must succeed" feature, it is possible to manually merge MRs prematurely as well as renovate automerging if using external pipelines like I described in #5743 (comment)

[rarkins]

The proposed solution would stop undesirable automerging. However we can't stop all undesirable manual behavior.

[geraintwhite]

True, but removing the status check as @viceice suggested would prevent the manual merging in this case. Although based on your prediction of future status checks, excluding them from counting is the more sensible solution.

[rarkins]

Are either of you sure that's even possible? I only see adding and updating here: https://docs.gitlab.com/ee/api/commits.html#commit-status

[geraintwhite]

@rarkins doesn't look like it so I guess that option is ruled out!

[rarkins]

Hopefully GitLab will support mandatory status checks soon

[geraintwhite]

Could this be tied into https://github.com/renovatebot/renovate/issues/1853 (and the closed PR #4597)?

[philippe-granet]

@rarkins In Gitlab API: https://docs.gitlab.com/ee/api/commits.html#post-the-build-status-to-a-commit

|pipeline_id | integer | no | The ID of the pipeline to set status. Use in case of several pipeline on same SHA.
I think you should replace this:

renovate/lib/modules/platform/gitlab/index.ts

Line 786 in 53bd90b

await gitlabApi.postJson(url, { body: options });

by something like:

• polling to retrieve pipeline id of branchSha, started less than x seconds
• pass pipeline_id to commit status api

[rarkins]

The main reason for this is for preventing automerge. There's probably some people who would like to use this still in prCreation=not-pending

[rarkins]

Today if someone has internalChecksFilter=strict, stabilityDays=x and automerge=true, then this can happen:

• Branch created after x days
• stabilityDays check set to green
• PR created
• PR automerged because the PR is green

If instead we say that to use prCreation=not-pending requires you to have your own tests (and not just rely on our stability days), then I think we can solve this as described.

[Gabriel-Ladzaretti]

Is the stability test cross-platform or GitHub only?

[viceice]

cross platform

[viceice]

currently every platform handles status checks on its own. we should refactor it to return massaged status checks, like we do with PR / issues and handle all other in worker code. that way we can filter some status checks more easy

[rarkins]

I think we have it had an open issue to refactor status checks to the worker layer. I got a feeling I closed it recently, maybe thinking it was too hard in GitHub's case due to both status checks and check runs.

[nabeelsaabna]

what about the new suggested approach ?

Suggested solution:
Use the stability days test as a failing check, this means we will never set the status to green, but we will set it to red when failing to prevent anyone from merging, and also indicate that this check failed (this is needed regardless IMHO).

[viceice]

what about the new suggested approach ?

Suggested solution:
Use the stability days test as a failing check, this means we will never set the status to green, but we will set it to red when failing to prevent anyone from merging, and also indicate that this check failed (this is needed regardless IMHO).

doesn't work, as it can't be deleted when it was red and should be green now. so we need to update it to green.

[brennerm]

Can I somehow support in getting this issue fixed? We had a few cases were Renovate decided to automerge although our pipeline was pending or even failing.
We are using a self-hosted Gitlab and Renovate creates a pipeline containing the stability-days check only and automerges after it turns green.

[rarkins]

The getBranchStatus() function in each platform determines whether a branch is considered green or not. Example for github: https://github.com/renovatebot/renovate/blob/main/lib/modules/platform/github/index.ts#L818

To close this issue, each platform's getBranchStatus() would need to exclude any passing renovate/ checks, although it only really matters if those are the only checks.

One possible problem - and why I just set this back to requirements - is that there may be users who have no tests but rely on something like stability days before automerging. Technically this change would be backwards incompatible for them, so we need to decide:

• Change the default behavior, with no way to revert it, or
• Change the default behavior, with a new config option to revert it, or
• Keep default behavior, with a new config option to change to the desired functionality in this issue

I think the second option is probably best for Renovate users overall, because of the bad impact the current behavior can have on users. So therefore the plan would be:

• Create new option internalChecksAsSuccess which defaults to false
• This option is passed to getBranchStatus() and determines if renovate/* checks are counted as green
• Needs to be supported in every platform which supports getBranchStatus()
• Issue a major release for the change

If nobody has any strong objections to that, we can set this back to status:ready

[brennerm]

@rarkins Sounds great, would work for our use case.

[XFNeo]

Hi @rarkins ! When do you plan to release this feature?

[brennerm]

Thanks @rarkins for solving this issue. 🙏

[renovate-release]

🎉 This issue has been resolved in version 35.0.0 🎉

The release is available on:

• GitHub release
• 35.0.0

Your semantic-release bot 📦🚀

[XFNeo]

@rarkins Hi! It looks like this feature does not work. Bot still provides green pipeline for stasbility-days and merges such MRs.
Version 35.25.0

renovatebot_log.txt

[rarkins]

Please create a new Discussion (not issue) with reproduction and logs from a public gitlab.com repo to demonstrate