Support passing httptest URLs in api.ClientOptions

[mntlty]

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

go-gh/pkg/api/client_options.go

Line 38 in 25db6b9

Host string

for certain values the argument is returned unmodified

go-gh/pkg/api/rest_client.go

Lines 151 to 154 in 25db6b9

	func restURL(hostname string, pathOrURL string) string {
	if strings.HasPrefix(pathOrURL, "https://") || strings.HasPrefix(pathOrURL, "http://") {
	return pathOrURL
	}

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

go-gh/pkg/api/rest_client.go

Line 163 in 25db6b9

if isEnterprise(hostname) {

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]

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?