From 378b92484015bc84734140bddec5bd335bdbf10b Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Fri, 26 Apr 2024 00:21:07 +0900 Subject: [PATCH 1/5] Add a fallback to git diff command to get diff of a GitHub pull request on doghouse --- CHANGELOG.md | 2 +- doghouse/server/github_checker.go | 55 +++++++- doghouse/server/github_checker_test.go | 169 +++++++++++++++++++++++++ service/github/github.go | 8 +- 4 files changed, 231 insertions(+), 3 deletions(-) create mode 100644 doghouse/server/github_checker_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c2580e905..1d643ce818 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 of a GitHub pull request on doghouse ### :rotating_light: Breaking changes - ... diff --git a/doghouse/server/github_checker.go b/doghouse/server/github_checker.go index 3051f0ff7b..6159787889 100644 --- a/doghouse/server/github_checker.go +++ b/doghouse/server/github_checker.go @@ -2,7 +2,12 @@ package server import ( "context" + "fmt" "io" + "log" + "net/http" + "os" + "os/exec" "time" "github.com/google/go-github/v60/github" @@ -21,10 +26,58 @@ type checkerGitHubClient struct { 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) + d, resp, err := c.PullRequests.GetRaw(ctx, owner, repo, number, opt) + if err != nil { + if resp != nil && resp.StatusCode == http.StatusNotAcceptable && c.checkInstallGitCommand() { + log.Print("fallback to use git command") + return c.getPullRequestDiffUsingGitCommand(ctx, owner, repo, number) + } + + return nil, err + } return []byte(d), err } +// checkInstallGitCommand checks if git command is installed. +func (c *checkerGitHubClient) checkInstallGitCommand() bool { + _, err := exec.Command("git", "-v").CombinedOutput() + return err == nil +} + +// 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..c8331974bd --- /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).gh.GetPullRequestDiff(context.Background(), owner, repo, pr) + 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).gh + + if _, err := gh.GetPullRequestDiff(context.Background(), "o", "r", 14); 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).gh + + if _, err := gh.GetPullRequestDiff(context.Background(), "o", "r", 14); 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..8a04211e01 100644 --- a/service/github/github.go +++ b/service/github/github.go @@ -304,7 +304,7 @@ func (g *PullRequest) Diff(ctx context.Context) ([]byte, error) { opt := github.RawOptions{Type: github.Diff} 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 { + if resp != nil && resp.StatusCode == http.StatusNotAcceptable && g.checkInstallGitCommand() { log.Print("fallback to use git command") return g.diffUsingGitCommand(ctx) } @@ -314,6 +314,12 @@ func (g *PullRequest) Diff(ctx context.Context) ([]byte, error) { return []byte(d), nil } +// checkInstallGitCommand checks if git command is installed. +func (g *PullRequest) checkInstallGitCommand() bool { + _, err := exec.Command("git", "-v").CombinedOutput() + return err == nil +} + // diffUsingGitCommand returns a diff of PullRequest using git command. func (g *PullRequest) diffUsingGitCommand(ctx context.Context) ([]byte, error) { pr, _, err := g.cli.PullRequests.Get(ctx, g.owner, g.repo, g.pr) From 4b153aadc6d3fa703adc145733e3b85594703520 Mon Sep 17 00:00:00 2001 From: haya14busa Date: Sat, 1 Jun 2024 15:16:33 +0900 Subject: [PATCH 2/5] doghouse / github-check: introduce inDogHouseServer field to check git CLI fallback capability --- doghouse/appengine/checker.go | 2 +- doghouse/client/github_client.go | 2 +- doghouse/server/doghouse.go | 16 +++++++++++----- doghouse/server/doghouse_test.go | 22 +++++++++++----------- doghouse/server/github_checker.go | 6 +++--- doghouse/server/github_checker_test.go | 10 +++++----- 6 files changed, 32 insertions(+), 26 deletions(-) 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 6159787889..30ea7cc5c5 100644 --- a/doghouse/server/github_checker.go +++ b/doghouse/server/github_checker.go @@ -15,7 +15,7 @@ import ( ) 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) } @@ -24,11 +24,11 @@ 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, resp, err := c.PullRequests.GetRaw(ctx, owner, repo, number, opt) if err != nil { - if resp != nil && resp.StatusCode == http.StatusNotAcceptable && c.checkInstallGitCommand() { + if fallbackToGitCLI && resp != nil && resp.StatusCode == http.StatusNotAcceptable && c.checkInstallGitCommand() { log.Print("fallback to use git command") return c.getPullRequestDiffUsingGitCommand(ctx, owner, repo, number) } diff --git a/doghouse/server/github_checker_test.go b/doghouse/server/github_checker_test.go index c8331974bd..d95cd5d209 100644 --- a/doghouse/server/github_checker_test.go +++ b/doghouse/server/github_checker_test.go @@ -55,7 +55,7 @@ index aa49124774..781ee2492f 100644 owner := "haya14busa" repo := "reviewdog" pr := 73 - b, err := NewChecker(&doghouse.CheckRequest{}, client).gh.GetPullRequestDiff(context.Background(), owner, repo, pr) + b, err := NewChecker(&doghouse.CheckRequest{}, client, false /* inDogHouseServer */).gh.GetPullRequestDiff(context.Background(), owner, repo, pr, false /* fallbackToGitCLI */) if err != nil { t.Fatal(err) } @@ -83,9 +83,9 @@ func TestChecker_GetPullRequestDiff_fake(t *testing.T) { cli := github.NewClient(nil) cli.BaseURL, _ = url.Parse(ts.URL + "/") - gh := NewChecker(&doghouse.CheckRequest{}, cli).gh + gh := NewChecker(&doghouse.CheckRequest{}, cli, false /* inDoghouseServer */).gh - if _, err := gh.GetPullRequestDiff(context.Background(), "o", "r", 14); err != nil { + if _, err := gh.GetPullRequestDiff(context.Background(), "o", "r", 14, false /* fallbackToGitCli */); err != nil { t.Fatal(err) } if apiCalled != 1 { @@ -158,9 +158,9 @@ func TestChecker_GetPullRequestDiff_fake_fallback(t *testing.T) { cli := github.NewClient(nil) cli.BaseURL, _ = url.Parse(ts.URL + "/") - gh := NewChecker(&doghouse.CheckRequest{}, cli).gh + gh := NewChecker(&doghouse.CheckRequest{}, cli, false /* inDogHouseServer */).gh - if _, err := gh.GetPullRequestDiff(context.Background(), "o", "r", 14); err != nil { + if _, err := gh.GetPullRequestDiff(context.Background(), "o", "r", 14, true /* fallbackToGitCli */); err != nil { t.Fatal(err) } if apiCalled != 2 { From 18ef638a0681dbe78916abf1083188de1ad4383b Mon Sep 17 00:00:00 2001 From: haya14busa Date: Sat, 1 Jun 2024 15:20:03 +0900 Subject: [PATCH 3/5] fix CHANGELOG explanation --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d643ce818..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 of a GitHub pull request on doghouse +- [#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 - ... From f18fa1008c7af089c08aca4fd5d088855d7e7e26 Mon Sep 17 00:00:00 2001 From: haya14busa Date: Sat, 1 Jun 2024 16:20:43 +0900 Subject: [PATCH 4/5] move GitCommandExists to serviceutil package --- doghouse/server/github_checker.go | 3 ++- service/github/github.go | 9 ++------- service/serviceutil/serviceutil.go | 7 +++++++ 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/doghouse/server/github_checker.go b/doghouse/server/github_checker.go index 30ea7cc5c5..53027bc1a1 100644 --- a/doghouse/server/github_checker.go +++ b/doghouse/server/github_checker.go @@ -11,6 +11,7 @@ import ( "time" "github.com/google/go-github/v60/github" + "github.com/reviewdog/reviewdog/service/serviceutil" "github.com/vvakame/sdlog/aelog" ) @@ -28,7 +29,7 @@ func (c *checkerGitHubClient) GetPullRequestDiff(ctx context.Context, owner, rep opt := github.RawOptions{Type: github.Diff} d, resp, err := c.PullRequests.GetRaw(ctx, owner, repo, number, opt) if err != nil { - if fallbackToGitCLI && resp != nil && resp.StatusCode == http.StatusNotAcceptable && c.checkInstallGitCommand() { + if fallbackToGitCLI && resp != nil && resp.StatusCode == http.StatusNotAcceptable && serviceutil.GitCommandExists() { log.Print("fallback to use git command") return c.getPullRequestDiffUsingGitCommand(ctx, owner, repo, number) } diff --git a/service/github/github.go b/service/github/github.go index 8a04211e01..e748b0ca37 100644 --- a/service/github/github.go +++ b/service/github/github.go @@ -304,7 +304,8 @@ func (g *PullRequest) Diff(ctx context.Context) ([]byte, error) { opt := github.RawOptions{Type: github.Diff} 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 && g.checkInstallGitCommand() { + 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) } @@ -314,12 +315,6 @@ func (g *PullRequest) Diff(ctx context.Context) ([]byte, error) { return []byte(d), nil } -// checkInstallGitCommand checks if git command is installed. -func (g *PullRequest) checkInstallGitCommand() bool { - _, err := exec.Command("git", "-v").CombinedOutput() - return err == nil -} - // diffUsingGitCommand returns a diff of PullRequest using git command. func (g *PullRequest) diffUsingGitCommand(ctx context.Context) ([]byte, error) { pr, _, err := g.cli.PullRequests.Get(ctx, g.owner, g.repo, g.pr) 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 { From b3fd52d944e37f8c6e3d9c395b16729e2f15c860 Mon Sep 17 00:00:00 2001 From: haya14busa Date: Sat, 1 Jun 2024 16:25:01 +0900 Subject: [PATCH 5/5] remove unused checkInstallGitCommand --- doghouse/server/github_checker.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/doghouse/server/github_checker.go b/doghouse/server/github_checker.go index 53027bc1a1..82b7b6afb4 100644 --- a/doghouse/server/github_checker.go +++ b/doghouse/server/github_checker.go @@ -39,12 +39,6 @@ func (c *checkerGitHubClient) GetPullRequestDiff(ctx context.Context, owner, rep return []byte(d), err } -// checkInstallGitCommand checks if git command is installed. -func (c *checkerGitHubClient) checkInstallGitCommand() bool { - _, err := exec.Command("git", "-v").CombinedOutput() - return err == nil -} - // 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)