renovate
renovate copied to clipboard
renovatebot
Don't treat renovate/stability-days as passing status check
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 https://github.com/renovatebot/renovate/issues/5743#issuecomment-732796687
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.
Maybe a better option is to remove the stabillity check status instead of add it as passing?
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
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 https://github.com/renovatebot/renovate/issues/5743#issuecomment-732796687
The proposed solution would stop undesirable automerging. However we can't stop all undesirable manual behavior.
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.
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
Could this be tied into https://github.com/renovatebot/renovate/issues/1853 (and the closed PR https://github.com/renovatebot/renovate/pull/4597)?
@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: https://github.com/renovatebot/renovate/blob/53bd90b30faddcc9bf916179adecaa902ab99beb/lib/modules/platform/gitlab/index.ts#L786
by something like:
- polling to retrieve pipeline id of branchSha, started less than x seconds
- pass pipeline_id to commit status api
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
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.
Currently we Updating renovate/stability-days status check state to green whenever the stability days test checks.
example log message:
,"msg":"Updating renovate/stability-days status check state to green","time":"2022-05-31T13:29:48.412Z","v":0}
,"context":"renovate/stability-days","state":"green","msg":"Setting branch status","time":"2022-05-31T13:29:48.421Z","v":0}
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).
currently renovate is creating a PR for a library that does not pass the stability days check AND it would automerge it in case all other "checks" pass and marks the branch green.
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
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.
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).
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.
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.
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
internalChecksAsSuccesswhich defaults tofalse - This option is passed to
getBranchStatus()and determines ifrenovate/*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
:tada: This issue has been resolved in version 35.0.0 :tada:
The release is available on:
- GitHub release
-
35.0.0
Your semantic-release bot :package::rocket:
@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
Please create a new Discussion (not issue) with reproduction and logs from a public gitlab.com repo to demonstrate