Deny merge in CI take 2 #106333
Deny merge in CI take 2 #106333
Conversation
This will prevent users with the pre-push hook from pushing a merge commit. Exceptions are added for subtree updates. These exceptions are a little hacky and may be non-exhaustive but can be extended in the future. Checkout `master` branch in CI Add `build_helper` crate to share code between tidy and bootstrap
|
r? @jyn514 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
T-bootstrap
|
@bors try |
|
⌛ Trying commit a677f5b57934bdc495cb514429f4f7f536a7cd6b with merge 4567d33aa10280ffe6ffe1633d718320a14f33e1... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-author
Noratrieb
force-pushed
the
deny-merge-in-ci-take-2
branch
from
a677f5b
to
fcc7c55
Compare
|
@bors try |
|
⌛ Trying commit fcc7c55 with merge 45c789a001deef1184c065548f06295984523f33... |
|
☀️ Try build successful - checks-actions |
|
@bors try |
|
⌛ Trying commit 5705d73 with merge 1b281d09bc758176d60aeb66ffcd4fb5f97697ce... |
|
I think this may not work if the contributor adds any commits after the merge commit, since the fetch depth is only 2. That might be unlikely, but something to be aware of. |
|
☀️ Try build successful - checks-actions |
False negatives are ok, at worst it means a reviewer needs to tell the author to get rid of the merge, like today. False positives are worse since tidy gives a hard error. That said we can increase the fetch depth if we need to, I expect ~20 commits will be enough in practice. But let's avoid doing that until we figure out why this broke when it merged last time. |
|
It's probably worth seeing if enabling GitHub's "require linear history" would give a visual warning here. I think it is maybe true that bors can be setup to bypass that, in which case we might get something relatively reasonable. |
|
@Mark-Simulacrum I think you and Pietro are the only ones with access to those settings - do you have time to look into it? I can see about trying this on a personal repo if not so I can link you the exact setting. We should also make sure that only shows up for merge commits and not when the merge-base for the PR is based on a old commit of master. |
|
I can, yes. I'll need a chunk of time around a bors merge so we can fix bors if it goes wrong; it might be best to do it on some other repo (e.g., crater?) that also uses bors. I can try to make that happen soon. |
|
It looks like getting bors to bypass that might be trickier than expected -- so let's assume that strategy won't work for now. |
This comment has been minimized.
This comment has been minimized.
|
gonna close this, either someone else picks it up or i may continue at some point |
Taking up #105058 which was reverted in #106320 because it caused build failures in bors builds.
https://github.com/rust-lang-ci/rust/actions/runs/3810081096/jobs/6481927643#logs