Skip to content

Conversation

@gaearon
Copy link
Collaborator

@gaearon gaearon commented Dec 18, 2017

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we handle the case where there is no build for a particular SHA? One case this could happen is if someone pushes two commits to master at once and there's no build for the first one, but your PR is based off the first one. Probably uncommon but we should at least handle the error gracefully.

Either by doing nothing or trying to go back and try some previous commits.

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 18, 2017

Hmm, we don't want the merge base with master here.

This fails because we force check out PR commit as master on CI (and thus its merge base with master is that commit itself).

We want the branch that the PR was sent against.
I'm not sure how to get that.

@sophiebits
Copy link
Collaborator

maybe CIRCLE_COMPARE_URL?

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 18, 2017

Seems like that's just a URL so we'd have to parse it.

Also not sure what this means:

Not present for builds that are triggered by GitHub pushes.

Aren't all builds triggered by pushes? (I guess this just means it's PR-only)

@sophiebits
Copy link
Collaborator

Yeah, PR vs push.

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 18, 2017

Even CIRCLE_COMPARE_URL isn't helpful 😞
It only shows the last commit, e.g. af469c4...27836c7 for the last run.
Not the actual PR's base.

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 18, 2017

Looks like @orta also bumped into this. danger/danger#45

@orta
Copy link
Contributor

orta commented Dec 18, 2017

Yeah, Circle can be real tricky to gather this kind of information - in Danger Ruby we use the Circle API to grab the PR and then use that to pull out the metadata like base/head.

@zpao
Copy link
Member

zpao commented Dec 19, 2017

One thing worth noting - if we still aren't doing builds for docs, the merge base might be a commit that we don't have a build for.

Otherwise, I made it possible to jump straight to the commit without needing to look at the commit time. Just /builds/branch/_commits/SHA eg: http://react.zpao.com/builds/master/_commits/247c5241706356033cd5bf1dc31197e058853e13/

@gaearon
Copy link
Collaborator Author

gaearon commented Jan 7, 2018

Closing since this is broken.

@gaearon gaearon closed this Jan 7, 2018
@sophiebits
Copy link
Collaborator

@gaearon We can use GitHub's API as @orta described, right? Maybe we can goodfirsttask it if you don't have time to follow up.

@gaearon
Copy link
Collaborator Author

gaearon commented Jan 7, 2018

I think @orta did something in #11865 that fixes this.

@gaearon gaearon deleted the gaearon-patch-4 branch January 7, 2018 18:43
@orta
Copy link
Contributor

orta commented Jan 7, 2018

Yep - that's what it does, uses the PR's base sha 👍 ( sorry for that lack of updates to the PR, I got distracted today for my OSS time with prettier prettier/prettier#3669 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants