Update go buildkite (#336)

* Update go-buildkite to v3.12

* Update factory to use new functional constructor for buildkite client

* Use httptest instead of shortcutting roundtrippers for HTTP testing

Using roundtrippers to modify HTTP requests for testing is pretty confusing, especially when there's a great testing tool right there for us to use!

This also lets us completely remove the factory's `HttpClient` member

cmd/bk/main.go

@@ -17,7 +17,11 @@ func main() {
 
 func mainRun() int {
 	ctx := context.Background()
-	f := factory.New(version.Version)
+	f, err := factory.New(version.Version)
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "failed to create factory: %s\n", err)
+		return 1
+	}
 
 	rootCmd, err := root.NewCmdRoot(f)
 	if err != nil {

go.mod

@@ -1,13 +1,13 @@
 module github.com/buildkite/cli/v3
 
-go 1.21
+go 1.22
 
-toolchain go1.21.6
+toolchain go1.22.5
 
 require (
 	github.com/AlecAivazis/survey/v2 v2.3.7
 	github.com/MakeNowJust/heredoc v1.0.0
-	github.com/buildkite/go-buildkite/v3 v3.11.0
+	github.com/buildkite/go-buildkite/v3 v3.12.0
 	github.com/charmbracelet/bubbles v0.18.0
 	github.com/charmbracelet/bubbletea v0.26.6
 	github.com/charmbracelet/huh v0.5.2

go.sum

@@ -40,6 +40,8 @@ github.com/bradleyjkemp/cupaloy/v2 v2.6.0 h1:knToPYa2xtfg42U3I6punFEjaGFKWQRXJwj
 github.com/bradleyjkemp/cupaloy/v2 v2.6.0/go.mod h1:bm7JXdkRd4BHJk9HpwqAI8BoAY1lps46Enkdqw6aRX0=
 github.com/buildkite/go-buildkite/v3 v3.11.0 h1:A43KDOuNczqrY8wqlsHNtPoYbgWXYC/slkB/2JYXr5E=
 github.com/buildkite/go-buildkite/v3 v3.11.0/go.mod h1:TmZggyr5HqkOhNbTrcdOdmwuYbQqcfwr9MSyKyMQWAA=
+github.com/buildkite/go-buildkite/v3 v3.12.0 h1:VkJVwqx+5GD1434eyf2YuX3/LuCzSUlOycSvUA7YW04=
+github.com/buildkite/go-buildkite/v3 v3.12.0/go.mod h1:ux2rjcqNoE54wfFHO3Vlet+a57Tt2jqOqfc9Afs7R7g=
 github.com/bwesterb/go-ristretto v1.2.3/go.mod h1:fUIoIZaG73pV5biE2Blr2xEzDoMj7NFEuV9ekS419A0=
 github.com/catppuccin/go v0.2.0 h1:ktBeIrIP42b/8FGiScP9sgrWOss3lw0Z5SktRoithGA=
 github.com/catppuccin/go v0.2.0/go.mod h1:8IHJuMGaUUjQM82qBrGNBv7LFq6JI3NnQCF6MOlZjpc=

internal/build/resolver/current_user_test.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"io"
 	"net/http"
+	"net/http/httptest"
 	"os"
 	"strings"
 	"testing"
@@ -30,20 +31,21 @@ func TestResolveBuildForCurrentUser(t *testing.T) {
 	t.Run("Errors if user cannot be found", func(t *testing.T) {
 		t.Parallel()
 
-		// mock a failed repsonse
-		transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
-			return &http.Response{
-				StatusCode: http.StatusNotFound,
-			}, nil
-		})
-		client := &http.Client{Transport: transport}
-		f := &factory.Factory{
-			RestAPIClient: buildkite.NewClient(client),
+		s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.WriteHeader(http.StatusNotFound)
+		}))
+		t.Cleanup(s.Close)
+
+		apiClient, err := buildkite.NewOpts(buildkite.WithBaseURL(s.URL))
+		if err != nil {
+			t.Fatal(err)
 		}
 
-		r := resolver.ResolveBuildForCurrentUser("main", pipelineResolver, f)
-		_, err := r(context.Background())
+		// mock a failed repsonse
+		f := &factory.Factory{RestAPIClient: apiClient}
 
+		r := resolver.ResolveBuildForCurrentUser("main", pipelineResolver, f)
+		_, err = r(context.Background())
 		if err == nil {
 			t.Fatal("Resolver should return error if user not found")
 		}
@@ -72,16 +74,24 @@ func TestResolveBuildForCurrentUser(t *testing.T) {
 				Body:       io.NopCloser(bytes.NewReader(in)),
 			},
 		}
+
 		// mock a failed repsonse
-		transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
+		s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			resp := responses[callIndex]
 			callIndex++
-			return &resp, nil
-		})
-		client := &http.Client{Transport: transport}
+			w.WriteHeader(resp.StatusCode)
+			io.Copy(w, resp.Body)
+		}))
+		t.Cleanup(s.Close)
+
+		apiClient, err := buildkite.NewOpts(buildkite.WithBaseURL(s.URL))
+		if err != nil {
+			t.Fatal(err)
+		}
+
 		fs := afero.NewMemMapFs()
 		f := &factory.Factory{
-			RestAPIClient: buildkite.NewClient(client),
+			RestAPIClient: apiClient,
 			Config:        config.New(fs, nil),
 		}
 
