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

Make danger work with pull_request_target in GH Actions #1501

Closed
wants to merge 1 commit into from

Conversation

tomhughes
Copy link

When trying to fetch base and head for a GitHub pull request make sure to do so from the correct remote instead of assuming that they are both present in the origin remote.

To explain more, when we are running as pull_request_target the checked out repository is the target repository and while it contains the proposed change it is as refs/pull/N/head and not as refs/heads/BRANCH so trying to fetch using the branch name will not work.

This change ensures that we fetch both base and head from the remote that matches the ref as given in the pull request data returned from the API.

I haven't checked but I think the opposite problem may be possible for pull_request if the source and target somehow had base branches that diverged as it will have the source repository as origin but will be looking for a target repository reference. That is probably a lot less likely but this ensures that we fetch both from the correct place anyway.

This should fix #1103.

Copy link
Member

@manicmaniac manicmaniac left a comment

Choose a reason for hiding this comment

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

@tomhughes
Thank you for your contribution! The purpose of this PR looks good. Let me first point out a few styling issues.

lib/danger/request_sources/github/github.rb Outdated Show resolved Hide resolved
lib/danger/request_sources/github/github.rb Outdated Show resolved Hide resolved
lib/danger/scm_source/git_repo.rb Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
When trying to fetch base and head for a GitHub pull request
make sure to do so from the correct remote instead of assuming
that they are both present in the origin remote.

Fixes danger#1103
@manicmaniac
Copy link
Member

@tomhughes Could you fix the failing specs?

  1. Danger::RequestSources::GitHub valid server response branch setup fetches when the branches are not in the local store
    Failure/Error:
    expect do
    @g.fetch_details
    @g.setup_danger_branches
    end.to raise_error(RuntimeError, /Commit (\w+|\b[0-9a-f]{5,40}\b) doesn't exist/)

    expected RuntimeError with message matching /Commit (\w+|\b[0-9a-f]{5,40}\b) doesn't exist/, got #<RSpec::Mocks::MockExpectationError: #Danger::GitRepo:0x00007f66e49ef640 received :exec with unexp...depth=20 --prune https://github.com/artsy/eigen.git +refs/heads/master:refs/remotes/origin/master")> with backtrace:
    # ./lib/danger/scm_source/git_repo.rb:107:in git_fetch_branch_to_depth' # ./lib/danger/scm_source/git_repo.rb:89:in block in ensure_commitish_exists_on_branch!'
    # ./lib/danger/scm_source/git_repo.rb:86:in each' # ./lib/danger/scm_source/git_repo.rb:86:in any?'
    # ./lib/danger/scm_source/git_repo.rb:86:in ensure_commitish_exists_on_branch!' # ./lib/danger/request_sources/github/github.rb:126:in setup_danger_branches'
    # ./spec/lib/danger/request_sources/github_spec.rb:649:in `block (5 levels) in <top (required)>'

@tomhughes
Copy link
Author

Apologies for the delay in looking at the test failures but I got distracted by discovering that this solution doesn't in fact work as well as I was hoping.

Specifically this fails when there are more than 20 commits on the PR branch because it doesn't fetch deeply enough while checking the head references and the later code that tries to establish the full diff isn't passing in the repo URLs and I couldn't see an easy way to do so.

I then came up with https://github.com/openstreetmap/openstreetmap-website/pull/5295/files which gets things working without any changes to danger so I think this is probably no longer useful.

@tomhughes tomhughes closed this Nov 4, 2024
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.

Danger fails on CI when running on PR from a fork in GH Actions / Buddy Build
2 participants