Skip to content
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

fix: use fallbacks to get PR id for CodeBuild #1079

Merged
merged 3 commits into from Nov 3, 2020
Merged

fix: use fallbacks to get PR id for CodeBuild #1079

merged 3 commits into from Nov 3, 2020

Conversation

alexandermendes
Copy link
Contributor

As mentioned in #1077 CODEBUILD_SOURCE_VERSION seems a little unreliable as a source for the PR number.

This PR keeps that as the default but falls back to CODEBUILD_WEBHOOK_TRIGGER, then to calling the various APIs via getPullRequestIDForBranch(), which fortunately was already there. Still may not be foolproof, but should be an improvement!

@alexandermendes
Copy link
Contributor Author

So, any thoughts on this one?

@orta
Copy link
Member

orta commented Oct 28, 2020

The idea seems good, I was just waiting for it to be green 👍🏻

@alexandermendes
Copy link
Contributor Author

Ok, that's great, thanks. I don't think that failure is anything to do with this PR, although I could be wrong. Looks like it only happens for one of those Travis tasks, and has happened for a few PRs recently, perhaps caused by some async task continuing after a test has ended 🤔 I will see if I can pin it down later.

@alexandermendes
Copy link
Contributor Author

So yeah, it seems like there were some slightly dodgy assertions in some unrelated tests. Those tests are basically asserting this rejection.

As the Node version isn't pinned in Travis when running the tests it probably only recently started using Node 15, which changes the default behaviour to throw on unhandled rejections.

Those Travis tasks are still pending as I write this, but I think they should pass now 🤞

@orta
Copy link
Member

orta commented Nov 3, 2020

Cool! Nice one this look good to me 👍🏻

@orta orta merged commit d8e3cef into danger:master Nov 3, 2020
@alexandermendes alexandermendes deleted the fix-1077 branch November 3, 2020 17:01
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.

None yet

2 participants