On Fri, Jun 18, 2021 at 6:04 AM kefu chai <tchaikov@xxxxxxxxx> wrote:
>
> hi folks,
>
> "This branch is out-of-date with the base branch" this message started
> to show up in the github PR web pages recently. and next to it is an
> "Update branch" button.
>
> i think the reason is that some of us change the per-branch "Branch
> protection rule" of "master" branch of ceph project in github. there
> is an option named "Require branches to be up to date before merging",
> if it is enabled, per the description of this policy:
>
> > This ensures pull requests targeting a matching branch have been tested with the latest code. This setting will not take effect unless at least one status check is enabled (see below).
>
> in other words,
>
> 1. it's required to pass some status checks to merge a PR
> 2. if another change is merged after all the status checks passed in
> your PR, these status checks results are invalidated, and you are
> supposed to rebase your change against master. and the repush
> re-triggers the status checks. the new result is used
> as a prerequisite of merging the PR instead of the old one.
>
> i see there is a good reason to enable the option. as it helps to
> prevent us from merging conflicting changes whose status checks pass
> individually. but these changes could break build or test if they are
> tested together.
>
> but the downside is:
>
> - developers are tempted to push the "Update branch" button next to
> the warning message. this helps the change to comply the "up to date
> before merging" policy, but it breaks our policy requiring a PR to
> avoid including any merge commit in it.
> because it introduces a merge commit into the PR in question.
> - the extra overhead of rebasing and repushing dance.
>
> is it a plausible alternative to require the maintainer who pushes the
> merge button to retrigger the test if the change to be merged is kind
> of old? i know, it's difficult to measure "old", and it's difficult to
> enforce a policy like this instead of leaving it to a system.. but
> it's more sustainable this way, i think.
>
> BTW, i think, the "stale test result of a snapshot" issue also applies
> to the integration test, a.k.a., qa suite test.
Yes, it is not only plausible but actually the only way to go in my
option. "Require branches to be up to date before merging" is pretty
useless for us precisely because the bulk of our testing is integration
testing and that is a manual process completely up to the maintainer.
If a "make check" test gets accidentally broken by another merge, it is
noticed and fixed up almost immediately. If an integration test gets
broken, it can take a couple days... The way I see it, the only thing
"Require branches to be up to date before merging" is going to get us
is having to chase down and ask contributors to get rid of generated
merge commits.
I suspect it was enabled by mistake because if you accidentally remove
the status checks and then go re-enable them, "Require branches to be up
to date before merging" gets set automatically and must be ticked off.
Thanks,
Ilya
_______________________________________________
Dev mailing list -- dev@xxxxxxx
To unsubscribe send an email to dev-leave@xxxxxxx
