Fork mode: Previously edited branch does not get overridden/deleted #16657

devversion · 2022-07-19T18:57:41Z

How are you running Renovate?

Self-hosted

If you're self-hosting Renovate, tell us what version of Renovate you run.

32.119.0

Please select which platform you are using if self-hosting.

github.com

If you're self-hosting Renovate, tell us what version of the platform you run.

Github

Was this something which used to work for you, and then stopped?

I never saw this working

Describe the bug

With fork mode, renovate stops updating a group when a PR has been manually modified previously. e.g.

Renovate sends an update for a group called angular-deps
The PR starts to fail because we have size failures.
A maintainer pushes the pull request to fix up the size failures (this is possible since the PR has allowed maintainers to modify the fork branch)
The PR gets merged.
A few days/weeks later, still no update is sent because Renovate still has the old branch that was modified by someone other than the Renovate Git user.

This issue was visible in the angular/components repository, and here is the self-hosted runner entry: https://github.com/angular/dev-infra/runs/7415118482?check_suite_focus=true (look for Last commit author does not match git author email)

The PR modified in (3) was not immortal and the branch never got deleted upon merge: angular/components#25187 (this PR is an example -- Note that the branch was deleted manually just today after discovering this)

Note: It's unclear to me how this is supposed to work. Should the branch be deleted on merge? This is obviously more difficult when Renovate operates in a fork. Or should we just disable maintainer edits when forkMode: true?

Also super happy to look into contributing a fix if the expected behavior is clear and it's not a super large undertaking.

Relevant debug logs

Logs

