Skip to content

Commit

Permalink
Do not set CreateCheckRunOptions.HeadBranch
Browse files Browse the repository at this point in the history 
It seems it's not required anymore and it's not referred from the
document. https://developer.github.com/v3/checks/runs/#create-a-check-run
  • Loading branch information
@haya14busa
haya14busa committed Sep 28, 2019
1 parent 72e9bb1 commit d70c5a9
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 175 deletions.
47 changes: 5 additions & 42 deletions doghouse/server/doghouse.go
View file
Expand Up @@ -12,7 +12,6 @@ import (
"github.com/reviewdog/reviewdog"
"github.com/reviewdog/reviewdog/diff"
"github.com/reviewdog/reviewdog/doghouse"
"golang.org/x/sync/errgroup"
)

// GitHub check runs API cannot handle too large requests.
Expand All @@ -33,38 +32,14 @@ func NewChecker(req *doghouse.CheckRequest, gh *github.Client) *Checker {
}

func (ch *Checker) Check(ctx context.Context) (*doghouse.CheckResponse, error) {
var branch string
var filediffs []*diff.FileDiff

// Get branch from PullRequest API and PullRequest diff from diff API
// concurrently.
eg, ctx4eg := errgroup.WithContext(ctx)
eg.Go(func() error {
br, err := ch.getBranch(ctx4eg)
if err != nil {
return err
}
if br == "" {
return fmt.Errorf("failed to get branch")
}
branch = br
return nil
})
eg.Go(func() error {
fd, err := ch.diff(ctx4eg)
if err != nil {
return fmt.Errorf("fail to parse diff: %v", err)
}
filediffs = fd
return nil
})
if err := eg.Wait(); err != nil {
return nil, fmt.Errorf("failed to get branch/diff: %v", err)
filediffs, err := ch.diff(ctx)
if err != nil {
return nil, fmt.Errorf("fail to parse diff: %v", err)
}

results := annotationsToCheckResults(ch.req.Annotations)
filtered := reviewdog.FilterCheck(results, filediffs, 1, "")
checkRun, err := ch.postCheck(ctx, branch, filtered)
checkRun, err := ch.postCheck(ctx, filtered)
if err != nil {
// If this error is StatusForbidden (403) here, it means reviewdog is
// running on GitHub Actions and has only read permission (because it's
Expand All @@ -83,18 +58,7 @@ func (ch *Checker) Check(ctx context.Context) (*doghouse.CheckResponse, error) {
return res, nil
}

func (ch *Checker) getBranch(ctx context.Context) (string, error) {
if ch.req.Branch != "" {
return ch.req.Branch, nil
}
pr, err := ch.gh.GetPullRequest(ctx, ch.req.Owner, ch.req.Repo, ch.req.PullRequest)
if err != nil {
return "", fmt.Errorf("failed to get pr: %v", err)
}
return pr.GetHead().GetRef(), nil
}

func (ch *Checker) postCheck(ctx context.Context, branch string, checks []*reviewdog.FilteredCheck) (*github.CheckRun, error) {
func (ch *Checker) postCheck(ctx context.Context, checks []*reviewdog.FilteredCheck) (*github.CheckRun, error) {
var annotations []*github.CheckRunAnnotation
for _, c := range checks {
if !c.InDiff {
Expand All @@ -115,7 +79,6 @@ func (ch *Checker) postCheck(ctx context.Context, branch string, checks []*revie
opt := github.CreateCheckRunOptions{
Name: name,
ExternalID: github.String(name),
HeadBranch: branch,
HeadSHA: ch.req.SHA,
Status: github.String("completed"),
Conclusion: github.String(conclusion),
Expand Down
133 changes: 0 additions & 133 deletions doghouse/server/doghouse_test.go
View file
Expand Up @@ -13,15 +13,10 @@ import (

type fakeCheckerGitHubCli struct {
checkerGitHubClientInterface
FakeGetPullRequest func(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error)
FakeGetPullRequestDiff func(ctx context.Context, owner, repo string, number int) ([]byte, error)
FakeCreateCheckRun func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error)
}

func (f *fakeCheckerGitHubCli) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) {
return f.FakeGetPullRequest(ctx, owner, repo, number)
}

func (f *fakeCheckerGitHubCli) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) ([]byte, error) {
return f.FakeGetPullRequestDiff(ctx, owner, repo, number)
}
Expand Down Expand Up @@ -59,7 +54,6 @@ func TestCheck_OK(t *testing.T) {
prNum = 14
sha = "1414"
reportURL = "http://example.com/report_url"
branch = "test-branch"
conclusion = "neutral"
)

Expand Down Expand Up @@ -87,26 +81,13 @@ func TestCheck_OK(t *testing.T) {
}

cli := &fakeCheckerGitHubCli{}
cli.FakeGetPullRequest = func(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) {
if number != prNum {
t.Errorf("PullRequest number = %d, want %d", number, prNum)
}
return &github.PullRequest{
Head: &github.PullRequestBranch{
Ref: github.String(branch),
},
}, nil
}
cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
return []byte(sampleDiff), nil
}
cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) {
if opt.Name != name {
t.Errorf("CreateCheckRunOptions.Name = %q, want %q", opt.Name, name)
}
if opt.HeadBranch != branch {
t.Errorf("CreateCheckRunOptions.HeadBranch = %q, want %q", opt.HeadBranch, branch)
}
if opt.HeadSHA != sha {
t.Errorf("CreateCheckRunOptions.HeadSHA = %q, want %q", opt.HeadSHA, sha)
}
Expand Down Expand Up @@ -141,102 +122,9 @@ func TestCheck_OK(t *testing.T) {
}
}

func TestCheck_branch_in_req(t *testing.T) {
const (
name = "haya14busa-linter"
owner = "haya14busa"
repo = "reviewdog"
prNum = 14
sha = "1414"
reportURL = "http://example.com/report_url"
branch = "test-branch"
)

req := &doghouse.CheckRequest{
Name: name,
Owner: owner,
Repo: repo,
PullRequest: prNum,
SHA: sha,
Branch: branch,
}

cli := &fakeCheckerGitHubCli{}
cli.FakeGetPullRequest = func(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) {
t.Fatal("GetPullRequest should not be called")
return nil, nil
}
cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
return []byte(sampleDiff), nil
}
cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) {
if opt.HeadBranch != branch {
t.Errorf("CreateCheckRunOptions.HeadBranch = %q, want %q", opt.HeadBranch, branch)
}
return &github.CheckRun{HTMLURL: github.String(reportURL)}, nil
}
checker := &Checker{req: req, gh: cli}
if _, err := checker.Check(context.Background()); err != nil {
t.Fatal(err)
}
}

func TestCheck_fail_get_pullrequest(t *testing.T) {
req := &doghouse.CheckRequest{}
cli := &fakeCheckerGitHubCli{}
cli.FakeGetPullRequest = func(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) {
return nil, errors.New("test failrue")
}
cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
return []byte(sampleDiff), nil
}
cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) {
return &github.CheckRun{}, nil
}
checker := &Checker{req: req, gh: cli}

if _, err := checker.Check(context.Background()); err == nil {
t.Fatalf("got no error, want some error")
} else {
t.Log(err)
}
}

func TestCheck_fail_empty_branch(t *testing.T) {
req := &doghouse.CheckRequest{}
cli := &fakeCheckerGitHubCli{}
cli.FakeGetPullRequest = func(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) {
return &github.PullRequest{
Head: &github.PullRequestBranch{
Ref: github.String(""),
},
}, nil
}
cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
return []byte(sampleDiff), nil
}
cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) {
return &github.CheckRun{}, nil
}
checker := &Checker{req: req, gh: cli}

