backend: add ListPullRequestsWithCommit into Github APIs #1899
Conversation
|
This PR has been marked as stale after 7 or more days of inactivity. Please have a maintainer add the |
mengmichael1
added
area: backend
backend/service/github/github.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| func (s *svc) ListPullRequestsWithCommit(ctx context.Context, ref *RemoteRef, sha string) ([]*PullRequestInfo, error) { |
There was a problem hiding this comment.
i would expand the signature a bit so this could be used to query for more than just open PRs in the future without a breaking signature change.
fake edit: according to https://docs.github.com/en/rest/reference/repos#list-pull-requests-associated-with-a-commit it doesn't accept state as a parameter though, so maybe the comment below is incorrect.
There was a problem hiding this comment.
Good point. Upon looking again, seems like go-github has some contradicting information.
Here (https://github.com/google/go-github/blob/ff33a554ef2c24c1710238b69942fe51e70d89da/github/pulls.go#L121-L123) PullRequestListOptions, which is the opts part of ListPullRequestsWithCommit(ctx context.Context, owner, repo, sha string, opts *githubv3.PullRequestListOptions) states that Default State filter is "open".
However, the method comment here https://github.com/google/go-github/blob/master/github/pulls.go#L168-L194 seems to imply that the default is open and closed.
I'll see if I can find out more.
There was a problem hiding this comment.
Following up with go-github here google/go-github#2228
There was a problem hiding this comment.
Added in a filter for all PRs :)
There was a problem hiding this comment.
I think Daniel was suggesting supporting either PullRequestListOptions in the signature so that callers could specify this.
There was a problem hiding this comment.
ahh I see, I jumped the gun too quickly
done -- added in state string into the signature so the githubv3 PullRequestListOptions struct is abstracted away for callers. wdyt?
There was a problem hiding this comment.
imo the PR list options object is gives the most flexibility for users of the service without having to change / amend the function signature.
There was a problem hiding this comment.
Ah ok fair point. Done -- d36be40
|
Addressed both comments, ready for another pass :) |
|
I'm out for the weekend, back next week, but would really appreciate a review whenever you get the chance @dschaller @danielhochman :) |
backend/service/github/github.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| func (s *svc) ListPullRequestsWithCommit(ctx context.Context, ref *RemoteRef, sha string) ([]*PullRequestInfo, error) { |
There was a problem hiding this comment.
I think Daniel was suggesting supporting either PullRequestListOptions in the signature so that callers could specify this.
|
This PR has been marked as stale after 7 or more days of inactivity. Please have a maintainer add the |
danielhochman
added
on hold
and removed
stale
|
ptal @dschaller @danielhochman |
|
lgtm - You'll need another +1 from Daniel |
Description
Adding the ListPullRequestsWithCommit API into the backend module's available Github APIs.
https://docs.github.com/en/rest/reference/repos#list-pull-requests-associated-with-a-commit
GET /repos/{owner}/{repo}/commits/{commit_sha}/pullsAs part of Lyft's internal extensions for Clutch, we are building a new dashboard to track k8s resources deployed for integrated testing purposes. One of the columns we'd like to surface for our users is the Github PR's branch name, hyperlinked to the Github PR's URL. This allows engineers to have a single-pane-of-glass view over their test deployments of pre-merged Github PR's. The only metadata baked into the k8s artifact is the SHA of the PR, so this API will enable us to translate that into more helpful information (HTML URL, branch name) when engineers are doing their routine maintenance/management.
Testing Performed
Added unit tests. First time contributing so seeking advice on what other testing techniques can be used for this type of change :)
GitHub Issue
n/a
TODOs
n/a at the time of this posting