From d5bf0281111b12afdbb5e1f755f9e605a8b2f4b9 Mon Sep 17 00:00:00 2001 From: haya14busa Date: Fri, 16 Dec 2016 12:16:50 +0900 Subject: [PATCH 1/7] github: implement GitHub PullRequest Create Review API --- github.go | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/github.go b/github.go index e6098af234..17ada2e55c 100644 --- a/github.go +++ b/github.go @@ -157,3 +157,44 @@ func (g *GitHubPullRequest) comment() ([]*github.PullRequestComment, error) { } return comments, nil } + +// --- +// GitHub PullRequest Review API Implementation +// ref: https://github.com/google/go-github/issues/495 + +const ( + mediaTypePullRequestReview = "application/vnd.github.black-cat-preview+json" +) + +type Review struct { + Body *string `json:"body,omitempty"` + Event *string `json:"event,omitempty"` + Comments []*ReviewComment `json:"comments,omitempty"` +} + +type ReviewComment struct { + Path *string `json:"path,omitempty"` + Position *int `json:"position,omitempty"` + Body *string `json:"body,omitempty"` +} + +// CreateReview creates a new review comment on the specified pull request. +// +// GitHub API docs: https://developer.github.com/v3/pulls/reviews/#create-a-pull-request-review +func (g *GitHubPullRequest) CreateReview(owner, repo string, number int, review *Review) (*github.PullRequestReview, *github.Response, error) { + u := fmt.Sprintf("repos/%v/%v/pulls/%d/reviews", owner, repo, number) + + req, err := g.cli.NewRequest("POST", u, review) + if err != nil { + return nil, nil, err + } + + req.Header.Set("Accept", mediaTypePullRequestReview) + + r := new(github.PullRequestReview) + resp, err := g.cli.Do(req, r) + if err != nil { + return nil, resp, err + } + return r, resp, err +} From b016b369e18117160f64b281809b019e8cb6be73 Mon Sep 17 00:00:00 2001 From: haya14busa Date: Fri, 16 Dec 2016 12:29:21 +0900 Subject: [PATCH 2/7] github: use GitHub Review API --- github.go | 35 ++++++++++++++++++++++++++++++++++- github_test.go | 2 +- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/github.go b/github.go index 17ada2e55c..f97ff48be8 100644 --- a/github.go +++ b/github.go @@ -4,8 +4,9 @@ import ( "fmt" "os/exec" - "github.com/google/go-github/github" "golang.org/x/sync/errgroup" + + "github.com/google/go-github/github" ) var _ = github.ScopeAdminOrg @@ -85,6 +86,38 @@ func (g *GitHubPullRequest) Flash() error { if err := g.setPostedComment(); err != nil { return err } + // TODO(haya14busa,#58): remove host check when GitHub Enterprise supports + // Pull Request API. + if g.cli.BaseURL.Host == "api.github.com" { + return g.postAsReviewComment() + } + return g.postCommentsForEach() +} + +func (g *GitHubPullRequest) postAsReviewComment() error { + comments := make([]*ReviewComment, len(g.postComments)) + for i, c := range g.postComments { + cbody := commentBody(c) + comments[i] = &ReviewComment{ + Path: &c.Path, + Position: &c.LnumDiff, + Body: &cbody, + } + } + + // TODO(haya14busa): it might be useful to report overview results by "body" + // field. + var event = "COMMENT" + review := &Review{Event: &event, Comments: comments} + + _, _, err := g.CreateReview(g.owner, g.repo, g.pr, review) + if err != nil { + return err + } + return nil +} + +func (g *GitHubPullRequest) postCommentsForEach() error { var eg errgroup.Group for _, c := range g.postComments { comment := c diff --git a/github_test.go b/github_test.go index d3fa18b4d8..2313196991 100644 --- a/github_test.go +++ b/github_test.go @@ -42,7 +42,7 @@ func TestGitHubPullRequest_Post(t *testing.T) { g := NewGitHubPullReqest(client, owner, repo, pr, sha) comment := &Comment{ CheckResult: &CheckResult{ - Path: "reviewdog.go", + Path: "watchdogs.go", }, LnumDiff: 17, Body: "[reviewdog] test", From c1092ea855e43cb34738a3236c02ef6262e85291 Mon Sep 17 00:00:00 2001 From: haya14busa Date: Fri, 16 Dec 2016 13:01:18 +0900 Subject: [PATCH 3/7] github: add test for GitHub Review API --- github.go | 4 ++- github_test.go | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/github.go b/github.go index f97ff48be8..42013ffa6f 100644 --- a/github.go +++ b/github.go @@ -81,6 +81,8 @@ func commentBody(c *Comment) string { return tool + bodyPrefix + "\n" + c.Body } +var githubAPIHost = "api.github.com" + // Flash posts comments which has not been posted yet. func (g *GitHubPullRequest) Flash() error { if err := g.setPostedComment(); err != nil { @@ -88,7 +90,7 @@ func (g *GitHubPullRequest) Flash() error { } // TODO(haya14busa,#58): remove host check when GitHub Enterprise supports // Pull Request API. - if g.cli.BaseURL.Host == "api.github.com" { + if g.cli.BaseURL.Host == githubAPIHost { return g.postAsReviewComment() } return g.postCommentsForEach() diff --git a/github_test.go b/github_test.go index 2313196991..c32eab973e 100644 --- a/github_test.go +++ b/github_test.go @@ -12,6 +12,22 @@ import ( "golang.org/x/oauth2" ) +func setupGithub() (*http.ServeMux, *httptest.Server, *github.Client) { + mux := http.NewServeMux() + server := httptest.NewServer(mux) + client := github.NewClient(nil) + url, _ := url.Parse(server.URL) + client.BaseURL = url + client.UploadURL = url + return mux, server, client +} + +func testMethod(t *testing.T, r *http.Request, want string) { + if got := r.Method; got != want { + t.Errorf("Request method: %v, want %v", got, want) + } +} + const notokenSkipTestMes = "skipping test (requires actual Personal access tokens. export REVIEWDOG_TEST_GITHUB_API_TOKEN=)" func setupGitHubClient() *github.Client { @@ -208,3 +224,54 @@ func TestGitHubPullRequest_Post_Flash_mock(t *testing.T) { t.Errorf("API should be called 2 times, but %v times", apiCalled) } } + +func TestGitHubPullRequest_Post_Flash_mock_review_api(t *testing.T) { + mux, server, client := setupGithub() + defer server.Close() + defer func(u string) { githubAPIHost = u }(githubAPIHost) + u, _ := url.Parse(server.URL) + githubAPIHost = u.Host + + mux.HandleFunc("/repos/o/r/pulls/1/comments", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + }) + mux.HandleFunc("/repos/o/r/pulls/1/reviews", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "POST") + + v := new(Review) + json.NewDecoder(r.Body).Decode(v) + + if v.Body != nil { + t.Errorf("Review body = %v, want nil", *v.Body) + } + if want := "COMMENT"; *v.Event != want { + t.Errorf("Review event = %v, want %v", *v.Event, want) + } + c := v.Comments[0] + + if want := bodyPrefix + "\n[reviewdog] test"; *c.Body != want { + t.Errorf("Review comment body = %v, want %v", *c.Body, want) + } + if want := "reviewdog.go"; *c.Path != want { + t.Errorf("Review comment position = %v, want %v", *c.Path, want) + } + if want := 17; *c.Position != want { + t.Errorf("Review comment position = %v, want %v", *c.Position, want) + } + }) + + g := NewGitHubPullReqest(client, "o", "r", 1, "SHA1") + comment := &Comment{ + CheckResult: &CheckResult{ + Path: "reviewdog.go", + }, + LnumDiff: 17, + Body: "[reviewdog] test", + } + if err := g.Post(comment); err != nil { + t.Error(err) + } + if err := g.Flash(); err != nil { + t.Error(err) + } +} From 5256edd27a264e2353e104d1cd871e8720757b9a Mon Sep 17 00:00:00 2001 From: haya14busa Date: Fri, 16 Dec 2016 13:24:37 +0900 Subject: [PATCH 4/7] reviewdog: return BulkCommentService.Flash() error --- reviewdog.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/reviewdog.go b/reviewdog.go index 11b9a56e8b..aa6ea852da 100644 --- a/reviewdog.go +++ b/reviewdog.go @@ -91,9 +91,6 @@ func (w *Reviewdog) Run(r io.Reader) error { return err } - if bulk, ok := w.c.(BulkCommentService); ok { - defer bulk.Flash() - } for _, result := range results { addedline := addedlines.Get(result.Path, result.Lnum) if filepath.IsAbs(result.Path) { @@ -117,6 +114,10 @@ func (w *Reviewdog) Run(r io.Reader) error { } } + if bulk, ok := w.c.(BulkCommentService); ok { + return bulk.Flash() + } + return nil } From 2851086423ce97c7f90c7558ba7db1d38f7b3932 Mon Sep 17 00:00:00 2001 From: haya14busa Date: Fri, 16 Dec 2016 16:48:42 +0900 Subject: [PATCH 5/7] github: skip comment if no comments and lock Post() and Flash() --- github.go | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/github.go b/github.go index 42013ffa6f..8fb2b0599a 100644 --- a/github.go +++ b/github.go @@ -2,7 +2,9 @@ package reviewdog import ( "fmt" + "log" "os/exec" + "sync" "golang.org/x/sync/errgroup" @@ -51,6 +53,8 @@ type GitHubPullRequest struct { sha string postedcs postedcomments + + muFlash sync.Mutex } // NewGitHubPullReqest returns a new GitHubPullRequest service. @@ -67,6 +71,8 @@ func NewGitHubPullReqest(cli *github.Client, owner, repo string, pr int, sha str // Post accepts a comment and holds it. Flash method actually posts comments to // GitHub in parallel. func (g *GitHubPullRequest) Post(c *Comment) error { + g.muFlash.Lock() + defer g.muFlash.Unlock() g.postComments = append(g.postComments, c) return nil } @@ -85,6 +91,9 @@ var githubAPIHost = "api.github.com" // Flash posts comments which has not been posted yet. func (g *GitHubPullRequest) Flash() error { + g.muFlash.Lock() + defer g.muFlash.Unlock() + if err := g.setPostedComment(); err != nil { return err } @@ -97,19 +106,26 @@ func (g *GitHubPullRequest) Flash() error { } func (g *GitHubPullRequest) postAsReviewComment() error { - comments := make([]*ReviewComment, len(g.postComments)) - for i, c := range g.postComments { + comments := make([]*ReviewComment, 0, len(g.postComments)) + for _, c := range g.postComments { + if g.postedcs.IsPosted(c) { + continue + } cbody := commentBody(c) - comments[i] = &ReviewComment{ + comments = append(comments, &ReviewComment{ Path: &c.Path, Position: &c.LnumDiff, Body: &cbody, - } + }) + } + + if len(comments) == 0 { + return nil } // TODO(haya14busa): it might be useful to report overview results by "body" // field. - var event = "COMMENT" + event := "COMMENT" review := &Review{Event: &event, Comments: comments} _, _, err := g.CreateReview(g.owner, g.repo, g.pr, review) @@ -229,6 +245,7 @@ func (g *GitHubPullRequest) CreateReview(owner, repo string, number int, review r := new(github.PullRequestReview) resp, err := g.cli.Do(req, r) if err != nil { + log.Printf("GitHub Review API error: %v", err) return nil, resp, err } return r, resp, err From dccb529ec920eb23bb96019f05cb00cceaae8d7b Mon Sep 17 00:00:00 2001 From: haya14busa Date: Fri, 16 Dec 2016 16:50:36 +0900 Subject: [PATCH 6/7] droneio: update secure token --- .drone.sec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.drone.sec b/.drone.sec index 4f46f61550..1c3ffd3449 100644 --- a/.drone.sec +++ b/.drone.sec @@ -1 +1 @@ -eyJhbGciOiJSU0EtT0FFUCIsImVuYyI6IkExMjhHQ00ifQ.jNZPGtoP-mdDdVeX3QoNjHkzW3hgBRL_HQKQRkC5dksDXBu9xOCH2dnrFVtPMMXlxdjBNAJw6bc3q27FZxgvBLqiIwj-x_gs5y_X7C5t11d3lWP2-bS-JbHpWiZ5t-Mh_lT-w0Tz4fPH9FtxdOERg_cYfFzXOLUNmq4G3lDFQbLy8Kx3MJNqnDgd3psTXAaJUOToII-wtBgtze-TbDb34kJtCrlEJ5-guepsRXXWl3uu8u7t8jvEgsrwp1rs09rdwGucb2dWf_vo5258pjud2-oinL2nzCKIdzXR4de5B-CFStm9D8QmLqoNOreWe05A2bS6tvsXLJPSOduFB_uH2A.IuaZb-A6fGgzcyff.3uGTMjhUXBGz7KjJGEDsoFar_EzpBb4U3geXkekhCoQoxsXbcWDWcCJLiuX3lXM8aeA6wZHgLpr_6dNGjO10UpwyRozzenzA1dJ3YIqT_PjIWn4sVlOUBT2oIOn7EyCh-2JJWghl9Jnsf7K1_a-fxfLzqB4NrkUHdJNaev5gvh2m85SrfIprKIjI6viT8lLMFtpTsPbmK56-J9Zsgxg-.toQoDcioYYq9TZ6yNiAtug \ No newline at end of file +eyJhbGciOiJSU0EtT0FFUCIsImVuYyI6IkExMjhHQ00ifQ.Zv7ykMwj9ktwhBSlXILbLYz5ZdKyVtEe0zdq5u9HIQ7lDceSYXl3oSspaZtz9TT7FbTj0Qf6p28HtutdE7QCWHuCSGXRXXi3AGS4UEVJq3MvnQGLnnTx8qXjCGhB9KSy3sDo7XqUw7GCCmr3RlZmBBgaIHE9QENnUDp0pXPjzkbNbOR8RQr9E8kja6FTBqCjqEI1xoWDuo3RoH82kRK68Yn3efW-7QFQwP0KufJWlFGZ1Nsr4RNg-Vz8wXzcM12FdUY054MkCHO1b_7A4_lwho52uLojWBnf8P873laDaxDy6au8pSXGoMcGljFzVyPg9FvWPOAu_3tX0JjyES3zRg.6H5T4VhMkZU9XzEn.igEV4rSNLj8beubTCj6xv8R1Nywc_A3OgXlY-svEzzgW_m_R_OJcj9x4P1Z_ZqyFdyeGq0myOFT4TXqrMjLLAA_Xl4-evAKDeBmWNHoRBxRrW4xEmvHlb4WyNfrS5Ve6DwM-tmoNPJSe-8-jWtgNWGaK-LN3lOyrbc_D0W039GYSLGLtutkMS2PWYIDLW_nrciUZuEVpuwMxc12VdH0L.agcIEJDOh_Ohido1KCLr8w \ No newline at end of file From 88976cff8332af2d5c8a37e48b45526bd08344b8 Mon Sep 17 00:00:00 2001 From: haya14busa Date: Fri, 16 Dec 2016 17:38:14 +0900 Subject: [PATCH 7/7] github: add comments --- github.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/github.go b/github.go index 8fb2b0599a..a38a22e38f 100644 --- a/github.go +++ b/github.go @@ -217,12 +217,14 @@ const ( mediaTypePullRequestReview = "application/vnd.github.black-cat-preview+json" ) +// Review represents a pull request review. type Review struct { Body *string `json:"body,omitempty"` Event *string `json:"event,omitempty"` Comments []*ReviewComment `json:"comments,omitempty"` } +// ReviewComment represents draft review comments. type ReviewComment struct { Path *string `json:"path,omitempty"` Position *int `json:"position,omitempty"`