Considering we're approaching the weekend, I'll switch PR-head off now. Also, I will retrigger all PRs in order to have a clean state below all PRs. If anybody objects afterwards, we can just flip the switch to re-enable these builds.
-Marco On Fri, Jan 12, 2018 at 7:57 PM, Marco de Abreu < [email protected]> wrote: > Hello, > > the protected master branch has successfully been switched to PR-merge > https://issues.apache.org/jira/browse/INFRA-15833. In the next step, I'd > like to remove PR-head from our CI. This means that in future only PR-merge > will be executed. > > Does anybody object? > > -Marco > > On Wed, Jan 10, 2018 at 9:40 PM, Marco de Abreu < > [email protected]> wrote: > >> 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 >>> > > >>> > >>> >> >> >
