-
Notifications
You must be signed in to change notification settings - Fork 475
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
Danger fails on CI when running on PR from a fork in GH Actions / Buddy Build #1103
Comments
Did that commit exist in the repo? |
@orta yeah the commit exists but it seems like it's trying to look at the wrong remote (origin) instead of my fork. |
Ah cool, might be worth looking that the buddy build part of the codebase and compare it to the env vars that bb shows and see If they need changing |
I'm unsure, I've never used buddy build: maybe there's an API? maybe there's another remote on the repo? Maybe there's another env var? |
Hey! Gonna resurrect this issue because I forgot about it for a while. @orta do you know what the |
I'm having this same issue with github actions - danger can't find the commit from the forked repo because it appears to be looking in the origin. |
Yes, happening for us too on public Open Source repo for Github Actions. Can we somehow work around that?
|
@orta Let me know if you have any idea what to do here - but it could be easy for someone who knows where is Danger looking for wrong remote? |
it looks like danger is not finding the head commit, so the pull from the remote repo seems to be failing somewhere? I can't really remember where all that code is anymore though, so I'm afraid you'll have to do some digging to find the commands it runs and try see why danger can't find the commits. |
@orta I already did today, and I think I'm not sure if I just checked other repos and it looks like all frameworks / repos / projects not only on Github have the same issue with Forks and some of them just removed Danger for open source with many fork PRs... which is sad 😢
I gave up after about two hours... |
I'd be very surprised if all projects using forks are seeing this, this issue has been open for a year and danger is used extensively in open source projects. I'm not sure why it's working in these cases though. The code here looks correct though, both head and base references are requested - and if those commands fail then it should also raise I believe danger/lib/danger/request_sources/github/github.rb Lines 99 to 114 in c51e39c
|
And isn't that exactly the problem?
which means
raises an issue... It seems like danger/lib/danger/scm_source/git_repo.rb Line 108 in c51e39c
Can you point me to any open source repo with Danger and PR from fork that works, please? I'll do my best to find out what's happening... |
This repo has had a few PRs from forks, here's a PR which still has a danger comment: #1207 |
Thanks @orta , that might help! I can see that Circle CI job has setup: Line 24 in 69809e8
Looking at the Circle CI run on given job, I can see that it skipped the first one due to external contributor, and then tried the second run - https://app.circleci.com/pipelines/github/danger/danger/2/workflows/8f64a9de-41c2-43e7-97ad-1e70eb7084cd/jobs/2865 What exactly the |
Travis should be the one running the dangerfile for PRs (need to test many CIs, but don't want it spamming and confusing for others) Though I don't see the status, here's the run: https://travis-ci.org/github/danger/danger/jobs/666482321#L2452 mousey and impatient are options to |
@orta Tried to dig into this. I'm pretty sure this is Danger issue - it's not an issue on Travis, it would be on Circle CI and is on Github Actions. Lately, even Google and Firebase tried to do it, with no luck: firebase/firebase-ios-sdk#5244 Would you mind trying to setup Github Actions for this repo too? We could play with it then (me from fork for example) and find the issue potentially. |
I've got a PR with it running: #1210 - though it's not from a fork |
Looks like what GitHub Actions does is:
Then it uses that as head, so the actual commit from your fork isn't there at all. Which is why danger never finds the HEAD commit from the PR metadata. This is likely because other CIs clone your repo fork, where as these other one's clone the main repo and merge the changes in. There are two valid options from my perspective:
I don't mind either route |
Sounds like the first would take more time as it would be cloning/fetching two repos where the second one would be just using different commit as HEAD, right? Honestly I don't care, but it's great to see this moving, thank you so much! I just wonder - is this issue also an issue for Danger-Swift for example? Or do they use this main danger under the hood? |
Nah, only Danger Ruby would get this - the others all use the GH/GL/BB API and don't need local FS access to git for their info. Your guess is right - it'd have to fetch from two remotes instead of one right now. The latter has a chance for inaccuracy though, it might be a safe assumption that HEAD represents the current tip of the PR, but it might also not. That's why danger is a bit pedantic here and really making sure it knows the full history. |
Let's do what you think is better, or maybe more bulletproof :) |
Here to chime in, we're also seeing this behavior in our repo which uses Github Actions. |
Cool, then I'd recommend someone make a PR which checks the PR data to make sure both remotes are fetched:
|
Github Actions also does not pass secrets to any fork builds, which might be a cause. |
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty.
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty. Signed-off-by: Phil Dibowitz <phil@ipom.com>
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty. Signed-off-by: Phil Dibowitz <phil@ipom.com>
Danger Ruby just full on doesn't work with forked repos. You can see my and other's many attempts here: danger/danger#1103 (comment) However, the JS version does in fact seem to work well. Move to that. The syntax for the Dangerfile is a bit less pretty, and obviously for a ruby project, we'd prefer a Ruby DSL, but working is better than pretty. Signed-off-by: Phil Dibowitz <phil@ipom.com>
We just set up danger in our repository and it fails when running on PRs from forks.
What did you do?
What did you expect to happen?
It would run danger and find the branch & commit in my fork.
What happened instead?
It failed to find the commits and/or the branch:
Your Environment
Buddybuild
Yup
The text was updated successfully, but these errors were encountered: