cli / go-gh

A Go module for interacting with gh and the GitHub API from the command line.
https://pkg.go.dev/github.com/cli/go-gh/v2
MIT License
322 stars 45 forks source link

Support passing httptest URLs in api.ClientOptions #167

Open mntlty opened 3 weeks ago

mntlty commented 3 weeks ago

The standard library's httptest package makes stubbing APIs easy, if the client calls the test server's URL:

package main

import (
    "fmt"
    "net/http"
    "net/http/httptest"
)

func main() {
    tls := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
    defer tls.Close()
    fmt.Println("tls hostname - ", tls.URL)
    // prints: tls hostname - https://127.0.0.1:42002

    ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
    defer ts.Close()
    fmt.Println("ts hostname - ", ts.URL)
    // prints: ts hostname -  http://127.0.0.1:35061
}

In api.ClientOptions, one of the configuration fields is Host https://github.com/cli/go-gh/blob/25db6b99518c88e03f71dbe9e58397c4cfb62caf/pkg/api/client_options.go#L38

for certain values the argument is returned unmodified https://github.com/cli/go-gh/blob/25db6b99518c88e03f71dbe9e58397c4cfb62caf/pkg/api/rest_client.go#L151-L154

I would like to propose adding another condition to the restURL function which would return the unmodified httptest server URL

if strings.HasPrefix(hostname, "https://127.0.0.1:") || strings.HasPrefix(hostname, "http://127.0.0.1:") {
    return hostname
}

This aligns with the conditional check on the prefix for the pathOrURL argument.

For reference, setting the ts.URL value to Host results in a URL similar to:

https://http//127.0.0.1:56261/api/v3/

as it satisfies this condition https://github.com/cli/go-gh/blob/25db6b99518c88e03f71dbe9e58397c4cfb62caf/pkg/api/rest_client.go#L163

I don't know if this means my proposed solution would break with Enterprises.

In writing this all out, I also realized that passing the hostname + path to client.Get, rather than just the api path, solves this issue.

andyfeller commented 3 weeks ago

Appreciate ideas for making it easier for extension authors to build better tests, a sentiment I believe @williammartin and I both support! ❤

I'm hesitant putting localhost/127.0.0.1 specific logic into restUrl as such URLs should be URLs and return back unmodified from the existing logic.

That said, I can see the challenge behind convincing the api logic to treat a given URL as a GHES or GitHub tenancy host 🤔 Possibly a host type override option?