From 4e78a959138d768e2c162038fb6e5a5339d9dd30 Mon Sep 17 00:00:00 2001 From: haya14busa Date: Sat, 28 Sep 2019 17:47:16 +0000 Subject: [PATCH] Support more than 50 annotations Fix #147 --- doghouse/server/doghouse.go | 72 ++++++++++++++--- doghouse/server/doghouse_test.go | 123 ++++++++++++++++++++++++------ doghouse/server/github_checker.go | 6 ++ 3 files changed, 167 insertions(+), 34 deletions(-) diff --git a/doghouse/server/doghouse.go b/doghouse/server/doghouse.go index 5115b67278..dc49c65016 100644 --- a/doghouse/server/doghouse.go +++ b/doghouse/server/doghouse.go @@ -22,6 +22,11 @@ import ( // Only 65535 characters are allowed; 250684 were supplied. [] const maxFilteredFinding = 150 +// > The Checks API limits the number of annotations to a maximum of 50 per API +// > request. +// https://developer.github.com/v3/checks/runs/#output-object +const maxAnnotationsPerRequest = 50 + type Checker struct { req *doghouse.CheckRequest gh checkerGitHubClientInterface @@ -39,7 +44,8 @@ func (ch *Checker) Check(ctx context.Context) (*doghouse.CheckResponse, error) { results := annotationsToCheckResults(ch.req.Annotations) filtered := reviewdog.FilterCheck(results, filediffs, 1, "") - checkRun, err := ch.postCheck(ctx, filtered) + + check, err := ch.createCheck(ctx) 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 @@ -50,6 +56,11 @@ func (ch *Checker) Check(ctx context.Context) (*doghouse.CheckResponse, error) { if err, ok := err.(*github.ErrorResponse); ok && err.Response.StatusCode == http.StatusForbidden { return &doghouse.CheckResponse{CheckedResults: filtered}, nil } + return nil, fmt.Errorf("failed to create check: %v", err) + } + + checkRun, err := ch.postCheck(ctx, check.GetID(), filtered) + if err != nil { return nil, fmt.Errorf("failed to post result: %v", err) } res := &doghouse.CheckResponse{ @@ -58,7 +69,7 @@ func (ch *Checker) Check(ctx context.Context) (*doghouse.CheckResponse, error) { return res, nil } -func (ch *Checker) postCheck(ctx context.Context, checks []*reviewdog.FilteredCheck) (*github.CheckRun, error) { +func (ch *Checker) postCheck(ctx context.Context, checkID int64, checks []*reviewdog.FilteredCheck) (*github.CheckRun, error) { var annotations []*github.CheckRunAnnotation for _, c := range checks { if !c.InDiff { @@ -66,32 +77,62 @@ func (ch *Checker) postCheck(ctx context.Context, checks []*reviewdog.FilteredCh } annotations = append(annotations, ch.toCheckRunAnnotation(c)) } + if err := ch.postAnnotations(ctx, checkID, annotations); err != nil { + return nil, fmt.Errorf("failed to post annotations: %v", err) + } + conclusion := "success" if len(annotations) > 0 { conclusion = ch.conclusion() } - name := "reviewdog" + name := ch.checkName() title := "reviewdog report" - if ch.req.Name != "" { - name = ch.req.Name + if name != "reviewdog" { title = fmt.Sprintf("reviewdog [%s] report", name) } - opt := github.CreateCheckRunOptions{ - Name: name, - ExternalID: github.String(name), - HeadSHA: ch.req.SHA, + opt := github.UpdateCheckRunOptions{ Status: github.String("completed"), Conclusion: github.String(conclusion), CompletedAt: &github.Timestamp{Time: time.Now()}, Output: &github.CheckRunOutput{ - Title: github.String(title), - Summary: github.String(ch.summary(checks)), - Annotations: annotations, + Title: github.String(title), + Summary: github.String(ch.summary(checks)), }, } + return ch.gh.UpdateCheckRun(ctx, ch.req.Owner, ch.req.Repo, checkID, opt) +} + +func (ch *Checker) createCheck(ctx context.Context) (*github.CheckRun, error) { + opt := github.CreateCheckRunOptions{ + Name: ch.checkName(), + HeadSHA: ch.req.SHA, + Status: github.String("in_progress"), + } return ch.gh.CreateCheckRun(ctx, ch.req.Owner, ch.req.Repo, opt) } +func (ch *Checker) postAnnotations(ctx context.Context, checkID int64, annotations []*github.CheckRunAnnotation) error { + opt := github.UpdateCheckRunOptions{ + Output: &github.CheckRunOutput{ + Annotations: annotations[:min(maxAnnotationsPerRequest, len(annotations))], + }, + } + if _, err := ch.gh.UpdateCheckRun(ctx, ch.req.Owner, ch.req.Repo, checkID, opt); err != nil { + return err + } + if len(annotations) > maxAnnotationsPerRequest { + return ch.postAnnotations(ctx, checkID, annotations[maxAnnotationsPerRequest:]) + } + return nil +} + +func (ch *Checker) checkName() string { + if ch.req.Name != "" { + return ch.req.Name + } + return "reviewdog" +} + // https://developer.github.com/v3/checks/runs/#parameters-1 func (ch *Checker) conclusion() string { switch strings.ToLower(ch.req.Level) { @@ -218,3 +259,10 @@ func annotationsToCheckResults(as []*doghouse.Annotation) []*reviewdog.CheckResu } return cs } + +func min(x, y int) int { + if x > y { + return y + } + return x +} diff --git a/doghouse/server/doghouse_test.go b/doghouse/server/doghouse_test.go index 2977f70854..55fb7e3915 100644 --- a/doghouse/server/doghouse_test.go +++ b/doghouse/server/doghouse_test.go @@ -15,6 +15,7 @@ type fakeCheckerGitHubCli struct { checkerGitHubClientInterface 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) + FakeUpdateCheckRun func(ctx context.Context, owner, repo string, checkID int64, opt github.UpdateCheckRunOptions) (*github.CheckRun, error) } func (f *fakeCheckerGitHubCli) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) ([]byte, error) { @@ -25,6 +26,10 @@ func (f *fakeCheckerGitHubCli) CreateCheckRun(ctx context.Context, owner, repo s return f.FakeCreateCheckRun(ctx, owner, repo, opt) } +func (f *fakeCheckerGitHubCli) UpdateCheckRun(ctx context.Context, owner, repo string, checkID int64, opt github.UpdateCheckRunOptions) (*github.CheckRun, error) { + return f.FakeUpdateCheckRun(ctx, owner, repo, checkID, opt) +} + const sampleDiff = `--- sample.old.txt 2016-10-13 05:09:35.820791185 +0900 +++ sample.new.txt 2016-10-13 05:15:26.839245048 +0900 @@ -1,3 +1,4 @@ @@ -48,13 +53,14 @@ const sampleDiff = `--- sample.old.txt 2016-10-13 05:09:35.820791185 +0900 func TestCheck_OK(t *testing.T) { const ( - name = "haya14busa-linter" - owner = "haya14busa" - repo = "reviewdog" - prNum = 14 - sha = "1414" - reportURL = "http://example.com/report_url" - conclusion = "neutral" + name = "haya14busa-linter" + owner = "haya14busa" + repo = "reviewdog" + prNum = 14 + sha = "1414" + reportURL = "http://example.com/report_url" + conclusion = "neutral" + wantCheckID = 1414 ) req := &doghouse.CheckRequest{ @@ -91,23 +97,32 @@ func TestCheck_OK(t *testing.T) { if opt.HeadSHA != sha { t.Errorf("CreateCheckRunOptions.HeadSHA = %q, want %q", opt.HeadSHA, sha) } - if *opt.Conclusion != conclusion { - t.Errorf("CreateCheckRunOptions.Conclusion = %q, want %q", *opt.Conclusion, conclusion) + return &github.CheckRun{ID: github.Int64(wantCheckID)}, nil + } + cli.FakeUpdateCheckRun = func(ctx context.Context, owner, repo string, checkID int64, opt github.UpdateCheckRunOptions) (*github.CheckRun, error) { + if checkID != wantCheckID { + t.Errorf("UpdateCheckRun: checkID = %d, want %d", checkID, wantCheckID) } annotations := opt.Output.Annotations - wantAnnotaions := []*github.CheckRunAnnotation{ - { - Path: github.String("sample.new.txt"), - StartLine: github.Int(2), - EndLine: github.Int(2), - AnnotationLevel: github.String("warning"), - Message: github.String("test message"), - Title: github.String("[haya14busa-linter] sample.new.txt#L2"), - RawDetails: github.String("raw test message"), - }, - } - if d := cmp.Diff(annotations, wantAnnotaions); d != "" { - t.Errorf("Annotation diff found:\n%s", d) + if len(annotations) == 0 { + if *opt.Conclusion != conclusion { + t.Errorf("UpdateCheckRunOptions.Conclusion = %q, want %q", *opt.Conclusion, conclusion) + } + } else { + wantAnnotaions := []*github.CheckRunAnnotation{ + { + Path: github.String("sample.new.txt"), + StartLine: github.Int(2), + EndLine: github.Int(2), + AnnotationLevel: github.String("warning"), + Message: github.String("test message"), + Title: github.String("[haya14busa-linter] sample.new.txt#L2"), + RawDetails: github.String("raw test message"), + }, + } + if d := cmp.Diff(annotations, wantAnnotaions); d != "" { + t.Errorf("Annotation diff found:\n%s", d) + } } return &github.CheckRun{HTMLURL: github.String(reportURL)}, nil } @@ -122,6 +137,70 @@ func TestCheck_OK(t *testing.T) { } } +func TestCheck_OK_multiple_update_runs(t *testing.T) { + const ( + name = "haya14busa-linter" + owner = "haya14busa" + repo = "reviewdog" + prNum = 14 + sha = "1414" + reportURL = "http://example.com/report_url" + conclusion = "neutral" + wantCheckID = 1414 + ) + + req := &doghouse.CheckRequest{ + Name: name, + Owner: owner, + Repo: repo, + PullRequest: prNum, + SHA: sha, + Level: "warning", + } + for i := 0; i < 101; i++ { + req.Annotations = append(req.Annotations, &doghouse.Annotation{ + Path: "sample.new.txt", + Line: 2, + Message: "test message", + RawMessage: "raw test message", + }) + } + + cli := &fakeCheckerGitHubCli{} + 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.HeadSHA != sha { + t.Errorf("CreateCheckRunOptions.HeadSHA = %q, want %q", opt.HeadSHA, sha) + } + return &github.CheckRun{ID: github.Int64(wantCheckID)}, nil + } + cli.FakeUpdateCheckRun = func(ctx context.Context, owner, repo string, checkID int64, opt github.UpdateCheckRunOptions) (*github.CheckRun, error) { + if checkID != wantCheckID { + t.Errorf("UpdateCheckRun: checkID = %d, want %d", checkID, wantCheckID) + } + annotations := opt.Output.Annotations + switch len(annotations) { + case 0: + if *opt.Conclusion != conclusion { + t.Errorf("UpdateCheckRunOptions.Conclusion = %q, want %q", *opt.Conclusion, conclusion) + } + case maxAnnotationsPerRequest, 1: // Expected + default: + t.Errorf("UpdateCheckRun: len(annotations) = %d, but it's unexpected", len(annotations)) + } + 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_diff(t *testing.T) { req := &doghouse.CheckRequest{} cli := &fakeCheckerGitHubCli{} diff --git a/doghouse/server/github_checker.go b/doghouse/server/github_checker.go index f26a11cc5a..537b9a6307 100644 --- a/doghouse/server/github_checker.go +++ b/doghouse/server/github_checker.go @@ -9,6 +9,7 @@ import ( type checkerGitHubClientInterface interface { GetPullRequestDiff(ctx context.Context, owner, repo string, number int) ([]byte, error) CreateCheckRun(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) + UpdateCheckRun(ctx context.Context, owner, repo string, checkID int64, opt github.UpdateCheckRunOptions) (*github.CheckRun, error) } type checkerGitHubClient struct { @@ -25,3 +26,8 @@ func (c *checkerGitHubClient) CreateCheckRun(ctx context.Context, owner, repo st checkRun, _, err := c.Checks.CreateCheckRun(ctx, owner, repo, opt) return checkRun, err } + +func (c *checkerGitHubClient) UpdateCheckRun(ctx context.Context, owner, repo string, checkID int64, opt github.UpdateCheckRunOptions) (*github.CheckRun, error) { + checkRun, _, err := c.Checks.UpdateCheckRun(ctx, owner, repo, checkID, opt) + return checkRun, err +}