mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marco de Abreu <marco.g.ab...@googlemail.com>
Subject Re: Switch PR validation to PR-merge
Date Fri, 12 Jan 2018 18:57:04 GMT
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 <
marco.g.abreu@googlemail.com> 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 <
> kellen.sunderland@gmail.com> wrote:
>
>> +1
>>
>> On Wed, Jan 10, 2018 at 6:51 PM, Gautam <gautamnitc@gmail.com> wrote:
>>
>> > +1
>> >
>> > On Jan 10, 2018 1:25 AM, "Marco de Abreu" <marco.g.abreu@googlemail.com
>> >
>> > 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
>> > >
>> >
>>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message