diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c2580e905..6197e1f2a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - ... ### :bug: Fixes -- ... +- [#1722](https://github.com/reviewdog/reviewdog/pull/1722) Add a fallback to `git diff` command to get diff for `github-check` / `github-pr-check` reporters with GitHub Actions (in other words, when not in DogHouse server). ### :rotating_light: Breaking changes - ... diff --git a/doghouse/appengine/checker.go b/doghouse/appengine/checker.go index 7a18e3bde8..a3926b61b2 100644 --- a/doghouse/appengine/checker.go +++ b/doghouse/appengine/checker.go @@ -59,7 +59,7 @@ func (gc *githubChecker) handleCheck(w http.ResponseWriter, r *http.Request) { return } - res, err := server.NewChecker(&req, gh).Check(ctx, true) + res, err := server.NewChecker(&req, gh, true /* inDogHouseServer */).Check(ctx, true) if err != nil { aelog.Errorf(ctx, "failed to run checker: %v", err) w.WriteHeader(http.StatusBadRequest) diff --git a/doghouse/client/github_client.go b/doghouse/client/github_client.go index 50cbab2d5d..2a9b80d1b0 100644 --- a/doghouse/client/github_client.go +++ b/doghouse/client/github_client.go @@ -16,5 +16,5 @@ type GitHubClient struct { } func (c *GitHubClient) Check(ctx context.Context, req *doghouse.CheckRequest, makeRequest bool) (*doghouse.CheckResponse, error) { - return server.NewChecker(req, c.Client).Check(ctx, makeRequest) + return server.NewChecker(req, c.Client, false /* inDogHouseServer */).Check(ctx, makeRequest) } diff --git a/doghouse/server/doghouse.go b/doghouse/server/doghouse.go index 596607fcc0..693604ff73 100644 --- a/doghouse/server/doghouse.go +++ b/doghouse/server/doghouse.go @@ -32,12 +32,17 @@ const maxAllowedSize = 65535 const maxAnnotationsPerRequest = 50 type Checker struct { - req *doghouse.CheckRequest - gh checkerGitHubClientInterface + req *doghouse.CheckRequest + gh checkerGitHubClientInterface + inDogHouseServer bool // If true, this checker runs in the DogHouse server. } -func NewChecker(req *doghouse.CheckRequest, gh *github.Client) *Checker { - return &Checker{req: req, gh: &checkerGitHubClient{Client: gh}} +func NewChecker(req *doghouse.CheckRequest, gh *github.Client, inDogHouseServer bool) *Checker { + return &Checker{ + req: req, + gh: &checkerGitHubClient{Client: gh}, + inDogHouseServer: inDogHouseServer, + } } func (ch *Checker) Check(ctx context.Context, makeRequest bool) (*doghouse.CheckResponse, error) { @@ -350,7 +355,8 @@ func (ch *Checker) pullRequestDiff(ctx context.Context, pr int) ([]*diff.FileDif } func (ch *Checker) rawPullRequestDiff(ctx context.Context, pr int) ([]byte, error) { - d, err := ch.gh.GetPullRequestDiff(ctx, ch.req.Owner, ch.req.Repo, pr) + fallbackToGitCLI := !ch.inDogHouseServer + d, err := ch.gh.GetPullRequestDiff(ctx, ch.req.Owner, ch.req.Repo, pr, fallbackToGitCLI) if err != nil { return nil, err } diff --git a/doghouse/server/doghouse_test.go b/doghouse/server/doghouse_test.go index eb14db478c..967ec63f7b 100644 --- a/doghouse/server/doghouse_test.go +++ b/doghouse/server/doghouse_test.go @@ -17,13 +17,13 @@ import ( ) type fakeCheckerGitHubCli struct { - FakeGetPullRequestDiff func(ctx context.Context, owner, repo string, number int) ([]byte, error) + FakeGetPullRequestDiff func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]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) { - return f.FakeGetPullRequestDiff(ctx, owner, repo, number) +func (f *fakeCheckerGitHubCli) GetPullRequestDiff(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) { + return f.FakeGetPullRequestDiff(ctx, owner, repo, number, fallbackToGitCLI) } func (f *fakeCheckerGitHubCli) CreateCheckRun(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) { @@ -191,7 +191,7 @@ func TestCheck_OK(t *testing.T) { } cli := &fakeCheckerGitHubCli{} - cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) { + cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) { return []byte(sampleDiff), nil } cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) { @@ -361,7 +361,7 @@ func testOutsideDiff(t *testing.T, outsideDiff bool, filterMode filter.Mode) { } cli := &fakeCheckerGitHubCli{} - cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) { + cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) { return []byte(sampleDiff), nil } cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) { @@ -451,7 +451,7 @@ func TestCheck_OK_multiple_update_runs(t *testing.T) { } cli := &fakeCheckerGitHubCli{} - cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) { + cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) { return []byte(sampleDiff), nil } cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) { @@ -532,7 +532,7 @@ func TestCheck_OK_nonPullRequests(t *testing.T) { } cli := &fakeCheckerGitHubCli{} - cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) { + cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) { t.Errorf("GetPullRequestDiff should not be called") return nil, nil } @@ -589,7 +589,7 @@ func TestCheck_OK_nonPullRequests(t *testing.T) { func TestCheck_fail_diff(t *testing.T) { req := &doghouse.CheckRequest{PullRequest: 1} cli := &fakeCheckerGitHubCli{} - cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) { + cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) { return nil, errors.New("test diff failure") } cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) { @@ -608,7 +608,7 @@ func TestCheck_fail_invalid_diff(t *testing.T) { t.Skip("Parse invalid diff function somehow doesn't return error") req := &doghouse.CheckRequest{PullRequest: 1} cli := &fakeCheckerGitHubCli{} - cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) { + cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) { return []byte("invalid diff"), nil } cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) { @@ -626,7 +626,7 @@ func TestCheck_fail_invalid_diff(t *testing.T) { func TestCheck_fail_check(t *testing.T) { req := &doghouse.CheckRequest{PullRequest: 1} cli := &fakeCheckerGitHubCli{} - cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) { + cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) { return []byte(sampleDiff), nil } cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) { @@ -644,7 +644,7 @@ func TestCheck_fail_check(t *testing.T) { func TestCheck_fail_check_with_403(t *testing.T) { req := &doghouse.CheckRequest{PullRequest: 1} cli := &fakeCheckerGitHubCli{} - cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) { + cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) { return []byte(sampleDiff), nil } cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) { diff --git a/doghouse/server/github_checker.go b/doghouse/server/github_checker.go index 3051f0ff7b..82b7b6afb4 100644 --- a/doghouse/server/github_checker.go +++ b/doghouse/server/github_checker.go @@ -2,15 +2,21 @@ package server import ( "context" + "fmt" "io" + "log" + "net/http" + "os" + "os/exec" "time" "github.com/google/go-github/v60/github" + "github.com/reviewdog/reviewdog/service/serviceutil" "github.com/vvakame/sdlog/aelog" ) type checkerGitHubClientInterface interface { - GetPullRequestDiff(ctx context.Context, owner, repo string, number int) ([]byte, error) + GetPullRequestDiff(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]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) } @@ -19,12 +25,54 @@ type checkerGitHubClient struct { *github.Client } -func (c *checkerGitHubClient) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) ([]byte, error) { +func (c *checkerGitHubClient) GetPullRequestDiff(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) { opt := github.RawOptions{Type: github.Diff} - d, _, err := c.PullRequests.GetRaw(ctx, owner, repo, number, opt) + d, resp, err := c.PullRequests.GetRaw(ctx, owner, repo, number, opt) + if err != nil { + if fallbackToGitCLI && resp != nil && resp.StatusCode == http.StatusNotAcceptable && serviceutil.GitCommandExists() { + log.Print("fallback to use git command") + return c.getPullRequestDiffUsingGitCommand(ctx, owner, repo, number) + } + + return nil, err + } return []byte(d), err } +// getPullRequestDiffUsingGitCommand returns a diff of PullRequest using git command. +func (c *checkerGitHubClient) getPullRequestDiffUsingGitCommand(ctx context.Context, owner, repo string, number int) ([]byte, error) { + pr, _, err := c.PullRequests.Get(ctx, owner, repo, number) + if err != nil { + return nil, err + } + + head := pr.GetHead() + headSha := head.GetSHA() + + commitsComparison, _, err := c.Repositories.CompareCommits(ctx, owner, repo, headSha, pr.GetBase().GetSHA(), nil) + if err != nil { + return nil, err + } + + mergeBaseSha := commitsComparison.GetMergeBaseCommit().GetSHA() + + if os.Getenv("REVIEWDOG_SKIP_GIT_FETCH") != "true" { + for _, sha := range []string{mergeBaseSha, headSha} { + _, err := exec.Command("git", "fetch", "--depth=1", head.GetRepo().GetHTMLURL(), sha).CombinedOutput() + if err != nil { + return nil, fmt.Errorf("failed to run git fetch: %w", err) + } + } + } + + bytes, err := exec.Command("git", "diff", "--find-renames", mergeBaseSha, headSha).CombinedOutput() + if err != nil { + return nil, fmt.Errorf("failed to run git diff: %w", err) + } + + return bytes, nil +} + func (c *checkerGitHubClient) CreateCheckRun(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) { checkRun, _, err := c.Checks.CreateCheckRun(ctx, owner, repo, opt) return checkRun, err diff --git a/doghouse/server/github_checker_test.go b/doghouse/server/github_checker_test.go new file mode 100644 index 0000000000..d95cd5d209 --- /dev/null +++ b/doghouse/server/github_checker_test.go @@ -0,0 +1,169 @@ +package server + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "os" + "strings" + "testing" + + "github.com/google/go-github/v60/github" + "github.com/reviewdog/reviewdog/doghouse" + "golang.org/x/oauth2" +) + +const notokenSkipTestMes = "skipping test (requires actual Personal access tokens. export REVIEWDOG_TEST_GITHUB_API_TOKEN=)" + +func setupGitHubClient() *github.Client { + token := os.Getenv("REVIEWDOG_TEST_GITHUB_API_TOKEN") + if token == "" { + return nil + } + ts := oauth2.StaticTokenSource( + &oauth2.Token{AccessToken: token}, + ) + tc := oauth2.NewClient(context.TODO(), ts) + return github.NewClient(tc) +} + +func TestChecker_GetPullRequestDiff(t *testing.T) { + if testing.Short() { + t.Skip("skipping test which contains actual API requests in short mode") + } + client := setupGitHubClient() + if client == nil { + t.Skip(notokenSkipTestMes) + } + + want := `diff --git a/.codecov.yml b/.codecov.yml +index aa49124774..781ee2492f 100644 +--- a/.codecov.yml ++++ b/.codecov.yml +@@ -7,5 +7,4 @@ coverage: + default: + target: 0% + +-comment: +- layout: "header" ++comment: false +` + + // https://github.com/reviewdog/reviewdog/pull/73 + owner := "haya14busa" + repo := "reviewdog" + pr := 73 + b, err := NewChecker(&doghouse.CheckRequest{}, client, false /* inDogHouseServer */).gh.GetPullRequestDiff(context.Background(), owner, repo, pr, false /* fallbackToGitCLI */) + if err != nil { + t.Fatal(err) + } + if got := string(b); got != want { + t.Errorf("got:\n%v\nwant:\n%v", got, want) + } +} + +func TestChecker_GetPullRequestDiff_fake(t *testing.T) { + apiCalled := 0 + mux := http.NewServeMux() + mux.HandleFunc("/repos/o/r/pulls/14", func(w http.ResponseWriter, r *http.Request) { + apiCalled++ + if r.Method != http.MethodGet { + t.Errorf("unexpected access: %v %v", r.Method, r.URL) + } + if accept := r.Header.Get("Accept"); !strings.Contains(accept, "diff") { + t.Errorf("Accept header doesn't contain 'diff': %v", accept) + } + w.Write([]byte("Pull Request diff")) + }) + ts := httptest.NewServer(mux) + defer ts.Close() + + cli := github.NewClient(nil) + cli.BaseURL, _ = url.Parse(ts.URL + "/") + + gh := NewChecker(&doghouse.CheckRequest{}, cli, false /* inDoghouseServer */).gh + + if _, err := gh.GetPullRequestDiff(context.Background(), "o", "r", 14, false /* fallbackToGitCli */); err != nil { + t.Fatal(err) + } + if apiCalled != 1 { + t.Errorf("GitHub API should be called once; called %v times", apiCalled) + } +} + +func TestChecker_GetPullRequestDiff_fake_fallback(t *testing.T) { + apiCalled := 0 + mux := http.NewServeMux() + headSHA := "HEAD^" + baseSHA := "HEAD" + mux.HandleFunc("/repos/o/r/pulls/14", func(w http.ResponseWriter, r *http.Request) { + apiCalled++ + if r.Method != http.MethodGet { + t.Errorf("unexpected access: %v %v", r.Method, r.URL) + } + if accept := r.Header.Get("Accept"); strings.Contains(accept, "diff") { + w.WriteHeader(http.StatusNotAcceptable) + return + } + if accept := r.Header.Get("Accept"); accept != "application/vnd.github.v3+json" { + t.Errorf("Accept header doesn't contain 'diff': %v", accept) + } + + pullRequestJSON, err := json.Marshal(github.PullRequest{ + Head: &github.PullRequestBranch{ + SHA: &headSHA, + }, + Base: &github.PullRequestBranch{ + SHA: &baseSHA, + }, + }) + if err != nil { + t.Fatal(err) + } + + if _, err := w.Write(pullRequestJSON); err != nil { + t.Fatal(err) + } + }) + mux.HandleFunc("/repos/o/r/compare/"+headSHA+"..."+baseSHA, func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + t.Errorf("unexpected access: %v %v", r.Method, r.URL) + } + if accept := r.Header.Get("Accept"); accept != "application/vnd.github.v3+json" { + t.Errorf("Accept header doesn't contain 'diff': %v", accept) + } + + mergeBaseSha := "HEAD^" + + commitsComparisonJSON, err := json.Marshal(github.CommitsComparison{ + MergeBaseCommit: &github.RepositoryCommit{ + SHA: &mergeBaseSha, + }, + }) + if err != nil { + t.Fatal(err) + } + + if _, err := w.Write(commitsComparisonJSON); err != nil { + t.Fatal(err) + } + }) + ts := httptest.NewServer(mux) + defer ts.Close() + + t.Setenv("REVIEWDOG_SKIP_GIT_FETCH", "true") + + cli := github.NewClient(nil) + cli.BaseURL, _ = url.Parse(ts.URL + "/") + + gh := NewChecker(&doghouse.CheckRequest{}, cli, false /* inDogHouseServer */).gh + + if _, err := gh.GetPullRequestDiff(context.Background(), "o", "r", 14, true /* fallbackToGitCli */); err != nil { + t.Fatal(err) + } + if apiCalled != 2 { + t.Errorf("GitHub API should be called twice; called %v times", apiCalled) + } +} diff --git a/service/github/github.go b/service/github/github.go index 00857a2b97..e748b0ca37 100644 --- a/service/github/github.go +++ b/service/github/github.go @@ -305,6 +305,7 @@ func (g *PullRequest) Diff(ctx context.Context) ([]byte, error) { d, resp, err := g.cli.PullRequests.GetRaw(ctx, g.owner, g.repo, g.pr, opt) if err != nil { if resp != nil && resp.StatusCode == http.StatusNotAcceptable { + // git command should exist here. See NewGitHubPullRequest. log.Print("fallback to use git command") return g.diffUsingGitCommand(ctx) } diff --git a/service/serviceutil/serviceutil.go b/service/serviceutil/serviceutil.go index 320b664d25..27d8b165df 100644 --- a/service/serviceutil/serviceutil.go +++ b/service/serviceutil/serviceutil.go @@ -3,6 +3,7 @@ package serviceutil import ( "fmt" "os" + "os/exec" "path/filepath" "strings" ) @@ -41,6 +42,12 @@ func GetGitRoot() (string, error) { return findGitRoot(cwd) } +// GitCommandExists checks if git command is installed. +func GitCommandExists() bool { + _, err := exec.Command("git", "-v").CombinedOutput() + return err == nil +} + func findGitRoot(path string) (string, error) { gitPath, err := findDotGitPath(path) if err != nil {