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

devversion · 2023-01-10T17:31:16Z

Changes

A new option called forkModeAllowMaintainerEdits is introduced. This option is only relevant if Renovate is used with forkToken.

The option is by default true, keeping backwards compatibilty
The option will only have an effect for github.

The option decides whether maintainer edits are allowed to edit pull requests.

Context

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 overridden 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. Users of the experimental fork mode option can do this by setting prAllowMaintainerEdits = false. 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 Fork mode: Previously edited branch does not get overridden/deleted #16657

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 select 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

lib/modules/platform/github/index.ts

viceice

needs to be configurable and enabled by default to be not breaking

This commit ensures the GitHub platform logic respects the new `prallowMaintainerEdits` option. Whenever a PR is created, depending on the option value, the GitHub API post request will have the respective option `maintainer_can_modify` option set. This option was always enabled in fork mode before this commit, and without fork mode enabled, the option is a noop because maintainers have access to upstream branches regardless. Users may want to leverage the option and disable it to avoid issues with modifications in Renovate fork accounts, as described in: renovatebot#19771 Fixes renovatebot#19771.

devversion · 2023-01-24T15:16:46Z

Thanks for the feedback. I've addressed it by introducing a new option. I will keep this a draft to iterate more on if this is what you expect. Would appreciate some early feedback. Thanks!

One note: I wasn't sure if this option should be "Github"-specific and reside in PlatformPrOptions. I've just made it a general PR create option similar to the draft-mode option. The option doesn't seem necessarily GitHub-specific. Happy to change though.

rarkins

This is only relevant to forkingRenovate. Right now it's configurable for both and the docs don't make it clear enough. Let's try to avoid confusing the 99% of users not affected by this
Can this be made global config instead (i.e. controlled by the bot admin, not the repo admin)?

docs/usage/configuration-options.md

lib/config/options/index.ts

docs/usage/configuration-options.md

This commit ensures the GitHub platform logic respects the new `prallowMaintainerEdits` option. Whenever a PR is created, depending on the option value, the GitHub API post request will have the respective option `maintainer_can_modify` option set. This option was always enabled in fork mode before this commit, and without fork mode enabled, the option is a noop because maintainers have access to upstream branches regardless. Users may want to leverage the option and disable it to avoid issues with modifications in Renovate fork accounts, as described in: renovatebot#19771 Fixes renovatebot#19771.

devversion · 2023-02-02T17:24:27Z

I've addressed all feedback (and appreciate anybody having another look. Thanks for all the comments)

^{Github seems to just send it to one of the reviewers}

lib/config/options/index.ts

lib/modules/platform/github/index.ts

This commit ensures the GitHub platform logic respects the new `prallowMaintainerEdits` option. Whenever a PR is created, depending on the option value, the GitHub API post request will have the respective option `maintainer_can_modify` option set. This option was always enabled in fork mode before this commit, and without fork mode enabled, the option is a noop because maintainers have access to upstream branches regardless. Users may want to leverage the option and disable it to avoid issues with modifications in Renovate fork accounts, as described in: renovatebot#19771 Fixes renovatebot#19771.

…oken` is set This commit ensures the GitHub platform logic respects the new `prallowMaintainerEdits` option when `forkToken` is set and Renovate operates in a fork. Whenever a PR is created with `forkToken` being set, depending on the option value, the GitHub API post request will have the respective option `maintainer_can_modify` option set. This option was always enabled in fork mode before this commit, and without fork mode enabled, the option is a noop because maintainers have access to upstream branches regardless. Users may want to leverage the option and disable it to avoid issues with modifications in Renovate fork accounts, as described in: renovatebot#19771 Fixes renovatebot#19771.

…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.

…oken` is set This commit ensures the GitHub platform logic respects the new `prallowMaintainerEdits` option when `forkToken` is set and Renovate operates in a fork. Whenever a PR is created with `forkToken` being set, depending on the option value, the GitHub API post request will have the respective option `maintainer_can_modify` option set. This option was always enabled in fork mode before this commit, and without fork mode enabled, the option is a noop because maintainers have access to upstream branches regardless. Users may want to leverage the option and disable it to avoid issues with modifications in Renovate fork accounts, as described in: renovatebot#19771 Fixes renovatebot#19771.

