add more config options for `no_merges` #1704

pitaj · 2023-06-14T02:21:30Z

option for excluding PRs with certain labels from consideration
option for adding labels when merge commits are detected
option for setting a custom comment message for when merge commits are detected

Motivation

Occasionally, a merge commit like rust-lang/rust@cb5c011 makes it past manual review and gets merged into master.

At one point, we tried adding a check to CI to prevent this from happening (rust-lang/rust#105058), but that ended up causing issues and was reverted. This kind of check is simply too fragile for CI, and there must be a way for a human to override the bot's decision.

The capability to detect and warn about merge commits has been present in triagebot for quite some time, but was never enabled at rust-lang/rust, possibly due to concerns about false positives on rollup and subtree/submodule sync PRs. This PR intends to alleviate those concerns.

This will allow us to use the following options at rust-lang/rust:

[no_merges]
exclude_labels = ["rollup", "sync"] # hypothetical label for PRs that sync subtrees or submodules
labels = ["has-merge-commits", "S-waiting-on-author"]

Which will exclude rollup PRs and subtree/submodule sync PRs from merge commit detection, and post the default warning message and add the has-merge-commits and S-waiting-on-author labels when merge commits are detected on all other PRs.

The eventual vision is to have bors refuse to merge if the has-merge-commits label is present. A reviewer can still force the merge by removing that label if they so wish.

Previous discussion

- option for excluding PRs with certain labels - option for setting labels when merge commits are detected - option for setting a custom comment message

tgross35 · 2023-06-30T00:26:21Z

Not something to do in this PR, but it would be kind of cool to have merge readiness levels:

merge-ok: 1-2 commits
merge-likely: 3-5 commits
merge-iffy: 5+ commits
merge-CAREFUL: contains a merge commit

Just to give a quick indicator that a squash may be needed, and tie in with this

Mark-Simulacrum · 2023-07-01T14:57:47Z

Can you say more about the motivation for this? Which PRs are we looking to exclude, how are we planning to use the has-merge-commits label, etc.?

We'll also want to document any features in the Forge (similar to rust-lang/rust-forge#690).

pitaj · 2023-07-01T15:51:16Z

Main motivation is to prevent merge commits like the following from being merged. For now this will just edit labels as a warning but in the future bors might refuse to merge if those labels are present.

rust-lang/rust@cb5c011

I'm just putting this here for now so I can find it later. I'll edit the PR body to include a more detailed description later.

for rust-lang/triagebot#1704

pitaj · 2023-07-04T04:12:30Z

Alright, I've edited the PR body with the more detailed motivation for this. And I opened a PR to rust-forge.

ehuss

Just some minor nits.

src/handlers/no_merges.rs

for rust-lang/triagebot#1704

ehuss

Thanks! I haven't tested, but I believe it should all work. I'm uncertain if there is risk about false positives, but I imagine we can adapt to them if they occur.

for rust-lang/triagebot#1704

Add triagebot no-merges config cc rust-lang/triagebot#1704 rust-lang/rust#114157

Enable triagebot no-merges check Follow-up on rust-lang/triagebot#1704 ### Motivation Occasionally, a merge commit like rust-lang@cb5c011 makes it past manual review and gets merged into master. At one point, we tried adding a check to CI to prevent this from happening (rust-lang#105058), but that ended up [problematic](rust-lang#106319 (comment)) and was [reverted](rust-lang#106320). This kind of check is simply too fragile for CI, and there must be a way for a human to override the bot's decision. The capability to detect and warn about merge commits has been present in triagebot for quite some time, but was never enabled at rust-lang/rust, possibly due to concerns about false positives on rollup and subtree sync PRs. This PR intends to alleviate those concerns. ### Configuration This configuration will exclude rollup PRs and subtree sync PRs from merge commit detection, and it will post the default warning message and add the `has-merge-commits` and `S-waiting-on-author` labels when merge commits are detected on other PRs. The eventual vision is to have bors refuse to merge if the `has-merge-commits` label is present. A reviewer can still force the merge by removing that label if they so wish. ### Note for contributors The rollup tool should add that label automatically, but anyone performing subtree updates should begin including "subtree update" in the titles of those PRs, to avoid false positives. r? infra ## Open Questions 1. This configuration uses the default message that's built into triagebot: > There are merge commits (commits with multiple parents) in your changes. We have a [no merge policy](https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy) so these commits will need to be removed for this pull request to be merged. > > You can start a rebase with the following commands: > ```shell-session > $ # rebase > $ git rebase -i master > $ # delete any merge commits in the editor that appears > $ git push --force-with-lease > ``` Any changes to this are easy, I'll just have to add a `message` option. Should we mention the excluded titles in the message?

Rollup merge of rust-lang#114157 - pitaj:triagebot_no-merges, r=ehuss Enable triagebot no-merges check Follow-up on rust-lang/triagebot#1704 ### Motivation Occasionally, a merge commit like rust-lang@cb5c011 makes it past manual review and gets merged into master. At one point, we tried adding a check to CI to prevent this from happening (rust-lang#105058), but that ended up [problematic](rust-lang#106319 (comment)) and was [reverted](rust-lang#106320). This kind of check is simply too fragile for CI, and there must be a way for a human to override the bot's decision. The capability to detect and warn about merge commits has been present in triagebot for quite some time, but was never enabled at rust-lang/rust, possibly due to concerns about false positives on rollup and subtree sync PRs. This PR intends to alleviate those concerns. ### Configuration This configuration will exclude rollup PRs and subtree sync PRs from merge commit detection, and it will post the default warning message and add the `has-merge-commits` and `S-waiting-on-author` labels when merge commits are detected on other PRs. The eventual vision is to have bors refuse to merge if the `has-merge-commits` label is present. A reviewer can still force the merge by removing that label if they so wish. ### Note for contributors The rollup tool should add that label automatically, but anyone performing subtree updates should begin including "subtree update" in the titles of those PRs, to avoid false positives. r? infra ## Open Questions 1. This configuration uses the default message that's built into triagebot: > There are merge commits (commits with multiple parents) in your changes. We have a [no merge policy](https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy) so these commits will need to be removed for this pull request to be merged. > > You can start a rebase with the following commands: > ```shell-session > $ # rebase > $ git rebase -i master > $ # delete any merge commits in the editor that appears > $ git push --force-with-lease > ``` Any changes to this are easy, I'll just have to add a `message` option. Should we mention the excluded titles in the message?

add more config options for no_merges

ee91f28

- option for excluding PRs with certain labels - option for setting labels when merge commits are detected - option for setting a custom comment message

pitaj force-pushed the no_merges-configs branch from aaf2652 to ee91f28 Compare June 14, 2023 02:27

pitaj added a commit to pitaj/rust-forge that referenced this pull request Jul 4, 2023

add more config options for no_merges

38a0daa

for rust-lang/triagebot#1704

pitaj mentioned this pull request Jul 4, 2023

Document added options for triagebot no_merges rust-lang/rust-forge#691

Merged

ehuss reviewed Jul 11, 2023

View reviewed changes

src/handlers/no_merges.rs Outdated Show resolved Hide resolved

src/handlers/no_merges.rs Show resolved Hide resolved

pitaj added a commit to pitaj/rust-forge that referenced this pull request Jul 14, 2023

add more config options for no_merges

ca56de9

for rust-lang/triagebot#1704

fixes

a1a20a5

ehuss approved these changes Jul 20, 2023

View reviewed changes

ehuss merged commit d18ba48 into rust-lang:master Jul 20, 2023

Mark-Simulacrum pushed a commit to rust-lang/rust-forge that referenced this pull request Jul 22, 2023

add more config options for no_merges

ddb67ae

for rust-lang/triagebot#1704

pitaj mentioned this pull request Jul 28, 2023

Enable triagebot no-merges check rust-lang/rust#114157

Merged

Veykril mentioned this pull request Jul 30, 2023

internal: Add triagebot no-merges config rust-lang/rust-analyzer#15358

Merged

bors added a commit to rust-lang/rust-analyzer that referenced this pull request Aug 1, 2023

Auto merge of #15358 - Veykril:no-merges, r=Veykril

43ecf29

Add triagebot no-merges config cc rust-lang/triagebot#1704 rust-lang/rust#114157

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add more config options for `no_merges` #1704

add more config options for `no_merges` #1704

pitaj commented Jun 14, 2023 •

edited

Loading

tgross35 commented Jun 30, 2023 •

edited

Loading

Mark-Simulacrum commented Jul 1, 2023

pitaj commented Jul 1, 2023 •

edited

Loading

pitaj commented Jul 4, 2023 •

edited

Loading

ehuss left a comment

ehuss left a comment

add more config options for no_merges #1704

add more config options for no_merges #1704

Conversation

pitaj commented Jun 14, 2023 • edited Loading

Motivation

Previous discussion

tgross35 commented Jun 30, 2023 • edited Loading

Mark-Simulacrum commented Jul 1, 2023

pitaj commented Jul 1, 2023 • edited Loading

pitaj commented Jul 4, 2023 • edited Loading

ehuss left a comment

Choose a reason for hiding this comment

ehuss left a comment

Choose a reason for hiding this comment

add more config options for `no_merges` #1704

add more config options for `no_merges` #1704

pitaj commented Jun 14, 2023 •

edited

Loading

tgross35 commented Jun 30, 2023 •

edited

Loading

pitaj commented Jul 1, 2023 •

edited

Loading

pitaj commented Jul 4, 2023 •

edited

Loading