search

rust-lang / triagebot

Automation/tooling for Rust spaces
https://triage.rust-lang.org
Apache License 2.0
170 stars 75 forks source link

add more config options for `no_merges` #1704

Closed pitaj closed 12 months ago

pitaj commented 1 year ago

rust-forge PR: https://github.com/rust-lang/rust-forge/pull/691

Motivation

Occasionally, a merge commit like rust-lang/rust@cb5c011670ce8d073d0aae8c45e73c20593bfa11 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

tgross35 commented 1 year ago

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

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

Mark-Simulacrum commented 1 year ago

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 https://github.com/rust-lang/rust-forge/pull/690).

pitaj commented 1 year ago

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.

https://github.com/rust-lang/rust/commit/cb5c011670ce8d073d0aae8c45e73c20593bfa11

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.

pitaj commented 1 year ago

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