From ebd25bf9cd84416adaf32bc3932e9d8470556eeb Mon Sep 17 00:00:00 2001 From: Torben Tretau Date: Thu, 25 May 2023 10:43:59 +0200 Subject: [PATCH] fix(github): Mergeable requirement for fork PRs While using a PR from a fork and the "Github allow mergeable bypass apply" flag, the mergeable checks were run with the wrong owner in the request, leading to 404. By choosing the owner from the head repo data it should work both, for fork PRs and in-repo PRs. --- server/events/vcs/github_client.go | 6 +++--- server/events/vcs/github_client_test.go | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index 9d6145cb22..417d544ad6 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -397,7 +397,7 @@ func isRequiredCheck(check string, required []string) bool { // GetCombinedStatusMinusApply checks Statuses for PR, excluding atlantis apply. Returns true if all other statuses are not in failure. func (g *GithubClient) GetCombinedStatusMinusApply(repo models.Repo, pull *github.PullRequest, vcstatusname string) (bool, error) { //check combined status api - status, _, err := g.client.Repositories.GetCombinedStatus(g.ctx, repo.Owner, repo.Name, *pull.Head.Ref, nil) + status, _, err := g.client.Repositories.GetCombinedStatus(g.ctx, *pull.Head.Repo.Owner.Login, repo.Name, *pull.Head.Ref, nil) if err != nil { return false, errors.Wrap(err, "getting combined status") } @@ -419,7 +419,7 @@ func (g *GithubClient) GetCombinedStatusMinusApply(repo models.Repo, pull *githu } //check check suite/check run api - checksuites, _, err := g.client.Checks.ListCheckSuitesForRef(context.Background(), repo.Owner, repo.Name, *pull.Head.Ref, nil) + checksuites, _, err := g.client.Checks.ListCheckSuitesForRef(context.Background(), *pull.Head.Repo.Owner.Login, repo.Name, *pull.Head.Ref, nil) if err != nil { return false, errors.Wrap(err, "getting check suites for ref") } @@ -428,7 +428,7 @@ func (g *GithubClient) GetCombinedStatusMinusApply(repo models.Repo, pull *githu for _, c := range checksuites.CheckSuites { if *c.Status == "completed" { //iterate over the runs inside the suite - suite, _, err := g.client.Checks.ListCheckRunsCheckSuite(context.Background(), repo.Owner, repo.Name, *c.ID, nil) + suite, _, err := g.client.Checks.ListCheckRunsCheckSuite(context.Background(), *pull.Head.Repo.Owner.Login, repo.Name, *c.ID, nil) if err != nil { return false, errors.Wrap(err, "getting check runs for check suite") } diff --git a/server/events/vcs/github_client_test.go b/server/events/vcs/github_client_test.go index c1c30f03d4..b84de928df 100644 --- a/server/events/vcs/github_client_test.go +++ b/server/events/vcs/github_client_test.go @@ -692,19 +692,19 @@ func TestGithubClient_PullIsMergeableWithAllowMergeableBypassApply(t *testing.T) testServer := httptest.NewTLSServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.RequestURI { - case "/api/v3/repos/owner/repo/pulls/1": + case "/api/v3/repos/octocat/repo/pulls/1": w.Write([]byte(response)) // nolint: errcheck return - case "/api/v3/repos/owner/repo/pulls/1/reviews?per_page=300": + case "/api/v3/repos/octocat/repo/pulls/1/reviews?per_page=300": w.Write([]byte("[]")) // nolint: errcheck return - case "/api/v3/repos/owner/repo/commits/new-topic/status": + case "/api/v3/repos/octocat/repo/commits/new-topic/status": w.Write([]byte(commitJSON)) // nolint: errcheck case "/api/graphql": w.Write([]byte(reviewDecision)) // nolint: errcheck - case "/api/v3/repos/owner/repo/branches/main/protection": + case "/api/v3/repos/octocat/repo/branches/main/protection": w.Write([]byte(branchProtectionJSON)) // nolint: errcheck - case "/api/v3/repos/owner/repo/commits/new-topic/check-suites": + case "/api/v3/repos/octocat/repo/commits/new-topic/check-suites": w.Write([]byte(checkSuites)) // nolint: errcheck default: t.Errorf("got unexpected request at %q", r.RequestURI) @@ -719,8 +719,8 @@ func TestGithubClient_PullIsMergeableWithAllowMergeableBypassApply(t *testing.T) defer disableSSLVerification()() actMergeable, err := client.PullIsMergeable(models.Repo{ - FullName: "owner/repo", - Owner: "owner", + FullName: "octocat/repo", + Owner: "octocat", Name: "repo", CloneURL: "", SanitizedCloneURL: "",