search

pytorch / test-infra

This repository hosts code that supports the testing infrastructure for the PyTorch organization. For example, this repo hosts the logic to track disabled tests and slow tests, as well as our continuation integration jobs HUD/dashboard.
https://hud.pytorch.org/
Other
81 stars 77 forks source link

Leave a comment if someone tries to force merges when all checks are passing anyways #4491

Open ZainRizvi opened 1 year ago

ZainRizvi commented 1 year ago

Goal: Remove people's muscle memory that's getting them to unnecessarily use force merges

If people use a force merge when CI checks are all already green, fail the merge with the complaint that the force merge is not needed and they should use a normal merge instead.

So far, I've noticed people using force merges when Dr. CI had flagged a failure as unrelated. It seems like people don't realize mergebot would let the PR get merged anyways, so this can be our tool for educating them.

Finally, as a safety fallback in case a force merge really becomes necessary, lets add an extra parameter (like --trust-me) to the force merge command to get back the original forcing behavior. We wouldn't expect anyone outside of dev-infra to ever use this one.

The failure message should explain that normal merges will succeed even if Dr. CI calls out unrelated failures.

huydhn commented 1 year ago

One use case requires this is to force merge old PR, but this check has been removed because the field from GitHub has been deprecated (and become None)

huydhn commented 1 year ago

May be we could fallback to regular merge instead of a force merge. However, we also to raise the awareness of when to use force merge.

Eli: we could do this in stages, with a deprecation date with fallback option before that date.

Stage 1: Don't block force merge, but has a fallback flag ready. Tell people that they shouldn't use force merge State 2: Block no-op force merges

malfet commented 1 year ago

IMO we should never abort force merges: if PR has the right approvals then merge could be forced. Also, this could result in an outage if HUD/RockSet is down for some reason. I.e. trymerge design should not make minimal amount of API calls when force merge is requested to reduce the possibility that force merge fails due to the infrastructure issues.