fine with me On Fri, Jan 12, 2018 at 4:52 PM Marco de Abreu <[email protected]> wrote:
> 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 > >>> > > > >>> > > >>> > >> > >> > > >
