Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions backend/mock/service/githubmock/githubmock.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ func (s svc) ListOrganizations(ctx context.Context, user string) ([]*githubv3.Or
}, nil
}

func (s svc) ListPullRequestsWithCommit(ctx context.Context, ref *github.RemoteRef, sha string) ([]*github.PullRequestInfo, error) {
panic("implement me")
}

func (s svc) GetOrgMembership(ctx context.Context, user, org string) (*githubv3.Membership, error) {
role := "member"
return &githubv3.Membership{Role: &role}, nil
Expand Down
29 changes: 27 additions & 2 deletions backend/service/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ type Client interface {
GetRepository(ctx context.Context, ref *RemoteRef) (*Repository, error)
GetOrganization(ctx context.Context, organization string) (*githubv3.Organization, error)
ListOrganizations(ctx context.Context, user string) ([]*githubv3.Organization, error)
ListPullRequestsWithCommit(ctx context.Context, ref *RemoteRef, sha string) ([]*PullRequestInfo, error)
GetOrgMembership(ctx context.Context, user, org string) (*githubv3.Membership, error)
GetUser(ctx context.Context, username string) (*githubv3.User, error)
}
Expand All @@ -117,8 +118,9 @@ func (s *svc) CreateIssueComment(ctx context.Context, ref *RemoteRef, number int
}

type PullRequestInfo struct {
Number int
HTMLURL string
Number int
HTMLURL string
BranchName string
}

type svc struct {
Expand Down Expand Up @@ -216,6 +218,10 @@ func boolPtr(b bool) *bool {
return &b
}

func intPtr(i int) *int {
return &i
}

func (s *svc) CreatePullRequest(ctx context.Context, ref *RemoteRef, base, title, body string) (*PullRequestInfo, error) {
req := &githubv3.NewPullRequest{
Title: strPtr(title),
Expand All @@ -236,6 +242,25 @@ func (s *svc) CreatePullRequest(ctx context.Context, ref *RemoteRef, base, title
}, nil
}

func (s *svc) ListPullRequestsWithCommit(ctx context.Context, ref *RemoteRef, sha string) ([]*PullRequestInfo, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mengmichael1 mengmichael1 Dec 14, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following up with go-github here google/go-github#2228

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in a filter for all PRs :)

1f45288

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Daniel was suggesting supporting either PullRequestListOptions in the signature so that callers could specify this.

Copy link
Contributor Author

@mengmichael1 mengmichael1 Dec 21, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

imo the PR list options object is gives the most flexibility for users of the service without having to change / amend the function signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok fair point. Done -- d36be40

// PullRequestListOptions left as nil since default opts are sufficient (State: "open", Sort: "created")
respPRs, _, err := s.rest.PullRequests.ListPullRequestsWithCommit(ctx, ref.RepoOwner, ref.RepoName, sha, nil)
if err != nil {
return nil, err
}

prInfos := make([]*PullRequestInfo, len(respPRs))
for i, pr := range respPRs {
prInfos[i] = &PullRequestInfo{
Number: pr.GetNumber(),
HTMLURL: pr.GetHTMLURL(),
BranchName: pr.GetHead().GetRef(),
}
}

return prInfos, nil
}

type CreateBranchRequest struct {
// The base for the new branch.
Ref *RemoteRef
Expand Down
104 changes: 104 additions & 0 deletions backend/service/github/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"io/ioutil"
"net/http"
"strconv"
"testing"
"time"

Expand Down Expand Up @@ -672,3 +673,106 @@ func TestGetRepository(t *testing.T) {
})
}
}

type mockPullRequests struct {
generalError bool

actualNumber int
actualHTMLURL string
actualBranchName string
}

// Dummy mock of Create API so mockPullRequests implements v3pullrequests
func (m *mockPullRequests) Create(ctx context.Context, owner string, repo string, pull *githubv3.NewPullRequest) (*githubv3.PullRequest, *githubv3.Response, error) {
return &githubv3.PullRequest{}, &githubv3.Response{}, nil
}

// Mock of ListPullRequestsWithCommit API
func (m *mockPullRequests) ListPullRequestsWithCommit(ctx context.Context, owner, repo, sha string, opts *githubv3.PullRequestListOptions) ([]*githubv3.PullRequest, *githubv3.Response, error) {
if m.generalError {
return nil, nil, errors.New(problem)
}

m.actualNumber = 1347
m.actualHTMLURL = fmt.Sprintf("https://github.com/%s/%s/pull/%s", owner, repo, strconv.Itoa(m.actualNumber))
m.actualBranchName = "my-branch"

return []*githubv3.PullRequest{
{
Number: intPtr(m.actualNumber),
State: strPtr("open"),
HTMLURL: strPtr(m.actualHTMLURL),
Head: &githubv3.PullRequestBranch{
Ref: strPtr(m.actualBranchName),
SHA: strPtr(sha),
Repo: &githubv3.Repository{
Name: strPtr(repo),
},
User: &githubv3.User{
Login: strPtr("octocat"),
},
},
},
}, nil, nil
}

var listPullRequestsWithCommitTests = []struct {
name string
errorText string
mockPullReq *mockPullRequests
repoOwner string
repoName string
ref string
sha string
}{
{
name: "v3 client error",
mockPullReq: &mockPullRequests{generalError: true},
errorText: "we've had a problem",
repoOwner: "my-org",
repoName: "my-repo",
ref: "my-branch",
sha: "asdf12345",
},
{
name: "happy path",
mockPullReq: &mockPullRequests{},
repoOwner: "my-org",
repoName: "my-repo",
ref: "my-branch",
sha: "asdf12345",
},
}

func TestListPullRequestsWithCommit(t *testing.T) {
for _, tt := range listPullRequestsWithCommitTests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

s := &svc{rest: v3client{
PullRequests: tt.mockPullReq,
}}

resp, err := s.ListPullRequestsWithCommit(
context.Background(),
&RemoteRef{
RepoOwner: tt.repoOwner,
RepoName: tt.repoName,
Ref: tt.ref,
},
tt.sha)

if tt.errorText != "" {
assert.Error(t, err)
assert.Contains(t, err.Error(), tt.errorText)
} else {
assert.NoError(t, err)
assert.Equal(t, 1, len(resp))
assert.Equal(t, tt.mockPullReq.actualNumber, resp[0].Number)
assert.Equal(t, tt.mockPullReq.actualHTMLURL, resp[0].HTMLURL)
assert.Equal(t, tt.mockPullReq.actualBranchName, resp[0].BranchName)
}
})
}
}
2 changes: 2 additions & 0 deletions backend/service/github/iface.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ type v3repositories interface {
type v3pullrequests interface {
// Create a new pull request on the specified repository.
Create(ctx context.Context, owner string, repo string, pull *githubv3.NewPullRequest) (*githubv3.PullRequest, *githubv3.Response, error)
// ListPullRequestsWithCommit returns pull requests associated with a commit SHA.
ListPullRequestsWithCommit(ctx context.Context, owner, repo, sha string, opts *githubv3.PullRequestListOptions) ([]*githubv3.PullRequest, *githubv3.Response, error)
}

type v4client interface {
Expand Down