if _, err := checker.Check(context.Background()); err == nil {
t.Fatalf("got no error, want some error")
} else {
t.Log(err)
}
}

func TestCheck_fail_diff(t *testing.T) {
req := &doghouse.CheckRequest{}
cli := &fakeCheckerGitHubCli{}
cli.FakeGetPullRequest = func(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) {
return &github.PullRequest{
Head: &github.PullRequestBranch{
Ref: github.String("branch"),
},
}, nil
}
cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
return nil, errors.New("test diff failure")
}
Expand All @@ -256,13 +144,6 @@ func TestCheck_fail_invalid_diff(t *testing.T) {
t.Skip("Parse invalid diff function somehow doesn't return error")
req := &doghouse.CheckRequest{}
cli := &fakeCheckerGitHubCli{}
cli.FakeGetPullRequest = func(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) {
return &github.PullRequest{
Head: &github.PullRequestBranch{
Ref: github.String("branch"),
},
}, nil
}
cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
return []byte("invalid diff"), nil
}
Expand All @@ -281,13 +162,6 @@ func TestCheck_fail_invalid_diff(t *testing.T) {
func TestCheck_fail_check(t *testing.T) {
req := &doghouse.CheckRequest{}
cli := &fakeCheckerGitHubCli{}
cli.FakeGetPullRequest = func(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) {
return &github.PullRequest{
Head: &github.PullRequestBranch{
Ref: github.String("branch"),
},
}, nil
}
cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
return []byte(sampleDiff), nil
}
Expand All @@ -306,13 +180,6 @@ func TestCheck_fail_check(t *testing.T) {
func TestCheck_fail_check_with_403(t *testing.T) {
req := &doghouse.CheckRequest{}
cli := &fakeCheckerGitHubCli{}
cli.FakeGetPullRequest = func(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) {
return &github.PullRequest{
Head: &github.PullRequestBranch{
Ref: github.String("branch"),
},
}, nil
}
cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
return []byte(sampleDiff), nil
}
Expand Down
2 changes: 2 additions & 0 deletions doghouse/service.go
View file
Expand Up @@ -16,8 +16,10 @@ type CheckRequest struct {
// Repository name.
// Required.
Repo string `json:"repo,omitempty"`

// Branch name.
// Optional.
// DEPRECATED: No need to fill this field.
Branch string `json:"branch,omitempty"`

// Annotations associated with the repository's commit and Pull Request.
Expand Down

0 comments on commit d70c5a9

Please sign in to comment.