From fe9433c5626387fdb4e9e32c279c33746d1196f8 Mon Sep 17 00:00:00 2001 From: Torben Tretau Date: Tue, 1 Aug 2023 04:25:31 +0200 Subject: [PATCH] fix(github): Mergeable requirement for fork PRs (#3620) 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. Co-authored-by: Dylan Page --- 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 7f67cb3dcc..aa64dc8cbc 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: "",