Set default Accept header for API command when one is not specified by samcoe · Pull Request #8303 · cli/cli · GitHub
Skip to content

Set default Accept header for API command when one is not specified #8303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
samcoe merged 1 commit into from
Nov 6, 2023
Merged

Set default Accept header for API command when one is not specified #8303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions api/client_test.go
View file
Open in desktop
18 changes: 7 additions & 11 deletions api/http_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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 {
Expand Down
59 changes: 26 additions & 33 deletions api/http_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ func TestNewHTTPClient(t *testing.T) {
type args struct {
config tokenGetter
appVersion string
setAccept bool
logVerboseHTTP bool
}
tests := []struct {
Expand All @@ -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",
Expand All @@ -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: "",
},
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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
Expand All @@ -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)

Expand Down
15 changes: 7 additions & 8 deletions pkg/cmd/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
55 changes: 55 additions & 0 deletions pkg/cmd/api/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
})
}
}
3 changes: 3 additions & 0 deletions pkg/cmd/api/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
34 changes: 26 additions & 8 deletions pkg/cmd/api/http_test.go
View file
Open in desktop