devversion · 2023-02-03T14:44:09Z

I've incorporated all the docs suggestions and switched back to a repo config setting. A global option would simplify this a bit, but would make it impossible to use this option when e.g. the forking renovate GitHub app is used.

Sorry. I've also force-pushed to clean-up the commits which made it more difficult to review- a full review is easier in this situation anyway (and very likely needed anyway)

devversion · 2023-02-10T16:18:26Z

I'm good restricting the option name also to forkMode. Updated as per review suggestions. One extra note: I changed the createPr option from allowMaintainerEdits also to forkModeAllowMaintainerEdits because otherwise it would be quite ambiguous. We could even go further and move it into platform options I assume.

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

docs/usage/configuration-options.md

devversion · 2023-02-10T16:34:01Z

FYI: I will wait with any docs changes until we have settled on runtime code, naming, or if this should be a global only flag etc.

lib/workers/repository/onboarding/pr/index.ts

…isallowMaintainerEdits`

docs/usage/configuration-options.md

lib/config/options/index.ts

lib/modules/platform/types.ts

HonkingGoose

How about using a simpler name like forkModeBlockMaintainerEdits?

The word disallow feels clunky to me. I think a word like block is easier to understand.

I'll let the maintainers decide if they want to apply my suggestions.

docs/usage/configuration-options.md

lib/config/options/index.ts

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

lib/modules/platform/github/index.ts

lib/modules/platform/types.ts

lib/workers/repository/update/pr/index.ts

devversion · 2023-02-22T10:26:37Z

Yeah, if the maintainers think they prefer block, I can make the change. I personally prefer (dis)allow since that is the terminology used by GitHub as well. Blocking feels different to me (but I'm a non-native speaker) See:

and 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.

viceice · 2023-02-22T10:38:05Z

i think disallow is the right thing 🤷‍♂️

HonkingGoose · 2023-02-22T10:38:52Z

I personally prefer (dis)allow since that is the terminology used by GitHub as well.

Good point, let's stick with dis(allow) then. 😄

HonkingGoose · 2023-02-22T10:41:26Z

@viceice can you resolve my PR review comments? I can't do this myself: ¹

You can resolve a conversation in a pull request if you opened the pull request or if you have write access to the repository where the pull request was opened.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews#resolving-conversations ↩

renovate-release · 2023-02-22T14:17:59Z

🎉 This PR is included in version 34.151.0 🎉

The release is available on:

GitHub release
34.151.0

Your semantic-release bot 📦🚀

rarkins marked this pull request as draft January 10, 2023 17:35

rarkins reviewed Jan 10, 2023

View reviewed changes

lib/modules/platform/github/index.ts Show resolved Hide resolved

viceice requested changes Jan 24, 2023

View reviewed changes

devversion force-pushed the maintainer-modify branch from 6f03122 to b477ca6 Compare January 24, 2023 15:11

devversion changed the title ~~fix(github): do not allow maintainer PR edits for fork PRs~~ feat: add support for controlling whether PRs can be edited by maintainers Jan 24, 2023

rarkins requested changes Jan 25, 2023

View reviewed changes

HonkingGoose suggested changes Feb 1, 2023

View reviewed changes

docs/usage/configuration-options.md Outdated Show resolved Hide resolved

docs/usage/configuration-options.md Outdated Show resolved Hide resolved

lib/config/options/index.ts Outdated Show resolved Hide resolved

docs/usage/configuration-options.md Outdated Show resolved Hide resolved

devversion force-pushed the maintainer-modify branch from b477ca6 to 134c62f Compare February 2, 2023 17:21

devversion requested review from HonkingGoose, rarkins and viceice and removed request for HonkingGoose, rarkins and viceice February 2, 2023 17:23

viceice requested changes Feb 2, 2023

View reviewed changes

lib/config/options/index.ts Outdated Show resolved Hide resolved

lib/modules/platform/github/index.ts Outdated Show resolved Hide resolved

devversion force-pushed the maintainer-modify branch from 134c62f to 3b753d3 Compare February 3, 2023 14:30

devversion force-pushed the maintainer-modify branch from 3b753d3 to 1e6a4ae Compare February 3, 2023 14:38

devversion added 2 commits February 3, 2023 14:38

devversion force-pushed the maintainer-modify branch from 1e6a4ae to d97e5b8 Compare February 3, 2023 14:39

devversion requested review from viceice and removed request for rarkins February 3, 2023 14:44

devversion requested a review from rarkins February 10, 2023 16:18

HonkingGoose reviewed Feb 10, 2023

View reviewed changes

lib/modules/platform/github/index.spec.ts Outdated Show resolved Hide resolved

docs/usage/configuration-options.md Outdated Show resolved Hide resolved

docs/usage/configuration-options.md Show resolved Hide resolved

viceice reviewed Feb 10, 2023

View reviewed changes

lib/workers/repository/onboarding/pr/index.ts Outdated Show resolved Hide resolved

devversion added 2 commits February 21, 2023 14:26

Merge remote-tracking branch 'upstream/main' into maintainer-modify

506e4e8

Pass as platform option, update docs, and rename option to `forkModeD…