@@ -118,16 +128,24 @@ func TestResolveBuildForCurrentUser(t *testing.T) {
 				Body:       io.NopCloser(strings.NewReader("[]")),
 			},
 		}
+
 		// mock a failed repsonse
-		transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
+		s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			resp := responses[callIndex]
 			callIndex++
-			return &resp, nil
-		})
-		client := &http.Client{Transport: transport}
+			w.WriteHeader(resp.StatusCode)
+			io.Copy(w, resp.Body)
+		}))
+		t.Cleanup(s.Close)
+
+		apiClient, err := buildkite.NewOpts(buildkite.WithBaseURL(s.URL))
+		if err != nil {
+			t.Fatal(err)
+		}
+
 		fs := afero.NewMemMapFs()
 		f := &factory.Factory{
-			RestAPIClient: buildkite.NewClient(client),
+			RestAPIClient: apiClient,
 			Config:        config.New(fs, nil),
 		}
 

internal/build/resolver/user_builds_test.go

@@ -3,9 +3,8 @@ package resolver_test
 import (
 	"context"
 	"errors"
-	"io"
 	"net/http"
-	"strings"
+	"net/http/httptest"
 	"testing"
 
 	"github.com/buildkite/cli/v3/internal/build/resolver"
@@ -24,15 +23,18 @@ func TestResolveBuildFromUserId(t *testing.T) {
 		return nil, errors.New("")
 	}
 
-	transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
-		return &http.Response{
-			StatusCode: http.StatusOK,
-			Body:       io.NopCloser(strings.NewReader(``)),
-		}, nil
-	})
-	client := &http.Client{Transport: transport}
+	s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	t.Cleanup(s.Close)
+
+	apiClient, err := buildkite.NewOpts(buildkite.WithBaseURL(s.URL))
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	f := &factory.Factory{
-		RestAPIClient: buildkite.NewClient(client),
+		RestAPIClient: apiClient,
 	}
 
 	t.Run("Errors if pipeline cannot be resolved", func(t *testing.T) {
@@ -55,9 +57,3 @@ func TestResolveBuildFromUserId(t *testing.T) {
 		}
 	})
 }
-
-type roundTripperFunc func(*http.Request) (*http.Response, error)
-
-func (fn roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) {
-	return fn(r)
-}

internal/build/resolver/userid_test.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"io"
 	"net/http"
+	"net/http/httptest"
 	"os"
 	"strings"
 	"testing"
@@ -30,20 +31,24 @@ func TestResolveBuildFromUserUUID(t *testing.T) {
 	t.Run("Errors when user id is not a member of the organization", func(t *testing.T) {
 		t.Parallel()
 		// mock a failed repsonse
-		transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
-			return &http.Response{
-				StatusCode: http.StatusNotFound,
-			}, nil
-		})
-		client := &http.Client{Transport: transport}
+		s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.WriteHeader(http.StatusNotFound)
+		}))
+		t.Cleanup(s.Close)
+
+		apiClient, err := buildkite.NewOpts(buildkite.WithBaseURL(s.URL))
+		if err != nil {
+			t.Fatal(err)
+		}
+
 		fs := afero.NewMemMapFs()
 		f := &factory.Factory{
-			RestAPIClient: buildkite.NewClient(client),
+			RestAPIClient: apiClient,
 			Config:        config.New(fs, nil),
 		}
 
 		r := resolver.ResolveBuildForUserID("1234", pipelineResolver, f)
-		_, err := r(context.Background())
+		_, err = r(context.Background())
 
 		if err == nil {
 			t.Fatal("Resolver should return error if user not found")
@@ -59,30 +64,37 @@ func TestResolveBuildFromUserUUID(t *testing.T) {
 			{
 				StatusCode: http.StatusOK,
 				Body: io.NopCloser(strings.NewReader(`[{
-					"id": "abc123-4567-8910-...", 
+					"id": "abc123-4567-8910-...",
 					"number": 584,
 					"creator": {
 					"id": "0183c4e6-c88c-xxxx-b15e-7801077a9181",
 					"graphql_id": "VXNlci0tLTAxODNjNGU2LWM4OGxxxxxxxxxiMTVlLTc4MDEwNzdhOTE4MQ=="
-					}					
+					}
 					}]`)),
 			},
 			{
 				StatusCode: http.StatusOK,
 				Body:       io.NopCloser(bytes.NewReader(in)),
 			},
 		}
+
 		// mock a failed repsonse
-		transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
+		s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			resp := responses[callIndex]
 			callIndex++
-			return &resp, nil
-		})
+			w.WriteHeader(resp.StatusCode)
+			io.Copy(w, resp.Body)
+		}))
+		t.Cleanup(s.Close)
+
+		apiClient, err := buildkite.NewOpts(buildkite.WithBaseURL(s.URL))
+		if err != nil {
+			t.Fatal(err)
+		}
 
-		client := &http.Client{Transport: transport}
 		fs := afero.NewMemMapFs()
 		f := &factory.Factory{
-			RestAPIClient: buildkite.NewClient(client),
+			RestAPIClient: apiClient,
 			Config:        config.New(fs, nil),
 		}