This was probably me, and an accident. I went in to reenable the submodule check and found that none of the checks were marked as required, and I remember being confused about whether that one should be checked or not. sage On Fri, Jun 18, 2021 at 5:03 AM kefu chai <tchaikov@gmail.com> wrote:
On Fri, Jun 18, 2021 at 4:24 PM Ilya Dryomov <idryomov@gmail.com> wrote:
On Fri, Jun 18, 2021 at 6:04 AM kefu chai <tchaikov@gmail.com> 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.
Ilya, thank you for your valuable input. unticked.
if this was an intentional change, let's continue the discussion in this thread.
Thanks,
Ilya
-- Regards Kefu Chai _______________________________________________ Dev mailing list -- dev@ceph.io To unsubscribe send an email to dev-leave@ceph.io