Repository: cli/cli
Pull request #8303 (2023-11-06 11:54:02)

Set default Accept header for API command when one is not specified
This PR sets a default Accept header for the `api` command when one is not specified by the user. The new default value is `*/*`, previously the value was an empty string. The empty string is not a valid Accept header for the GitHub API and thus the need for this change. As part of this change I was able to remove `SkipAcceptHeaders` from `HTTPClientOptions` which I think is a great improvement because that option was very opaque.

Fixes https://github.com/github/cli/issues/247


Patch:
diff --git a/api/http_client.go b/api/http_client.go
index 9f2d59ce3..fcf036008 100644
--- a/api/http_client.go
+++ b/api/http_client.go
@@ -17,14 +17,13 @@ type tokenGetter interface {
 }
 
 type HTTPClientOptions struct {
-	AppVersion        string
-	CacheTTL          time.Duration
-	Config            tokenGetter
-	EnableCache       bool
-	Log               io.Writer
-	LogColorize       bool
-	LogVerboseHTTP    bool
-	SkipAcceptHeaders bool
+	AppVersion     string
+	CacheTTL       time.Duration
+	Config         tokenGetter
+	EnableCache    bool
+	Log            io.Writer
+	LogColorize    bool
+	LogVerboseHTTP bool
 }
 
 func NewHTTPClient(opts HTTPClientOptions) (*http.Client, error) {
@@ -50,9 +49,6 @@ func NewHTTPClient(opts HTTPClientOptions) (*http.Client, error) {
 	headers := map[string]string{
 		userAgent: fmt.Sprintf("GitHub CLI %s", opts.AppVersion),
 	}
-	if opts.SkipAcceptHeaders {
-		headers[accept] = ""
-	}
 	clientOpts.Headers = headers
 
 	if opts.EnableCache {
diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go
index 38b6278b4..610f2f830 100644
--- a/pkg/cmd/api/api.go
+++ b/pkg/cmd/api/api.go
@@ -298,14 +298,13 @@ func apiRun(opts *ApiOptions) error {
 				log = opts.IO.Out
 			}
 			opts := api.HTTPClientOptions{
-				AppVersion:        opts.AppVersion,
-				CacheTTL:          opts.CacheTTL,
-				Config:            cfg.Authentication(),
-				EnableCache:       opts.CacheTTL > 0,
-				Log:               log,
-				LogColorize:       opts.IO.ColorEnabled(),
-				LogVerboseHTTP:    opts.Verbose,
-				SkipAcceptHeaders: true,
+				AppVersion:     opts.AppVersion,
+				CacheTTL:       opts.CacheTTL,
+				Config:         cfg.Authentication(),
+				EnableCache:    opts.CacheTTL > 0,
+				Log:            log,
+				LogColorize:    opts.IO.ColorEnabled(),
+				LogVerboseHTTP: opts.Verbose,
 			}
 			return api.NewHTTPClient(opts)
 		}
diff --git a/pkg/cmd/api/http.go b/pkg/cmd/api/http.go
index 3d6bb240e..b1b503cc2 100644
--- a/pkg/cmd/api/http.go
+++ b/pkg/cmd/api/http.go
@@ -74,6 +74,9 @@ func httpRequest(client *http.Client, hostname string, method string, p string,
 	if bodyIsJSON && req.Header.Get("Content-Type") == "" {
 		req.Header.Set("Content-Type", "application/json; charset=utf-8")
 	}
+	if req.Header.Get("Accept") == "" {
+		req.Header.Set("Accept", "*/*")
+	}
 
 	return client.Do(req)
 }
diff --git a/pkg/cmd/run/view/view.go b/pkg/cmd/run/view/view.go
index bc40b66ee..e77def23a 100644
--- a/pkg/cmd/run/view/view.go
+++ b/pkg/cmd/run/view/view.go
@@ -305,23 +305,14 @@ func runView(opts *ViewOptions) error {
 	}
 
 	var annotations []shared.Annotation
-
-	var annotationErr error
-	var as []shared.Annotation
 	for _, job := range jobs {
-		as, annotationErr = shared.GetAnnotations(client, repo, job)
-		if annotationErr != nil {
-			break
+		as, err := shared.GetAnnotations(client, repo, job)
+		if err != nil {
+			return fmt.Errorf("failed to get annotations: %w", err)
 		}
 		annotations = append(annotations, as...)
 	}
 
-	opts.IO.StopProgressIndicator()
-
-	if annotationErr != nil {
-		return fmt.Errorf("failed to get annotations: %w", annotationErr)
-	}
-
 	out := opts.IO.Out
 
 	fmt.Fprintln(out)


Test patch:
diff --git a/api/client_test.go b/api/client_test.go
index 0f36e3f69..1701a17a9 100644
--- a/api/client_test.go
+++ b/api/client_test.go
@@ -234,10 +234,9 @@ func TestHTTPHeaders(t *testing.T) {
 
 	ios, _, _, stderr := iostreams.Test()
 	httpClient, err := NewHTTPClient(HTTPClientOptions{
-		AppVersion:        "v1.2.3",
-		Config:            tinyConfig{ts.URL[7:] + ":oauth_token": "MYTOKEN"},
-		Log:               ios.ErrOut,
-		SkipAcceptHeaders: false,
+		AppVersion: "v1.2.3",
+		Config:     tinyConfig{ts.URL[7:] + ":oauth_token": "MYTOKEN"},
+		Log:        ios.ErrOut,
 	})
 	assert.NoError(t, err)
 	client := NewClientFromHTTP(httpClient)
diff --git a/api/http_client_test.go b/api/http_client_test.go
index b1e84e582..f8fe28d40 100644
--- a/api/http_client_test.go
+++ b/api/http_client_test.go
@@ -20,7 +20,6 @@ func TestNewHTTPClient(t *testing.T) {
 	type args struct {
 		config         tokenGetter
 		appVersion     string
-		setAccept      bool
 		logVerboseHTTP bool
 	}
 	tests := []struct {
@@ -31,11 +30,10 @@ func TestNewHTTPClient(t *testing.T) {
 		wantStderr string
 	}{
 		{
-			name: "github.com with Accept header",
+			name: "github.com",
 			args: args{
 				config:         tinyConfig{"github.com:oauth_token": "MYTOKEN"},
 				appVersion:     "v1.2.3",
-				setAccept:      true,
 				logVerboseHTTP: false,
 			},
 			host: "github.com",
@@ -47,18 +45,16 @@ func TestNewHTTPClient(t *testing.T) {
 			wantStderr: "",
 		},
 		{
-			name: "github.com no Accept header",
+			name: "GHES",
 			args: args{
-				config:         tinyConfig{"github.com:oauth_token": "MYTOKEN"},
-				appVersion:     "v1.2.3",
-				setAccept:      false,
-				logVerboseHTTP: false,
+				config:     tinyConfig{"example.com:oauth_token": "GHETOKEN"},
+				appVersion: "v1.2.3",
 			},
-			host: "github.com",
+			host: "example.com",
 			wantHeader: map[string]string{
-				"authorization": "token MYTOKEN",
+				"authorization": "token GHETOKEN",
 				"user-agent":    "GitHub CLI v1.2.3",
-				"accept":        "",
+				"accept":        "application/vnd.github.merge-info-preview+json, application/vnd.github.nebula-preview",
 			},
 			wantStderr: "",
 		},
@@ -67,7 +63,6 @@ func TestNewHTTPClient(t *testing.T) {
 			args: args{
 				config:         tinyConfig{"example.com:oauth_token": "MYTOKEN"},
 				appVersion:     "v1.2.3",
-				setAccept:      true,
 				logVerboseHTTP: false,
 			},
 			host: "github.com",
@@ -78,12 +73,26 @@ func TestNewHTTPClient(t *testing.T) {
 			},
 			wantStderr: "",
 		},
+		{
+			name: "GHES no authentication token",
+			args: args{
+				config:         tinyConfig{"github.com:oauth_token": "MYTOKEN"},
+				appVersion:     "v1.2.3",
+				logVerboseHTTP: false,
+			},
+			host: "example.com",
+			wantHeader: map[string]string{
+				"authorization": "",
+				"user-agent":    "GitHub CLI v1.2.3",
+				"accept":        "application/vnd.github.merge-info-preview+json, application/vnd.github.nebula-preview",
+			},
+			wantStderr: "",
+		},
 		{
 			name: "github.com in verbose mode",
 			args: args{
 				config:         tinyConfig{"github.com:oauth_token": "MYTOKEN"},
 				appVersion:     "v1.2.3",
-				setAccept:      true,
 				logVerboseHTTP: true,
 			},
 			host: "github.com",
@@ -109,21 +118,6 @@ func TestNewHTTPClient(t *testing.T) {
 				* Request took <duration>
 			`),
 		},
-		{
-			name: "GHES Accept header",
-			args: args{
-				config:     tinyConfig{"example.com:oauth_token": "GHETOKEN"},
-				appVersion: "v1.2.3",
-				setAccept:  true,
-			},
-			host: "example.com",
-			wantHeader: map[string]string{
-				"authorization": "token GHETOKEN",
-				"user-agent":    "GitHub CLI v1.2.3",
-				"accept":        "application/vnd.github.merge-info-preview+json, application/vnd.github.nebula-preview",
-			},
-			wantStderr: "",
-		},
 	}
 
 	var gotReq *http.Request
@@ -137,11 +131,10 @@ func TestNewHTTPClient(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			ios, _, _, stderr := iostreams.Test()
 			client, err := NewHTTPClient(HTTPClientOptions{
-				AppVersion:        tt.args.appVersion,
-				Config:            tt.args.config,
-				Log:               ios.ErrOut,
-				SkipAcceptHeaders: !tt.args.setAccept,
-				LogVerboseHTTP:    tt.args.logVerboseHTTP,
+				AppVersion:     tt.args.appVersion,
+				Config:         tt.args.config,
+				Log:            ios.ErrOut,
+				LogVerboseHTTP: tt.args.logVerboseHTTP,
 			})
 			require.NoError(t, err)
 
diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go
index 178bebd86..3ffc6a0b3 100644
--- a/pkg/cmd/api/api_test.go
+++ b/pkg/cmd/api/api_test.go
@@ -1455,3 +1455,58 @@ func Test_parseErrorResponse(t *testing.T) {
 		})
 	}
 }
+
+func Test_apiRun_acceptHeader(t *testing.T) {
+	tests := []struct {
+		name             string
+		options          ApiOptions
+		wantAcceptHeader string
+	}{
+		{
+			name:             "sets default accept header",
+			options:          ApiOptions{},
+			wantAcceptHeader: "*/*",
+		},
+		{
+			name: "does not override user accept header",
+			options: ApiOptions{
+				RequestHeaders: []string{"Accept: testing"},
+			},
+			wantAcceptHeader: "testing",
+		},
+		{
+			name: "does not override preview names",
+			options: ApiOptions{
+				Previews: []string{"nebula"},
+			},
+			wantAcceptHeader: "application/vnd.github.nebula-preview+json",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ios, _, _, _ := iostreams.Test()
+			tt.options.IO = ios
+
+			tt.options.Config = func() (config.Config, error) {
+				return config.NewBlankConfig(), nil
+			}
+
+			var gotReq *http.Request
+			tt.options.HttpClient = func() (*http.Client, error) {
+				var tr roundTripper = func(req *http.Request) (*http.Response, error) {
+					gotReq = req
+					resp := &http.Response{
+						StatusCode: 200,
+						Request:    req,
+						Body:       io.NopCloser(bytes.NewBufferString("")),
+					}
+					return resp, nil
+				}
+				return &http.Client{Transport: tr}, nil
+			}
+
+			assert.NoError(t, apiRun(&tt.options))
+			assert.Equal(t, tt.wantAcceptHeader, gotReq.Header.Get("Accept"))
+		})
+	}
+}
diff --git a/pkg/cmd/api/http_test.go b/pkg/cmd/api/http_test.go
index f63566490..2778ea38c 100644
--- a/pkg/cmd/api/http_test.go
+++ b/pkg/cmd/api/http_test.go
@@ -126,7 +126,25 @@ func Test_httpRequest(t *testing.T) {
 				method:  "GET",
 				u:       "https://api.github.com/repos/octocat/spoon-knife",
 				body:    "",
-				headers: "",
+				headers: "Accept: */*\r\n",
+			},
+		},
+		{
+			name: "GET with accept header",
+			args: args{
+				client:  &httpClient,
+				host:    "github.com",
+				method:  "GET",
+				p:       "repos/octocat/spoon-knife",
+				params:  nil,
+				headers: []string{"Accept: testing"},
+			},
+			wantErr: false,
+			want: expects{
+				method:  "GET",
+				u:       "https://api.github.com/repos/octocat/spoon-knife",
+				body:    "",
+				headers: "Accept: testing\r\n",
 			},
 		},
 		{
@@ -144,7 +162,7 @@ func Test_httpRequest(t *testing.T) {
 				method:  "GET",
 				u:       "https://api.github.com/repos/octocat/spoon-knife",
 				body:    "",
-				headers: "",
+				headers: "Accept: */*\r\n",
 			},
 		},
 		{
@@ -162,7 +180,7 @@ func Test_httpRequest(t *testing.T) {
 				method:  "GET",
 				u:       "https://api.github.com/repos/octocat/spoon-knife",
 				body:    "",
-				headers: "",
+				headers: "Accept: */*\r\n",
 			},
 		},
 		{
@@ -180,7 +198,7 @@ func Test_httpRequest(t *testing.T) {
 				method:  "GET",
 				u:       "https://example.org/api/v3/repos/octocat/spoon-knife",
 				body:    "",
-				headers: "",
+				headers: "Accept: */*\r\n",
 			},
 		},
 		{
@@ -200,7 +218,7 @@ func Test_httpRequest(t *testing.T) {
 				method:  "GET",
 				u:       "https://api.github.com/repos/octocat/spoon-knife?a=b",
 				body:    "",
-				headers: "",
+				headers: "Accept: */*\r\n",
 			},
 		},
 		{
@@ -220,7 +238,7 @@ func Test_httpRequest(t *testing.T) {
 				method:  "POST",
 				u:       "https://api.github.com/repos",
 				body:    `{"a":"b"}`,
-				headers: "Content-Type: application/json; charset=utf-8\r\n",
+				headers: "Accept: */*\r\nContent-Type: application/json; charset=utf-8\r\n",
 			},
 		},
 		{
@@ -240,7 +258,7 @@ func Test_httpRequest(t *testing.T) {
 				method:  "POST",
 				u:       "https://api.github.com/graphql",
 				body:    `{"variables":{"a":"b"}}`,
-				headers: "Content-Type: application/json; charset=utf-8\r\n",
+				headers: "Accept: */*\r\nContent-Type: application/json; charset=utf-8\r\n",
 			},
 		},
 		{
@@ -258,7 +276,7 @@ func Test_httpRequest(t *testing.T) {
 				method:  "POST",
 				u:       "https://example.org/api/graphql",
 				body:    `{}`,
-				headers: "Content-Type: application/json; charset=utf-8\r\n",
+				headers: "Accept: */*\r\nContent-Type: application/json; charset=utf-8\r\n",
 			},
 		},
 		{
diff --git a/pkg/cmd/run/view/view_test.go b/pkg/cmd/run/view/view_test.go
index 86aab619b..ac318b0c9 100644
--- a/pkg/cmd/run/view/view_test.go
+++ b/pkg/cmd/run/view/view_test.go
@@ -1279,6 +1279,40 @@ func TestViewRun(t *testing.T) {
 			},
 			wantOut: "fetched 5 jobs\n",
 		},
+		{
+			name: "Returns error when failing to get annotations",
+			opts: &ViewOptions{
+				RunID:      "1234",
+				ExitStatus: true,
+			},
+			httpStubs: func(reg *httpmock.Registry) {
+				reg.Register(
+					httpmock.REST("GET", "repos/OWNER/REPO/actions/runs/1234"),
+					httpmock.JSONResponse(shared.FailedRun))
+				reg.Register(
+					httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/123"),
+					httpmock.JSONResponse(shared.TestWorkflow))
+				reg.Register(
+					httpmock.REST("GET", "repos/OWNER/REPO/actions/runs/1234/artifacts"),
+					httpmock.StringResponse(`{}`))
+				reg.Register(
+					httpmock.GraphQL(`query PullRequestForRun`),
+					httpmock.StringResponse(``))
+				reg.Register(
+					httpmock.REST("GET", "runs/1234/jobs"),
+					httpmock.JSONResponse(shared.JobsPayload{
+						Jobs: []shared.Job{
+							shared.FailedJob,
+						},
+					}))
+				reg.Register(
+					httpmock.REST("GET", "repos/OWNER/REPO/check-runs/20/annotations"),
+					httpmock.StatusStringResponse(500, "internal server error"),
+				)
+			},
+			errMsg:  "failed to get annotations: HTTP 500 (https://api.github.com/repos/OWNER/REPO/check-runs/20/annotations)",
+			wantErr: true,
+		},
 	}
 
 	for _, tt := range tests {