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

Bug: assignees and reviewers are reapplied to automerge PRs with failed status checks #4624

Closed
buchstabensalat opened this issue Oct 10, 2019 · 14 comments
Labels
core:automerge Relating to Renovate's automerge capabilities priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:requirements Full requirements are not yet known, so implementation should not be started type:bug Bug fix of existing functionality

Comments

@buchstabensalat
Copy link

What would you like Renovate to be able to do?
We are using a self-hosted version of renovate on self-hosted gitlab instances (both CE and EE).
We have a general dispatching user to which new Merge-Requests are assigned.
The dispatching user now decides if the merge request will be merged directly or assigns it to another user if he is not able to decide if this is going to break stuff.
The second user has then the responsibility to check this MR in detail.

Our renovate bot runs every hour and every time it runs, it re-assigns the MR back to the dispatching user, which is quite annoying

Describe the solution you'd like
It would be great if there would be a config option to tell renovate to only assign a MR once upon creation.

Additional context
renovate_assign

@rarkins rarkins added platform:gitlab GitLab Platform ready type:bug Bug fix of existing functionality priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others labels Oct 10, 2019
@rarkins
Copy link
Collaborator

rarkins commented Oct 10, 2019

I'm really surprised that's happening. Can you capture the logs at debug level during one run when Renovate reassigns the user? Renovate normally shouldn't update an MR at all unless it thinks title or body needs to change, and then I wouldn't expect it to update the reviewers.

@testorangemac
Copy link

We experience the same issue. Here is our debug log.

Please let us know if you need any further information.

@rarkins
Copy link
Collaborator

rarkins commented Jan 6, 2020

What line(s) of the log are applicable? Eg where is the assignment and unassignment happening?

@testorangemac
Copy link

This is only the part of the log where @types/node is processed. From line 65 a new assignee is added (again) but it seems like it doesn't log that there was already an assignee at the time of processing.

Log

@rarkins
Copy link
Collaborator

rarkins commented Jan 7, 2020

We do not add assignees and reviewers on MR creation if the MR is configured for automerge.

If status checks fail (preventing automerge) then this logic adds them:

if (
config.automerge &&
['failure', 'error', 'failed'].includes(await getBranchStatus())
) {
logger.debug(`Setting assignees and reviewers as status checks failed`);
await addAssigneesReviewers(config, existingPr);
}

This means assignees and reviewers will continue to be added. I think if you add prCreation="not-pending" to your config then this might work around the issue because then assignees and reviewers will be added at MR creation time. It shouldn't hurt to add it even if it isn't effective as a workaround.

@rarkins rarkins changed the title Assign a PR/MR only on creation Assignees and Reviewers are reapplied to automerge PRs with failed status checks Jan 7, 2020
@rarkins rarkins changed the title Assignees and Reviewers are reapplied to automerge PRs with failed status checks Bug: assignees and reviewers are reapplied to automerge PRs with failed status checks Jan 7, 2020
@rarkins
Copy link
Collaborator

rarkins commented Jan 7, 2020

A better fix would be if we check for existing assignees and reviewers before applying the in this function:

async function addAssigneesReviewers(config, pr: Pr): Promise<void> {

If any exist, then we shouldn't add them.

@testorangemac
Copy link

testorangemac commented Jan 7, 2020

I forgot to add that our situation seems a little different. We simply let the renovate bot do its thing hourly, let it automatically merge when our pipeline succeeds and only want people to be assigned in case the pipeline fails.

The problem being that as soon as a merge request fails that it assigns someone but keeps re-assigning people hourly afterwards. I think prCreation="not-pending" would not serve as a workaround in this case? Since every hour after creation it will still re-assign people?

@rarkins
Copy link
Collaborator

rarkins commented Jan 7, 2020

If we do a check for any existing assignees/reviewers then that should solve both problems, i.e.

  • We won't keep re-assigning the same over and over
  • We won't unassign/assign if you manually make changes

Essentially: if automerge is enabled, and tests fail, then only add assignees and reviewers if none already exist.

The only edge case this won't cover is if you want to fully remove assignees/reviewers, because in such a case Renovate will keep adding them again.

@rarkins rarkins removed the ready label Jun 18, 2020
@rarkins rarkins added the core:automerge Relating to Renovate's automerge capabilities label May 31, 2021
@rarkins
Copy link
Collaborator

rarkins commented Jan 14, 2022

I'm not sure the proposed solution is perfect because it would prevent the following from working:

  • Platform already has some form of automated assignees (via CODEOWNERS or another bot)
  • Renovate users wish to assign someone else

@rarkins rarkins added status:requirements Full requirements are not yet known, so implementation should not be started and removed status:ready labels Jan 14, 2022
@njibhu
Copy link

njibhu commented Feb 7, 2022

I am encountering the same issue. I was actually expecting the behavior to be:

  • Renovate will assign the reviewers only when it creates the PR following its configuration, and not touch reviewers anymore until the PR is merged.

And if I want to change the reviewers afterwards myself, renovate won't try to interfere with it
This would play nicely with branch protection rules, and even if there are CODEOWNERS that are not setup as reviewers in renovate, then the git platform would automatically add them (That's how github works, and I think gitlab as well)

@rarkins
Copy link
Collaborator

rarkins commented Feb 7, 2022

@njibhu sounds like you're expecting https://docs.renovatebot.com/configuration-options/#assignautomerge to default to true?

For that scenario we could definitely skip assigning once tests fail.

@njibhu
Copy link

njibhu commented Feb 7, 2022

I missed that configuration setting while reading the doc!
Then ignore my comment above
Thanks @rarkins

@rarkins
Copy link
Collaborator

rarkins commented Feb 7, 2022

@njibhu I think it will only fully solve your problem once #14065 is merged

@hasanwhitesource
Copy link
Contributor

This is fixed.
I had renovate create a merge request and to assign it to a user on a Gitlab repo then I changed the assignee and ran renovate again. It did not reassign the merge request back to the original user after the change.
Also I had the pipeline fail for the test.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core:automerge Relating to Renovate's automerge capabilities priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:requirements Full requirements are not yet known, so implementation should not be started type:bug Bug fix of existing functionality
Projects
None yet
Development

No branches or pull requests

5 participants