017af7d

…isallowMaintainerEdits`

devversion requested review from HonkingGoose and removed request for rarkins February 21, 2023 17:11

no bare links

2efbbfa

viceice reviewed Feb 22, 2023

View reviewed changes

docs/usage/configuration-options.md Outdated Show resolved Hide resolved

docs/usage/configuration-options.md Outdated Show resolved Hide resolved

lib/config/options/index.ts Outdated Show resolved Hide resolved

lib/modules/platform/types.ts Outdated Show resolved Hide resolved

cleanup markdown and use same option name in platform options

824c99f

viceice approved these changes Feb 22, 2023

View reviewed changes

viceice requested a review from rarkins February 22, 2023 10:04

viceice enabled auto-merge February 22, 2023 10:05

viceice disabled auto-merge February 22, 2023 10:05

viceice changed the title ~~feat: flag to control whether PRs can be edited by maintainers if forkTokenis set~~ feat(platform/github): flag to control whether PRs can be edited by maintainers if forkTokenis set Feb 22, 2023

viceice enabled auto-merge February 22, 2023 10:05

HonkingGoose reviewed Feb 22, 2023

View reviewed changes

HonkingGoose approved these changes Feb 22, 2023

View reviewed changes

rarkins approved these changes Feb 22, 2023

View reviewed changes

viceice added this pull request to the merge queue Feb 22, 2023

Merged via the queue into renovatebot:main with commit 340a913 Feb 22, 2023

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

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

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

devversion commented Jan 10, 2023 •

edited

Loading

viceice left a comment

devversion commented Jan 24, 2023

rarkins left a comment

devversion commented Feb 2, 2023

devversion commented Feb 3, 2023

devversion commented Feb 10, 2023

devversion commented Feb 10, 2023

HonkingGoose left a comment

devversion commented Feb 22, 2023

viceice commented Feb 22, 2023

HonkingGoose commented Feb 22, 2023

HonkingGoose commented Feb 22, 2023

renovate-release commented Feb 22, 2023

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

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

Conversation

devversion commented Jan 10, 2023 • edited Loading

Changes

Context

Documentation (please check one with an [x])

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

viceice left a comment

Choose a reason for hiding this comment

devversion commented Jan 24, 2023

rarkins left a comment

Choose a reason for hiding this comment

devversion commented Feb 2, 2023

devversion commented Feb 3, 2023

devversion commented Feb 10, 2023

devversion commented Feb 10, 2023

HonkingGoose left a comment

Choose a reason for hiding this comment

devversion commented Feb 22, 2023

viceice commented Feb 22, 2023

HonkingGoose commented Feb 22, 2023

HonkingGoose commented Feb 22, 2023

Footnotes

renovate-release commented Feb 22, 2023

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

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

devversion commented Jan 10, 2023 •

edited

Loading