Skip to content

Find commit by PR when polling Travis builds #21

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 12, 2016
Merged

Conversation

phillipj
Copy link
Member

We've been struggling with PR builds being triggered with commit SHA we have no reference of - seems like some kind of meta merge commit made by GitHub. Therefore our effort of matching builds by the actual commit SHA from PRs doesn't always work.

There's a trivial change included here in 05af2ed which also tries to match commits by their related PR. Fingers crossed this is what we need.

  • reverting the revert (deleting the commenting code)

P.S. this is currently deployed to production.

@@ -129,7 +63,9 @@ function pollTravisBuildBySha (options, checkNumber) {
return console.error(`! ${prInfo} Got error when retrieving Travis builds`, err.stack)
}

const matchingCommit = res.commits.find((commit) => commit.sha === shaToMatch)
const matchingCommit = res.commits.find((commit) => {
return commit.sha === shaToMatch || commit.pull_request_number === prToMatch

This comment was marked as off-topic.

This comment was marked as off-topic.

@phillipj
Copy link
Member Author

Tested with a branch from the main repo nodejs/nodejs.org#490. We need a test PR from a nodejs.org fork as well.

@Fishrock123
Copy link
Contributor

See nodejs/nodejs.org#647 (comment) -- this sometimes incorrectly gets outdated builds

@Fishrock123
Copy link
Contributor

Hmm, looks like it's working now. Strange.

@Fishrock123
Copy link
Contributor

@phillipj might as well merge for now?

@phillipj
Copy link
Member Author

@Fishrock123 sure enough

@phillipj phillipj merged commit 81ee8ce into master Apr 12, 2016
@phillipj phillipj deleted the find-commit-by-pr branch April 12, 2016 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants