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/92ca1942d67a87ee6a2b4d448c621e433f2f8aca81e4d913d8b2537e@%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
