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(a)gmail.com> wrote: > > On Fri, Jun 18, 2021 at 4:24 PM Ilya Dryomov <idryomov(a)gmail.com> wrote: > > > > On Fri, Jun 18, 2021 at 6:04 AM kefu chai <tchaikov(a)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(a)ceph.io > To unsubscribe send an email to dev-leave(a)ceph.io