From dcff6c4f2d849733b35a8d331bb2ddd03f710429 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 18 Feb 2021 17:46:13 +0100 Subject: [PATCH] Fix `pr status` for GHE 2.22 and older This queries for the availability of the `branchProtectionRule` field on "Ref" before trying to request it from GraphQL. --- api/queries_pr.go | 61 +++++++++++++++++++++--------- api/queries_pr_test.go | 84 ++++++++++++++++++++++++++---------------- go.mod | 1 + go.sum | 1 + 4 files changed, 98 insertions(+), 49 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 0cc46477538..d3e8d149047 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -13,6 +13,7 @@ import ( "github.com/cli/cli/internal/ghinstance" "github.com/cli/cli/internal/ghrepo" "github.com/shurcooL/githubv4" + "golang.org/x/sync/errgroup" ) type PullRequestsPayload struct { @@ -232,14 +233,16 @@ func (c Client) PullRequestDiff(baseRepo ghrepo.Interface, prNumber int) (io.Rea } type pullRequestFeature struct { - HasReviewDecision bool - HasStatusCheckRollup bool + HasReviewDecision bool + HasStatusCheckRollup bool + HasBranchProtectionRule bool } func determinePullRequestFeatures(httpClient *http.Client, hostname string) (prFeatures pullRequestFeature, err error) { if !ghinstance.IsEnterprise(hostname) { prFeatures.HasReviewDecision = true prFeatures.HasStatusCheckRollup = true + prFeatures.HasBranchProtectionRule = true return } @@ -256,8 +259,26 @@ func determinePullRequestFeatures(httpClient *http.Client, hostname string) (prF } `graphql:"Commit: __type(name: \"Commit\")"` } + // needs to be a separate query because the backend only supports 2 `__type` expressions in one query + var featureDetection2 struct { + Ref struct { + Fields []struct { + Name string + } `graphql:"fields(includeDeprecated: true)"` + } `graphql:"Ref: __type(name: \"Ref\")"` + } + v4 := graphQLClient(httpClient, hostname) - err = v4.QueryNamed(context.Background(), "PullRequest_fields", &featureDetection, nil) + + g := new(errgroup.Group) + g.Go(func() error { + return v4.QueryNamed(context.Background(), "PullRequest_fields", &featureDetection, nil) + }) + g.Go(func() error { + return v4.QueryNamed(context.Background(), "PullRequest_fields2", &featureDetection2, nil) + }) + + err = g.Wait() if err != nil { return } @@ -274,6 +295,12 @@ func determinePullRequestFeatures(httpClient *http.Client, hostname string) (prF prFeatures.HasStatusCheckRollup = true } } + for _, field := range featureDetection2.Ref.Fields { + switch field.Name { + case "branchProtectionRule": + prFeatures.HasBranchProtectionRule = true + } + } return } @@ -333,6 +360,16 @@ func PullRequests(client *Client, repo ghrepo.Interface, currentPRNumber int, cu ` } + var requiresStrictStatusChecks string + if prFeatures.HasBranchProtectionRule { + requiresStrictStatusChecks = ` + baseRef { + branchProtectionRule { + requiresStrictStatusChecks + } + }` + } + fragments := fmt.Sprintf(` fragment pr on PullRequest { number @@ -344,11 +381,7 @@ func PullRequests(client *Client, repo ghrepo.Interface, currentPRNumber int, cu headRepositoryOwner { login } - baseRef { - branchProtectionRule { - requiresStrictStatusChecks - } - } + %s isCrossRepository isDraft %s @@ -357,16 +390,13 @@ func PullRequests(client *Client, repo ghrepo.Interface, currentPRNumber int, cu ...pr %s } - `, statusesFragment, reviewsFragment) + `, requiresStrictStatusChecks, statusesFragment, reviewsFragment) queryPrefix := ` query PullRequestStatus($owner: String!, $repo: String!, $headRefName: String!, $viewerQuery: String!, $reviewerQuery: String!, $per_page: Int = 10) { repository(owner: $owner, name: $repo) { defaultBranchRef { name - branchProtectionRule { - requiresStrictStatusChecks - } } pullRequests(headRefName: $headRefName, first: $per_page, orderBy: { field: CREATED_AT, direction: DESC }) { totalCount @@ -382,12 +412,9 @@ func PullRequests(client *Client, repo ghrepo.Interface, currentPRNumber int, cu queryPrefix = ` query PullRequestStatus($owner: String!, $repo: String!, $number: Int!, $viewerQuery: String!, $reviewerQuery: String!, $per_page: Int = 10) { repository(owner: $owner, name: $repo) { - defaultBranchRef { - name - branchProtectionRule { - requiresStrictStatusChecks + defaultBranchRef { + name } - } pullRequest(number: $number) { ...prWithReviews } diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 4e0c1581e86..5441be950db 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -52,7 +52,7 @@ func Test_determinePullRequestFeatures(t *testing.T) { tests := []struct { name string hostname string - queryResponse string + queryResponse map[string]string wantPrFeatures pullRequestFeature wantErr bool }{ @@ -60,58 +60,80 @@ func Test_determinePullRequestFeatures(t *testing.T) { name: "github.com", hostname: "github.com", wantPrFeatures: pullRequestFeature{ - HasReviewDecision: true, - HasStatusCheckRollup: true, + HasReviewDecision: true, + HasStatusCheckRollup: true, + HasBranchProtectionRule: true, }, wantErr: false, }, { name: "GHE empty response", hostname: "git.my.org", - queryResponse: heredoc.Doc(` - {"data": {}} - `), + queryResponse: map[string]string{ + `query PullRequest_fields\b`: `{"data": {}}`, + `query PullRequest_fields2\b`: `{"data": {}}`, + }, wantPrFeatures: pullRequestFeature{ - HasReviewDecision: false, - HasStatusCheckRollup: false, + HasReviewDecision: false, + HasStatusCheckRollup: false, + HasBranchProtectionRule: false, }, wantErr: false, }, { name: "GHE has reviewDecision", hostname: "git.my.org", - queryResponse: heredoc.Doc(` - {"data": { - "PullRequest": { - "fields": [ + queryResponse: map[string]string{ + `query PullRequest_fields\b`: heredoc.Doc(` + { "data": { "PullRequest": { "fields": [ {"name": "foo"}, {"name": "reviewDecision"} - ] - } - } } - `), + ] } } } + `), + `query PullRequest_fields2\b`: `{"data": {}}`, + }, wantPrFeatures: pullRequestFeature{ - HasReviewDecision: true, - HasStatusCheckRollup: false, + HasReviewDecision: true, + HasStatusCheckRollup: false, + HasBranchProtectionRule: false, }, wantErr: false, }, { name: "GHE has statusCheckRollup", hostname: "git.my.org", - queryResponse: heredoc.Doc(` - {"data": { - "Commit": { - "fields": [ + queryResponse: map[string]string{ + `query PullRequest_fields\b`: heredoc.Doc(` + { "data": { "Commit": { "fields": [ {"name": "foo"}, {"name": "statusCheckRollup"} - ] - } - } } - `), + ] } } } + `), + `query PullRequest_fields2\b`: `{"data": {}}`, + }, + wantPrFeatures: pullRequestFeature{ + HasReviewDecision: false, + HasStatusCheckRollup: true, + HasBranchProtectionRule: false, + }, + wantErr: false, + }, + { + name: "GHE has branchProtectionRule", + hostname: "git.my.org", + queryResponse: map[string]string{ + `query PullRequest_fields\b`: `{"data": {}}`, + `query PullRequest_fields2\b`: heredoc.Doc(` + { "data": { "Ref": { "fields": [ + {"name": "foo"}, + {"name": "branchProtectionRule"} + ] } } } + `), + }, wantPrFeatures: pullRequestFeature{ - HasReviewDecision: false, - HasStatusCheckRollup: true, + HasReviewDecision: false, + HasStatusCheckRollup: false, + HasBranchProtectionRule: true, }, wantErr: false, }, @@ -121,10 +143,8 @@ func Test_determinePullRequestFeatures(t *testing.T) { fakeHTTP := &httpmock.Registry{} httpClient := NewHTTPClient(ReplaceTripper(fakeHTTP)) - if tt.queryResponse != "" { - fakeHTTP.Register( - httpmock.GraphQL(`query PullRequest_fields\b`), - httpmock.StringResponse(tt.queryResponse)) + for query, resp := range tt.queryResponse { + fakeHTTP.Register(httpmock.GraphQL(query), httpmock.StringResponse(resp)) } gotPrFeatures, err := determinePullRequestFeatures(httpClient, tt.hostname) diff --git a/go.mod b/go.mod index 6de526901d7..2ddd3697b2c 100644 --- a/go.mod +++ b/go.mod @@ -29,6 +29,7 @@ require ( github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.6.1 golang.org/x/crypto v0.0.0-20201016220609-9e8e0b390897 + golang.org/x/sync v0.0.0-20190423024810-112230192c58 golang.org/x/text v0.3.4 // indirect gopkg.in/yaml.v3 v3.0.0-20200615113413-eeeca48fe776 ) diff --git a/go.sum b/go.sum index d2f73e24348..dfbd5af86bc 100644 --- a/go.sum +++ b/go.sum @@ -331,6 +331,7 @@ golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190227155943-e225da77a7e6/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20180823144017-11551d06cbcc/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=