Repository: reviewdog/reviewdog
Pull request #327 (2019-09-28 16:36:44)

Do not set CreateCheckRunOptions.HeadBranch
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


Patch:
diff --git a/doghouse/server/doghouse.go b/doghouse/server/doghouse.go
index 422c24a7..5115b672 100644
--- a/doghouse/server/doghouse.go
+++ b/doghouse/server/doghouse.go
@@ -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.
@@ -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
@@ -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 {
@@ -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),
diff --git a/doghouse/server/github_checker.go b/doghouse/server/github_checker.go
index 2e899105..f26a11cc 100644
--- a/doghouse/server/github_checker.go
+++ b/doghouse/server/github_checker.go
@@ -7,7 +7,6 @@ import (
 )
 
 type checkerGitHubClientInterface interface {
-	GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error)
 	GetPullRequestDiff(ctx context.Context, owner, repo string, number int) ([]byte, error)
 	CreateCheckRun(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error)
 }
@@ -16,11 +15,6 @@ type checkerGitHubClient struct {
 	*github.Client
 }
 
-func (c *checkerGitHubClient) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) {
-	pr, _, err := c.PullRequests.Get(ctx, owner, repo, number)
-	return pr, err
-}
-
 func (c *checkerGitHubClient) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) ([]byte, error) {
 	opt := github.RawOptions{Type: github.Diff}
 	d, _, err := c.PullRequests.GetRaw(ctx, owner, repo, number, opt)
diff --git a/doghouse/service.go b/doghouse/service.go
index f35d0b0a..b6c77dd4 100644
--- a/doghouse/service.go
+++ b/doghouse/service.go
@@ -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.


Test patch:
diff --git a/doghouse/server/doghouse_test.go b/doghouse/server/doghouse_test.go
index b797bda7..2977f708 100644
--- a/doghouse/server/doghouse_test.go
+++ b/doghouse/server/doghouse_test.go
@@ -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)
 }
@@ -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"
 	)
 
@@ -87,16 +81,6 @@ 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
 	}
@@ -104,9 +88,6 @@ func TestCheck_OK(t *testing.T) {
 		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)
 		}
@@ -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")
 	}
@@ -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
 	}
@@ -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
 	}
@@ -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
 	}