From 8719c7a2db43d5bffdf74cc042de2e7ce1ccd65d Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Fri, 17 May 2024 15:22:50 -0500 Subject: [PATCH] Standardize error messages and url handling within NewGitHubClient Co-authored-by: Ryan Richard --- internal/githubclient/githubclient.go | 37 +++++++++++++--------- internal/githubclient/githubclient_test.go | 25 ++++++--------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/internal/githubclient/githubclient.go b/internal/githubclient/githubclient.go index 04e978507..807fe5ae4 100644 --- a/internal/githubclient/githubclient.go +++ b/internal/githubclient/githubclient.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" "net/http" + "net/url" + "strings" "github.com/google/go-github/v62/github" @@ -40,29 +42,34 @@ type githubClient struct { var _ GitHubInterface = (*githubClient)(nil) func NewGitHubClient(httpClient *http.Client, apiBaseURL, token string) (GitHubInterface, error) { + const errorPrefix = "unable to build new github client" + if httpClient == nil { - return nil, fmt.Errorf("httpClient cannot be nil") + return nil, fmt.Errorf("%s: httpClient cannot be nil", errorPrefix) + } + + parsedURL, err := url.Parse(apiBaseURL) + if err != nil { + return nil, fmt.Errorf("%s: %w", errorPrefix, err) + } + + if !strings.HasSuffix(parsedURL.Path, "/") { + parsedURL.Path += "/" + } + + if parsedURL.Scheme != "https" { + return nil, fmt.Errorf(`%s: apiBaseURL must use "https" protocol, found %q instead`, errorPrefix, parsedURL.Scheme) } if token == "" { - return nil, fmt.Errorf("token cannot be empty string") + return nil, fmt.Errorf("%s: token cannot be empty string", errorPrefix) } - if apiBaseURL == "https://github.com" { - apiBaseURL = "https://api.github.com/" - } - - client, err := github.NewClient(httpClient).WithEnterpriseURLs(apiBaseURL, "") - if err != nil { - return nil, fmt.Errorf("unable to create GitHub client using WithEnterpriseURLs: %w", err) - } - - if client.BaseURL.Scheme != "https" { - return nil, fmt.Errorf(`apiBaseURL must use "https" protocol, found "%s" instead`, client.BaseURL.Scheme) - } + client := github.NewClient(httpClient).WithAuthToken(token) + client.BaseURL = parsedURL return &githubClient{ - client: client.WithAuthToken(token), + client: client, }, nil } diff --git a/internal/githubclient/githubclient_test.go b/internal/githubclient/githubclient_test.go index e9214e7c8..36924fc8a 100644 --- a/internal/githubclient/githubclient_test.go +++ b/internal/githubclient/githubclient_test.go @@ -21,7 +21,7 @@ func TestNewGitHubClient(t *testing.T) { t.Run("rejects nil http client", func(t *testing.T) { _, err := NewGitHubClient(nil, "https://api.github.com/", "") - require.EqualError(t, err, "httpClient cannot be nil") + require.EqualError(t, err, "unable to build new github client: httpClient cannot be nil") }) tests := []struct { @@ -38,45 +38,39 @@ func TestNewGitHubClient(t *testing.T) { wantBaseURL: "https://api.github.com/", }, { - name: "happy path with https://api.github.com", + name: "adds trailing slash to path for https://api.github.com", apiBaseURL: "https://api.github.com", token: "other-token", wantBaseURL: "https://api.github.com/", }, { - name: "happy path with Enterprise URL https://fake.enterprise.tld", - apiBaseURL: "https://fake.enterprise.tld", + name: "adds trailing slash to path for Enterprise URL https://fake.enterprise.tld/api/v3", + apiBaseURL: "https://fake.enterprise.tld/api/v3", token: "some-enterprise-token", wantBaseURL: "https://fake.enterprise.tld/api/v3/", }, - { - name: "coerces https://github.com into https://api.github.com/", - apiBaseURL: "https://github.com", - token: "some-token", - wantBaseURL: "https://api.github.com/", - }, { name: "rejects apiBaseURL without https:// scheme", apiBaseURL: "scp://github.com", token: "some-token", - wantErr: `apiBaseURL must use "https" protocol, found "scp" instead`, + wantErr: `unable to build new github client: apiBaseURL must use "https" protocol, found "scp" instead`, }, { name: "rejects apiBaseURL with empty scheme", apiBaseURL: "github.com", token: "some-token", - wantErr: `apiBaseURL must use "https" protocol, found "" instead`, + wantErr: `unable to build new github client: apiBaseURL must use "https" protocol, found "" instead`, }, { name: "rejects empty token", apiBaseURL: "https://api.github.com/", - wantErr: "token cannot be empty string", + wantErr: "unable to build new github client: token cannot be empty string", }, { - name: "returns errors from WithEnterpriseURLs", + name: "returns errors from url.Parse", apiBaseURL: "https:// example.com", token: "some-token", - wantErr: `unable to create GitHub client using WithEnterpriseURLs: parse "https:// example.com": invalid character " " in host name`, + wantErr: `unable to build new github client: parse "https:// example.com": invalid character " " in host name`, }, } for _, test := range tests { @@ -111,7 +105,6 @@ func TestNewGitHubClient(t *testing.T) { require.True(t, ok) require.NotNil(t, actual.client.BaseURL) require.Equal(t, test.wantBaseURL, actual.client.BaseURL.String()) - //require.Equal(t, httpClient, actual.client.Client()) // Force the githubClient's httpClient roundTrippers to run and add the Authorization header _, err = actual.client.Client().Get(testServer.URL)