Thanks for your opinions. Could a committer please contact a mentor in order to create an Apache Infra ticket to change the protected master branch from PR-head to PR-merge?
-Marco On Wed, Jan 10, 2018 at 9:26 PM, kellen sunderland < [email protected]> wrote: > +1 > > On Wed, Jan 10, 2018 at 6:51 PM, Gautam <[email protected]> wrote: > > > +1 > > > > On Jan 10, 2018 1:25 AM, "Marco de Abreu" <[email protected]> > > wrote: > > > > > Hello, > > > > > > TLDR: We wish to change how PRs are validated, turning off PR-head > which > > > tests PRs in their current branch, and turning on PR-merge, which tests > > PRs > > > rebased on the current master branch. We believe this will catch more > > > potential errors that would otherwise get merged into master, and it > > should > > > not cause any extra work for commiters or reviewers. > > > > > > as announced in > > > https://lists.apache.org/thread.html/92ca1942d67a87ee6a2b4d448c621e > > > 433f2f8aca81e4d913d8b2537e@%3Cdev.mxnet.apache.org%3E > > > and as probably most have noticed, we have been running an experiment > > with > > > the PR-validation-jobs. During the past month, every PR was checked by > > the > > > jobs called PR-head and PR-merge. In the past, only PR-head has been > > > executed and was the required job to pass in order to merge a PR into > the > > > protected master branch. Before I continue any further, I’d like to > > explain > > > the detailed meaning of both jobs: > > > > > > PR-head: The PR and its commit history is taken as-is and tested in > > exactly > > > the same state as in your local fork. > > > > > > PR-merge: The PR and its commit history are rebased on top of latest > > master > > > commit and thus tested as if the PR would be merged at this point in > > time. > > > > > > I have noticed that many PRs are rarely rebased before a merge. > > Considering > > > the fast development of MXNet, this could cause serious issues: > Imagine a > > > PR is based on a 4 weeks old commit and accesses an API which has been > > > modified in the meantime. PR-head would report this PR as ready to > merge > > as > > > the changes, based on the 4 weeks old commit. But as soon as a > committer > > > merges this PR into the master branch, the master branch will suddenly > > > report errors because this PR tries to access an API which does not > exist > > > anymore. > > > > > > Using PR-merge will reduce the chance of this happening as the PR is > > always > > > getting rebased on top of the master branch before it is getting > > validated. > > > But there is one pitfall: CI only runs if a new commit is getting > pushed. > > > If a PR stays untouched for a certain amount of time it still could be > > > possible that it missed a breaking change due to the fact that CI > hasn’t > > > been triggered for a while, but this happens quite rarely. In order to > > > solve this problem, we could think about introducing a job which > > validates > > > PRs that haven’t been run for a week, but that’s a different > discussion. > > > Also, if multiple PRs get merged at the same time, conflicting changes > > (in > > > terms of changes in one part which cause another part to fail) could be > > > introduced – but the committers who merge the PRs usually notice two > > > conflicting PRs. Additionally, merge conflicts in terms of changing the > > > same lines of code on the other hand will fail fast and tell the > > > contributor in the GitHub-webinterface that they will have to resolve > the > > > merge-conflicts before the PR can be validated – it couldn’t be merged > > with > > > merge-conflicts anyways. > > > > > > PR-merge is a safer choice in terms of health for the master-branch. > > Thus, > > > I’d like to put it up for discussion to turn off PR-head and switch the > > > required check to PR-merge. > > > > > > Does anybody object? > > > > > > Best regards, > > > Marco > > > > > >
