scream
scream copied to clipboard
E3SM-Project
Non-BFB PR's shouldn't be able to Automerge
Would it be possible to deactivate the AUTOMERGE label if a PR also has the Non-BFB label? Or just deactivate the AUTOMERGE ability?
Typically, we wouldn't expect both labels to be attached to the same PR, but in the edge case where they are and a PR we expect to be non-BFB ends up passing all tests, if it has been approved and has the AUTOMERGE label it will go in. If a PR is labeled non-BFB we would expect it to fail the AT, so all tests passing should be a red-flag that holds up the PR until we figure out whats going on.
An example where this might happen would be
if (some_conditional_we_expect_to_trigger) {
DO SOME NON BFB STUFF
}
if the conditional doesn't actually trigger than all tests may pass and it gets merged. Other users may assume the new change has actually taken effect when in fact it hasn't.
We could modify our Jenkins script to report a fail if all tests pass and non-BFB is on. However, there are different builds, and we don't know if the nonbfb is to be expected everywhere, or only on certain machines, or only for CIME runs,... A few days ago we had a non-BFB pr that passed all AT tests, b/c the non-BFB-ness was only for ne30 (or higher) res, and the AT does not run those.
In short, this is doable, but could report FAIL in cases the non-BFB has a valid reason not to show up. I'll leave it up to others to decide if it's wort adding, but for now I'm adding the wishlist label.
I'm happy to bench this to wishlist as well. But I think it is worth thinking about. I added this issue after the wet/dry MMR PR went in. It was supposed to be non-BFB but it passed the AT and automerged. We probably would have caught that something was up if it passed and was labeled non-BFB. Honestly, I think the simplest approach would be to have the non-BFB label suppress automerge, if possible. Any non-BFB PR would theoretically need a manual merge anyways.
We cannot hack how the AT handles labels. The AT only knows the meaning of 3-4 labels (the green ones starting with AT:), the others are just random strings from its perspective.
What we can do, is make the Jenkins jobs fail if non-BFB is set but all the tests passed. I think that's as far as we can go without hacking the AT implementation.
If one wanted to get to a fine-grain level of control, they'd have to introduce labels like non-BFB-gpu, non-BFB-cpu, non-BFB-CIME, non-BFB-SA, to allow better match of expected non-BFB-ness. But I wouldn't go down this route...
The AT devs will not add anything to their workflow, so we can't ask for a helper feature from them. We could make our testing script return non-zero if non-BFB is on, but the tests pass though, maybe adding a nice message that explains what happened... @jgfouca I think this would be easy, no?