[MRG+1] Use GitHub's merge refs to test PRs on CircleCI #8211
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Unlike other CIs, CircleCI does not test the merge commit of a PR with its base branch. Instead it tests the PR's head commit. The problem with this is the PR's status could differ when it is merged with the base branch. For instance, if one needs changes in the base branch to get their PR to pass, they must rebase/merge to get them into the history the PR. This normally isn't a problem, but sometimes things do go wrong when doing this merge/rebase, which adds a new unneeded difficulty. Alternatively, a passing PR could turn out to fail when merged into the base branch because some new content in the base branch was not tested against in the PR. To solve these issues, we checkout the merge ref for a PR (if it is a PR) from GitHub. However, it should be noted that the merge ref can be out-of-date in some cases w.r.t. the base branch. Still this is the commonly used strategy on Travis CI and AppVeyor. If we had enough info, we could ideally terminate a build that has merge conflicts. Unfortunately it doesn't seem that CircleCI gives us this info.
d99e388 to
adc2b67
Compare
|
LGTM. I cursorily checked that this should not conflict with our |
|
Thanks a lot @jakirkham... In it goes... (Merging without confirmation from appveyor as it should not be affected in anyway...) |
|
Glad to help. 😄 Wasn't sure if this was |
|
We've become very lax about our commit messages...
…On 18 January 2017 at 13:02, jakirkham ***@***.***> wrote:
Glad to help. 😄
Wasn't sure if this was BLD or MNT. So sorry if I messed that up.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8211 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz6zuC-aB9I76RnZwgmwWo6ws9N32Qks5rTXLGgaJpZM4LmWKp>
.
|
|
Out of interest, has anyone tried to see what happens if you add CircleCI on your fork? This is sometimes convenient to test things out without having to open a PR. |
I guess this works fine because CIRCLE_PR_NUMBER is unset. |
raghavrv
pushed a commit
to raghavrv/scikit-learn
that referenced
this pull request
Jan 18, 2017
…#8211) Unlike other CIs, CircleCI does not test the merge commit of a PR with its base branch. Instead it tests the PR's head commit. The problem with this is the PR's status could differ when it is merged with the base branch. For instance, if one needs changes in the base branch to get their PR to pass, they must rebase/merge to get them into the history the PR. This normally isn't a problem, but sometimes things do go wrong when doing this merge/rebase, which adds a new unneeded difficulty. Alternatively, a passing PR could turn out to fail when merged into the base branch because some new content in the base branch was not tested against in the PR. To solve these issues, we checkout the merge ref for a PR (if it is a PR) from GitHub. However, it should be noted that the merge ref can be out-of-date in some cases w.r.t. the base branch. Still this is the commonly used strategy on Travis CI and AppVeyor. If we had enough info, we could ideally terminate a build that has merge conflicts. Unfortunately it doesn't seem that CircleCI gives us this info.
|
Thanks @jakirkham :) |
Yes, we have tried this. As you have noted, the guards checking That said, there is an important caveat. CircleCI's build model is different from other CIs like Travis and AppVeyor. Namely CircleCI views all builds as equivalent regardless of whether they are on your fork or in a PR. This differs from Travis and AppVeyor, which view these as two separate builds (one at the branch tip and the other at the merge of the PR). Thus if one enables CircleCI on their fork, then the build will only occur on user's fork. As a result this script won't do anything in that case. Just something to keep in mind. |
That's what I thought, but thanks a lot spelling it out and confirming my suspicions. |
sergeyf
pushed a commit
to sergeyf/scikit-learn
that referenced
this pull request
Feb 28, 2017
<
8000
a class="avatar avatar-user" style="width:20px;height:20px;" data-test-selector="commits-avatar-stack-avatar-link" data-hovercard-type="user" data-hovercard-url="/users/sergeyf/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="/sergeyf">
…#8211) Unlike other CIs, CircleCI does not test the merge commit of a PR with its base branch. Instead it tests the PR's head commit. The problem with this is the PR's status could differ when it is merged with the base branch. For instance, if one needs changes in the base branch to get their PR to pass, they must rebase/merge to get them into the history the PR. This normally isn't a problem, but sometimes things do go wrong when doing this merge/rebase, which adds a new unneeded difficulty. Alternatively, a passing PR could turn out to fail when merged into the base branch because some new content in the base branch was not tested against in the PR. To solve these issues, we checkout the merge ref for a PR (if it is a PR) from GitHub. However, it should be noted that the merge ref can be out-of-date in some cases w.r.t. the base branch. Still this is the commonly used strategy on Travis CI and AppVeyor. If we had enough info, we could ideally terminate a build that has merge conflicts. Unfortunately it doesn't seem that CircleCI gives us this info.
Closed
Sundrique
pushed a commit
to Sundrique/scikit-learn
that referenced
this pull request
Jun 14, 2017
…#8211) Unlike other CIs, CircleCI does not test the merge commit of a PR with its base branch. Instead it tests the PR's head commit. The problem with this is the PR's status could differ when it is merged with the base branch. For instance, if one needs changes in the base branch to get their PR to pass, they must rebase/merge to get them into the history the PR. This normally isn't a problem, but sometimes things do go wrong when doing this merge/rebase, which adds a new unneeded difficulty. Alternatively, a passing PR could turn out to fail when merged into the base branch because some new content in the base branch was not tested against in the PR. To solve these issues, we checkout the merge ref for a PR (if it is a PR) from GitHub. However, it should be noted that the merge ref can be out-of-date in some cases w.r.t. the base branch. Still this is the commonly used strategy on Travis CI and AppVeyor. If we had enough info, we could ideally terminate a build that has merge conflicts. Unfortunately it doesn't seem that CircleCI gives us this info.
NelleV
pushed a commit
to NelleV/scikit-learn
that referenced
this pull request
Aug 11, 2017
…#8211) Unlike other CIs, CircleCI does not test the merge commit of a PR with its base branch. Instead it tests the PR's head commit. The problem with this is the PR's status could differ when it is merged with the base branch. For instance, if one needs changes in the base branch to get their PR to pass, they must rebase/merge to get them into the history the PR. This normally isn't a problem, but sometimes things do go wrong when doing this merge/rebase, which adds a new unneeded difficulty. Alternatively, a passing PR could turn out to fail when merged into the base branch because some new content in the base branch was not tested against in the PR. To solve these issues, we checkout the merge ref for a PR (if it is a PR) from GitHub. However, it should be noted that the merge ref can be out-of-date in some cases w.r.t. the base branch. Still this is the commonly used strategy on Travis CI and AppVeyor. If we had enough info, we could ideally terminate a build that has merge conflicts. Unfortunately it doesn't seem that CircleCI gives us this info.
paulha
pushed a commit
to paulha/scikit-learn
that referenced
this pull request
Aug 19, 2017
…#8211) Unlike other CIs, CircleCI does not test the merge commit of a PR with its base branch. Instead it tests the PR's head commit. The problem with this is the PR's status could differ when it is merged with the base branch. For instance, if one needs changes in the base branch to get their PR to pass, they must rebase/merge to get them into the history the PR. This normally isn't a problem, but sometimes things do go wrong when doing this merge/rebase, which adds a new unneeded difficulty. Alternatively, a passing PR could turn out to fail when merged into the base branch because some new content in the base branch was not tested against in the PR. To solve these issues, we checkout the merge ref for a PR (if it is a PR) from GitHub. However, it should be noted that the merge ref can be out-of-date in some cases w.r.t. the base branch. Still this is the commonly used strategy on Travis CI and AppVeyor. If we had enough info, we could ideally terminate a build that has merge conflicts. Unfortunately it doesn't seem that CircleCI gives us this info.
maskani-moh
pushed a commit
to maskani-moh/scikit-learn
that referenced
this pull request
Nov 15, 2017
…#8211) Unlike other CIs, CircleCI does not test the merge commit of a PR with its base branch. Instead it tests the PR's head commit. The problem with this is the PR's status could differ when it is merged with the base branch. For instance, if one needs changes in the base branch to get their PR to pass, they must rebase/merge to get them into the history the PR. This normally isn't a problem, but sometimes things do go wrong when doing this merge/rebase, which adds a new unneeded difficulty. Alternatively, a passing PR could turn out to fail when merged into the base branch because some new content in the base branch was not tested against in the PR. To solve these issues, we checkout the merge ref for a PR (if it is a PR) from GitHub. However, it should be noted that the merge ref can be out-of-date in some cases w.r.t. the base branch. Still this is the commonly used strategy on Travis CI and AppVeyor. If we had enough info, we could ideally terminate a build that has merge conflicts. Unfortunately it doesn't seem that CircleCI gives us this info.
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8058
Unlike other CIs, CircleCI does not test the merge commit of a PR with its base branch. Instead it tests the PR's head commit. The problem with this is the PR's status could differ when it is merged with the base branch. For instance, if one needs changes in the base branch to get their PR to pass, they must rebase/merge to get them into the history the PR. This normally isn't a problem, but sometimes things do go wrong when doing this merge/rebase, which adds a new unneeded difficulty. Alternatively, a passing PR could turn out to fail when merged into the base branch because some new content in the base branch was not tested against in the PR.
To solve these issues, we checkout the merge ref for a PR (if it is a PR) from GitHub. However, it should be noted that the merge ref can be out-of-date in some cases w.r.t. the base branch. Still this is the commonly used strategy on Travis CI and AppVeyor. If we had enough info, we could ideally terminate a build that has merge conflicts. Unfortunately it doesn't seem that CircleCI gives us this info.
cc @jnothman