DEBUG: getBranchPr(ng-renovate/angular-shared-dev-infra-code) (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: findPr(ng-renovate/angular-shared-dev-infra-code, undefined, open) (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: findPr(ng-renovate/angular-shared-dev-infra-code, undefined, closed) (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: branchExists=true (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: dependencyDashboardCheck=undefined (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: recreateClosed is false (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: findPr(ng-renovate/angular-shared-dev-infra-code, build: update angular shared dev-infra code to cc061c7, !open) (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: findPr(renovate/angular-shared-dev-infra-code, build: update angular shared dev-infra code to cc061c7, !open) (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: prAlreadyExisted=false (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: Checking if PR has been edited (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: Last commit author does not match git author email - branch has been modified (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) "branchName": "ng-renovate/angular-shared-dev-infra-code", "lastAuthor": "[email protected]", "gitAuthorEmail": "[email protected]" DEBUG: findPr(ng-renovate/angular-shared-dev-infra-code, undefined, !open) (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: Found PR #25180 (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: Found old PR but the SHA is different (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) "oldPrNumber": 25180, "oldPrSha": "f5d8e2acdc03b25318d6c78e8c9a420071d0c3eb", "branchSha": "f21a665004c41f68cff5effb8ad2c7b1217e728f" DEBUG: getBranchPr(ng-renovate/angular-shared-dev-infra-code) (repository=angular/components) DEBUG: findPr(ng-renovate/angular-shared-dev-infra-code, undefined, open) (repository=angular/components) DEBUG: findPr(ng-renovate/angular-shared-dev-infra-code, undefined, closed) (repository=angular/components) DEBUG: Ensuring Dependency Dashboard (repository=angular/components) DEBUG: ensureIssue(Dependency Dashboard) (repository=angular/components) DEBUG: Issue is open and up to date - nothing to do (repository=angular/components)

Have you created a minimal reproduction repository?

No reproduction repository

The text was updated successfully, but these errors were encountered:

rarkins · 2022-07-20T05:35:18Z

For now I'd say that if you edit a PR and then merge it then you need to delete the source branch.

For future, could Renovate reliably know when it can delete such a branch? Eg iterate through closed pull requests, check the branch it found against the head sha?

devversion · 2022-07-20T08:14:44Z

For now I'd say that if you edit a PR and then merge it then you need to delete the source branch.

yeah, it's difficult to communicate this to the whole team, that's why I was hoping we could just prevent maintainer edits for the renovate PRs, and instead team members would just clone the PR and fix it up separately (that's what I'm usually doing)

For future, could Renovate reliably know when it can delete such a branch? Eg iterate through closed pull requests, check the branch it found against the head sha?

Yeah, you mean, it would iterate through closed PRs and checks if the current edited SHA was HEAD a previous renovate PR? And if so, it would not mark the branch as edited but rather let it be re-used/overridden? That would seem reasonable to me.

Alternatively it might be a fair assumption/mental model to make: If there is no pending/open PR with the SHA from the edited branch, then the manual edits are no longer relevant and can be discarded. Usually when a Renovate-owned branch is modified, a PR is already open and once the PR is closed, the edits either landed or should be discarded. I like this a lot, but I'm not sure if I miss something here.

rarkins · 2022-07-20T11:53:37Z

I think we used to have that assumption (if a branch exists but no PR and someone has edited it then assume we are free to force push over the changes) but it caused problems with some edge case. I guess we need to decide which inconvenience is worse

codyoss · 2022-07-25T13:35:50Z

I have noticed this in the past month or so as well. I think it would be great if there was a flag to tell the bot if it could delete or force push to the branch if PRs for said dep upgrade have been merged. To workaround this today I have had to just change the branch naming that is used to get things unstuck. We for instance have some golden files that commonly need updating when new versions of deps get pulled in so this happens to us currently after ever renovate PR.

rarkins · 2022-07-26T04:08:56Z

I'm ok with the idea of adding intelligence to detect if an existing branch can be deleted (or at least reused), although I'm not sure how easy that will be to implement. Right now the logic is something like this:

if branchExists
- if branchIsModified then skip branch

We can hopefully add some logic here to say detect "if branch is actually left over from a previously merged PR", assuming that we can get a list of all previously merged PRs and their HEAD SHA.

devversion · 2022-07-26T14:21:26Z

I think we used to have that assumption (if a branch exists but no PR and someone has edited it then assume we are free to force push over the changes) but it caused problems with some edge case

I wonder what this edge-case was back then. To me, this philosophy seems like the most predictable and obvious one. There might be situations I don't think of here though -- hence my interest in the edge-case, if you remember 😄

rarkins · 2022-07-26T15:24:16Z

IIRC the edge case may have been with onboarding

devversion · 2022-07-26T16:47:20Z

Ah that makes sense. Potentially we could have a special case for the onboardingBranch. So that the rules are:

If the current branch is the onboardingBranch, do not delete it if there is an associated PR
For other branches, delete them if there is no associated pull request.

rarkins · 2022-07-27T05:48:25Z

The problem with onboarding branch is hopefully resolved already anyway. The reason people needed to edit the branch was to add config or credentials to overcome external host error aborts, but now we changed logic to not abort for external host errors during onboarding

devversion · 2022-10-25T08:32:05Z

Changing the deletion policy seems like a fundamental change, not necessarily complicated I'd assume- but would it be an option to make

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

Line 1432 in 8ee28a4

options.body.maintainer_can_modify = true;

configurable or false? This would generally avoid the issue and would encourage authors to just cherry-pick the Renovate commit and and their fixes on top of it.

If a maintainer pushes changes to a fork renovate PR, the branch will never be deleted by Renovate because there are changes not corresponding to the Git account configured in Renovate. This prevents future updates as Renovate basically locks down the branch and marks it as `pr-edited`, preventing it from being overriden or cleaned-up as part of the scheduled branch deletion. We can fix this by encouraging that maintainers cherry-pick the commit and make changes in a separate PR. It is not possible for maintainers to trivially delete the branch of the e.g. fork robot account because they would need to log into that and not every team member would e.g. necessarily have access to the robot account. Fixes renovatebot#16657.

…rkToken`is set Platforms like GitHub expose options to control whether a PR can be modified by maintainers. See e.g. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork This commit introduces a way to control this in Renovate when Renovate operates in a fork. The option will default to `true` for backwards compatibility. Users may use this option to workaround issues as outlined in: renovatebot#16657.

renovate-release · 2023-02-22T14:18:02Z

🎉 This issue has been resolved in version 34.151.0 🎉

The release is available on:

GitHub release
34.151.0

Your semantic-release bot 📦🚀

More context: renovatebot/renovate#16657 renovatebot/renovate@340a913

devversion added priority-5-triage status:requirements Full requirements are not yet known, so implementation should not be started type:bug Bug fix of existing functionality labels Jul 19, 2022

rarkins added priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:ready and removed priority-5-triage status:requirements Full requirements are not yet known, so implementation should not be started labels Jul 26, 2022

devversion mentioned this issue Jan 10, 2023

feat(platform/github): flag to control whether PRs can be edited by maintainers if forkTokenis set #19771

Merged

6 tasks

viceice closed this as completed in #19771 Feb 22, 2023

devversion added a commit to angular/dev-infra that referenced this issue Feb 22, 2023

build: never allow renovate PRs to be edited directly

65d6fba

More context: renovatebot/renovate#16657 renovatebot/renovate@340a913

devversion mentioned this issue Feb 22, 2023

build: never allow renovate PRs to be edited directly angular/dev-infra#1035

Merged

devversion added a commit to angular/dev-infra that referenced this issue Feb 22, 2023

build: never allow renovate PRs to be edited directly

a8ea953

More context: renovatebot/renovate#16657 renovatebot/renovate@340a913

github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fork mode: Previously edited branch does not get overridden/deleted #16657

Fork mode: Previously edited branch does not get overridden/deleted #16657

devversion commented Jul 19, 2022 •

edited

Loading

rarkins commented Jul 20, 2022

devversion commented Jul 20, 2022 •

edited

Loading

rarkins commented Jul 20, 2022

codyoss commented Jul 25, 2022

rarkins commented Jul 26, 2022

devversion commented Jul 26, 2022

rarkins commented Jul 26, 2022

devversion commented Jul 26, 2022

rarkins commented Jul 27, 2022

devversion commented Oct 25, 2022

renovate-release commented Feb 22, 2023

Fork mode: Previously edited branch does not get overridden/deleted #16657

Fork mode: Previously edited branch does not get overridden/deleted #16657

Comments

devversion commented Jul 19, 2022 • edited Loading

How are you running Renovate?

If you're self-hosting Renovate, tell us what version of Renovate you run.

Please select which platform you are using if self-hosting.

If you're self-hosting Renovate, tell us what version of the platform you run.

Was this something which used to work for you, and then stopped?

Describe the bug

Relevant debug logs

Have you created a minimal reproduction repository?

rarkins commented Jul 20, 2022

devversion commented Jul 20, 2022 • edited Loading

rarkins commented Jul 20, 2022

codyoss commented Jul 25, 2022

rarkins commented Jul 26, 2022

devversion commented Jul 26, 2022

rarkins commented Jul 26, 2022

devversion commented Jul 26, 2022

rarkins commented Jul 27, 2022

devversion commented Oct 25, 2022

renovate-release commented Feb 22, 2023

devversion commented Jul 19, 2022 •

edited

Loading

devversion commented Jul 20, 2022 •

edited

Loading