From 555b1c80e30fe863deb83005fa19d0e7aef84751 Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Fri, 17 May 2024 14:07:23 -0500 Subject: [PATCH] Use passed-in context Co-authored-by: Ryan Richard --- internal/githubclient/githubclient.go | 22 ++++---- internal/githubclient/githubclient_test.go | 60 ++++++++++++++++++++-- 2 files changed, 67 insertions(+), 15 deletions(-) diff --git a/internal/githubclient/githubclient.go b/internal/githubclient/githubclient.go index 62d1b00a9..0832e1b83 100644 --- a/internal/githubclient/githubclient.go +++ b/internal/githubclient/githubclient.go @@ -24,9 +24,9 @@ type TeamInfo struct { } type GitHubInterface interface { - GetUserInfo() (*UserInfo, error) - GetOrgMembership() ([]string, error) - GetTeamMembership(allowedOrganizations sets.Set[string]) ([]TeamInfo, error) + GetUserInfo(ctx context.Context) (*UserInfo, error) + GetOrgMembership(ctx context.Context) ([]string, error) + GetTeamMembership(ctx context.Context, allowedOrganizations sets.Set[string]) ([]TeamInfo, error) } type githubClient struct { @@ -63,9 +63,9 @@ func NewGitHubClient(httpClient *http.Client, apiBaseURL, token string) (GitHubI } // GetUserInfo returns the "Login" and "ID" attributes of the logged-in user. -// TODO: where should context come from? -func (g *githubClient) GetUserInfo() (*UserInfo, error) { - user, response, err := g.client.Users.Get(context.Background(), emptyUserMeansTheAuthenticatedUser) +// TODO: should we check ID and Login for nil? +func (g *githubClient) GetUserInfo(ctx context.Context) (*UserInfo, error) { + user, response, err := g.client.Users.Get(ctx, emptyUserMeansTheAuthenticatedUser) if err != nil { return nil, fmt.Errorf("error fetching authenticated user: %w", err) } @@ -89,16 +89,15 @@ func (g *githubClient) GetUserInfo() (*UserInfo, error) { } // GetOrgMembership returns an array of the "Login" attributes for all organizations to which the authenticated user belongs. -// TODO: where should context come from? // TODO: what happens if login is nil? // TODO: what is a good page size? -func (g *githubClient) GetOrgMembership() ([]string, error) { +func (g *githubClient) GetOrgMembership(ctx context.Context) ([]string, error) { organizationLoginStrings := make([]string, 0) opt := &github.ListOptions{PerPage: 10} // get all pages of results for { - organizationResults, response, err := g.client.Organizations.List(context.Background(), emptyUserMeansTheAuthenticatedUser, opt) + organizationResults, response, err := g.client.Organizations.List(ctx, emptyUserMeansTheAuthenticatedUser, opt) if err != nil { return nil, fmt.Errorf("error fetching organizations for authenticated user: %w", err) } @@ -122,16 +121,15 @@ func isOrgAllowed(allowedOrganizations sets.Set[string], login string) bool { // GetTeamMembership returns a description of each team to which the authenticated user belongs. // If allowedOrganizations is not empty, will filter the results to only those teams which belong to the allowed organizations. // Parent teams will also be returned. -// TODO: where should context come from? // TODO: what happens if org or login or id are nil? // TODO: what is a good page size? -func (g *githubClient) GetTeamMembership(allowedOrganizations sets.Set[string]) ([]TeamInfo, error) { +func (g *githubClient) GetTeamMembership(ctx context.Context, allowedOrganizations sets.Set[string]) ([]TeamInfo, error) { teamInfos := make([]TeamInfo, 0) opt := &github.ListOptions{PerPage: 10} // get all pages of results for { - teamsResults, response, err := g.client.Teams.ListUserTeams(context.Background(), opt) + teamsResults, response, err := g.client.Teams.ListUserTeams(ctx, opt) if err != nil { return nil, fmt.Errorf("error fetching team membership for authenticated user: %w", err) } diff --git a/internal/githubclient/githubclient_test.go b/internal/githubclient/githubclient_test.go index 716e3340d..7e2c6987c 100644 --- a/internal/githubclient/githubclient_test.go +++ b/internal/githubclient/githubclient_test.go @@ -1,6 +1,7 @@ package githubclient import ( + "context" "net/http" "strings" "testing" @@ -126,6 +127,7 @@ func TestGetUser(t *testing.T) { name string httpClient *http.Client token string + ctx context.Context wantErr string wantUserInfo UserInfo }{ @@ -191,6 +193,17 @@ func TestGetUser(t *testing.T) { token: "does-this-token-work", wantErr: `the "ID" attribute is missing for authenticated user`, }, + { + name: "passes the context parameter into the API call", + token: "some-token", + httpClient: mock.NewMockedHTTPClient(), + ctx: func() context.Context { + canceledCtx, cancel := context.WithCancel(context.Background()) + cancel() + return canceledCtx + }(), + wantErr: "error fetching authenticated user: context canceled", + }, { name: "returns errors from the API", httpClient: mock.NewMockedHTTPClient( @@ -216,7 +229,13 @@ func TestGetUser(t *testing.T) { githubClient := &githubClient{ client: github.NewClient(test.httpClient).WithAuthToken(test.token), } - actual, err := githubClient.GetUserInfo() + + ctx := context.Background() + if test.ctx != nil { + ctx = test.ctx + } + + actual, err := githubClient.GetUserInfo(ctx) if test.wantErr != "" { rt, ok := test.httpClient.Transport.(*mock.EnforceHostRoundTripper) require.True(t, ok) @@ -238,6 +257,7 @@ func TestGetOrgMembership(t *testing.T) { name string httpClient *http.Client token string + ctx context.Context wantErr string wantOrgs []string }{ @@ -292,6 +312,17 @@ func TestGetOrgMembership(t *testing.T) { token: "does-this-token-work", wantOrgs: []string{"some-org-to-which-the-authenticated-user-belongs"}, }, + { + name: "passes the context parameter into the API call", + token: "some-token", + httpClient: mock.NewMockedHTTPClient(), + ctx: func() context.Context { + canceledCtx, cancel := context.WithCancel(context.Background()) + cancel() + return canceledCtx + }(), + wantErr: "error fetching organizations for authenticated user: context canceled", + }, { name: "returns errors from the API", httpClient: mock.NewMockedHTTPClient( @@ -317,7 +348,13 @@ func TestGetOrgMembership(t *testing.T) { githubClient := &githubClient{ client: github.NewClient(test.httpClient).WithAuthToken(test.token), } - actual, err := githubClient.GetOrgMembership() + + ctx := context.Background() + if test.ctx != nil { + ctx = test.ctx + } + + actual, err := githubClient.GetOrgMembership(ctx) if test.wantErr != "" { rt, ok := test.httpClient.Transport.(*mock.EnforceHostRoundTripper) require.True(t, ok) @@ -339,6 +376,7 @@ func TestGetTeamMembership(t *testing.T) { name string httpClient *http.Client token string + ctx context.Context allowedOrganizations []string wantErr string wantTeams []TeamInfo @@ -647,6 +685,17 @@ func TestGetTeamMembership(t *testing.T) { }, }, }, + { + name: "passes the context parameter into the API call", + token: "some-token", + httpClient: mock.NewMockedHTTPClient(), + ctx: func() context.Context { + canceledCtx, cancel := context.WithCancel(context.Background()) + cancel() + return canceledCtx + }(), + wantErr: "error fetching team membership for authenticated user: context canceled", + }, { name: "returns errors from the API", httpClient: mock.NewMockedHTTPClient( @@ -673,7 +722,12 @@ func TestGetTeamMembership(t *testing.T) { client: github.NewClient(test.httpClient).WithAuthToken(test.token), } - actual, err := githubClient.GetTeamMembership(sets.New[string](test.allowedOrganizations...)) + ctx := context.Background() + if test.ctx != nil { + ctx = test.ctx + } + + actual, err := githubClient.GetTeamMembership(ctx, sets.New[string](test.allowedOrganizations...)) if test.wantErr != "" { rt, ok := test.httpClient.Transport.(*mock.EnforceHostRoundTripper) require.True(t, ok)