fix getting pull requests that come from forks #563
Conversation
|
Summarizing from our internal discussion, I think it would be good explore ways we could use this more expensive listing method only when required. That might be:
|
pull/pull_requests.go
Outdated
| return nil, errors.Wrapf(err, "failed to list pull requests for repository %s/%s", owner, repoName) | ||
| } | ||
| // GitHubClient is an interface that wraps the methods used from the github.Client. | ||
| type GitHubClient interface { |
There was a problem hiding this comment.
nit: I'd call this GitHubPullRequestClient, since it is only satisfied by the PullRequestsService from go-github
pull/pull_requests.go
Outdated
|
|
||
| if len(prs) == 0 { | ||
| logger.Debug().Msg("No pull requests associated with the check run, searching by SHA") |
There was a problem hiding this comment.
nit: Let's not mention check runs here specifically, since this could be used for other things too
pull/pull_requests.go
Outdated
|
|
||
| // GetAllPossibleOpenPullRequestsForSHA attempts to find all open pull requests | ||
| // associated with the given SHA using multiple methods in case we are dealing with a fork | ||
| func GetAllPossibleOpenPullRequestsForSHA(ctx context.Context, client GitHubClient, owner, repo, sha string) ([]*github.PullRequest, error) { |
There was a problem hiding this comment.
While the comments help, I'm wondering if we can make the differences between these functions more obvious by name. From the usage, it looks like GetAllPossibleOpenPullRequestsForSHA, ListOpenPullRequestsForSHA, and ListOpenPullRequestsForRef are the only functions that need to be public, so making the remaining ones private for now would help.
Beyond that, I think we should try to make it obvious which functions you should call by default and which are special cases:
- I think
GetAllPossibleOpenPullRequestsForSHAandListOpenPullRequestsForRefare the two "by default" methods, so they should have similar names. Maybe use theGetpattern for both of these? - I think
ListOpenPullRequestsForSHAis the special method that you only call directly when you know you are working with a fork and want to skip a request by not calling the "default" method. Maybe call thisListAllOpenPullRequestsFilteredBySHA? That might be enough to imply that there's something special about this compared toGetOpenPullRequestsForSHA. Open to other naming ideas too.
There was a problem hiding this comment.
Agree with what you are saying about the function names being a bit confusing. Followed your advice to make the two main functions start with Get and changed the name of the List function to make it clear that it is filtering all pull requests
server/handler/check_run.go
Outdated
| logger.Debug().Msg("Doing nothing since status change event affects no open pull requests") | ||
| return nil | ||
| logger.Debug().Msg("No pull requests associated with the check run, searching by SHA") | ||
| // if no PR's were attached with the event let's check with Github in case it is a fork |
There was a problem hiding this comment.
It might be good to clarify here that check runs for fork PRs will always have an empty pull request list. I think the other case is check runs on branches that aren't part of any PR?
pull/pull_requests.go
Outdated
| if len(prs) == 0 { | ||
| logger.Debug().Msg("No open pull requests found for the given SHA") | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
I think we can probably drop this and return prs on L115 - it looks like the callers already log something when there are no PRs, so this feels redundant.
While switching the github api that we use to get pull requests given a commit sha we noticed it was not retrieving pull requests from forks. This PR includes a change to revert back to the old API that also lists PRs from forks. During investigation I also noticed the check_run event also did not list the pull requests if it came from a fork, so this PR also includes a change to use the list all commits given a commit like how it is done for the status event.