Factor out a high-level githubClient interface for all operations.

github/auth.go

@@ -305,6 +305,9 @@ func buildTransportChain(ctx context.Context, opts *clientOptions) (http.RoundTr
 
 	// If a custom roundtripper was set, pipe it through the transport too
 	if opts.RoundTripperFactory != nil {
+		// TODO: Document usage of a nil transport here
+		// TODO: Provide some kind of debug logging if/when the httpcache is used
+		// One can see if the request hit the cache using: resp.Header[httpcache.XFromCache]
 		customTransport := opts.RoundTripperFactory.Transport(transport)
 		if customTransport == nil {
 			// The lint failure here is a false positive, for some (unknown) reason

github/client.go

@@ -26,7 +26,8 @@ import (
 const ProviderID = gitprovider.ProviderID("github")
 
 func newClient(c *github.Client, domain string, destructiveActions bool) *Client {
-	ctx := &clientContext{c, domain, destructiveActions}
+	ghClient := &githubClientImpl{c, destructiveActions}
+	ctx := &clientContext{ghClient, domain, destructiveActions}
 	return &Client{
 		clientContext: ctx,
 		orgs: &OrganizationsClient{
@@ -42,7 +43,7 @@ func newClient(c *github.Client, domain string, destructiveActions bool) *Client
 }
 
 type clientContext struct {
-	c                  *github.Client
+	c                  githubClient
 	domain             string
 	destructiveActions bool
 }

github/client_organization_teams.go

@@ -18,7 +18,6 @@ package github
 
 import (
 	"context"
-	"fmt"
 
 	"github.com/google/go-github/v32/github"
 
@@ -41,25 +40,16 @@ type TeamsClient struct {
 //
 // ErrNotFound is returned if the resource does not exist.
 func (c *TeamsClient) Get(ctx context.Context, teamName string) (gitprovider.Team, error) {
-	apiObjs := []*github.User{}
-	opts := &github.TeamListTeamMembersOptions{}
-	err := allPages(&opts.ListOptions, func() (*github.Response, error) {
-		// GET /orgs/{org}/teams/{team_slug}/members
-		pageObjs, resp, listErr := c.c.Teams.ListTeamMembersBySlug(ctx, c.ref.Organization, teamName, opts)
-		apiObjs = append(apiObjs, pageObjs...)
-		return resp, listErr
-	})
+	// GET /orgs/{org}/teams/{team_slug}/members
+	apiObjs, err := c.c.ListOrgTeamMembers(ctx, c.ref.Organization, teamName)
 	if err != nil {
 		return nil, err
 	}
 
+	// Collect a list of the members' names. Login is validated to be non-nil in ListOrgTeamMembers.
 	logins := make([]string, 0, len(apiObjs))
 	for _, apiObj := range apiObjs {
-		// Make sure login isn't nil
-		if apiObj.Login == nil {
-			return nil, fmt.Errorf("didn't expect login to be nil for user: %+v: %w", apiObj, gitprovider.ErrInvalidServerData)
-		}
-
+		// Login is validated to be non-nil in ListOrgTeamMembers
 		logins = append(logins, *apiObj.Login)
 	}
 
@@ -77,28 +67,17 @@ func (c *TeamsClient) Get(ctx context.Context, teamName string) (gitprovider.Tea
 //
 // List returns all available organizations, using multiple paginated requests if needed.
 func (c *TeamsClient) List(ctx context.Context) ([]gitprovider.Team, error) {
-	// List all teams, using pagination. This does not contain information about the members
-	apiObjs := []*github.Team{}
-	opts := &github.ListOptions{}
-	err := allPages(opts, func() (*github.Response, error) {
-		// GET /orgs/{org}/teams
-		pageObjs, resp, listErr := c.c.Teams.ListTeams(ctx, c.ref.Organization, opts)
-		apiObjs = append(apiObjs, pageObjs...)
-		return resp, listErr
-	})
+	// GET /orgs/{org}/teams
+	apiObjs, err := c.c.ListOrgTeams(ctx, c.ref.Organization)
 	if err != nil {
 		return nil, err
 	}
 
 	// Use .Get() to get detailed information about each member
 	teams := make([]gitprovider.Team, 0, len(apiObjs))
 	for _, apiObj := range apiObjs {
-		// Make sure name isn't nil
-		if apiObj.Slug == nil {
-			return nil, fmt.Errorf("didn't expect slug to be nil for team: %+v: %w", apiObj, gitprovider.ErrInvalidServerData)
-		}
-
-		// Get information about individual teams
+		// Get detailed information about individual teams (including members).
+		// Slug is validated to be non-nil in ListOrgTeams.
 		team, err := c.Get(ctx, *apiObj.Slug)
 		if err != nil {
 			return nil, err

github/client_organizations.go

@@ -18,9 +18,6 @@ package github
 
 import (
 	"context"
-	"fmt"
-
-	"github.com/google/go-github/v32/github"
 
 	"github.com/fluxcd/go-git-providers/gitprovider"
 )
@@ -44,11 +41,9 @@ func (c *OrganizationsClient) Get(ctx context.Context, ref gitprovider.Organizat
 	}
 
 	// GET /orgs/{org}
-	apiObj, _, err := c.c.Organizations.Get(ctx, ref.Organization)
-	// TODO: Provide some kind of debug logging if/when the httpcache is used
-	// One can see if the request hit the cache using: resp.Header[httpcache.XFromCache]
+	apiObj, err := c.c.GetOrg(ctx, ref.Organization)
 	if err != nil {
-		return nil, handleHTTPError(err)
+		return nil, err
 	}
 
 	return newOrganization(c.clientContext, apiObj, ref), nil
@@ -58,25 +53,15 @@ func (c *OrganizationsClient) Get(ctx context.Context, ref gitprovider.Organizat
 //
 // List returns all available organizations, using multiple paginated requests if needed.
 func (c *OrganizationsClient) List(ctx context.Context) ([]gitprovider.Organization, error) {
-	apiObjs := []*github.Organization{}
-	opts := &github.ListOptions{}
-	err := allPages(opts, func() (*github.Response, error) {
-		// GET /user/orgs
-		pageObjs, resp, listErr := c.c.Organizations.List(ctx, "", opts)
-		apiObjs = append(apiObjs, pageObjs...)
-		return resp, listErr
-	})
+	// GET /user/orgs
+	apiObjs, err := c.c.ListOrgs(ctx)
 	if err != nil {
 		return nil, err
 	}
 
 	orgs := make([]gitprovider.Organization, 0, len(apiObjs))
 	for _, apiObj := range apiObjs {
-		// Make sure name isn't nil
-		if apiObj.Login == nil {
-			return nil, fmt.Errorf("didn't expect login to be nil for org: %+v: %w", apiObj, gitprovider.ErrInvalidServerData)
-		}
-
+		// apiObj.Login is already validated to be non-nil in ListOrgs
 		orgs = append(orgs, newOrganization(c.clientContext, apiObj, gitprovider.OrganizationRef{
 			Domain:       c.domain,
 			Organization: *apiObj.Login,

github/client_repositories_org.go

@@ -41,7 +41,8 @@ func (c *OrgRepositoriesClient) Get(ctx context.Context, ref gitprovider.OrgRepo
 	if err := validateOrgRepositoryRef(ref, c.domain); err != nil {
 		return nil, err
 	}
-	apiObj, err := getRepository(ctx, c.c, ref)
+	// GET /repos/{owner}/{repo}
+	apiObj, err := c.c.GetRepo(ctx, ref.GetIdentity(), ref.GetRepository())
 	if err != nil {
 		return nil, err
 	}
@@ -57,27 +58,16 @@ func (c *OrgRepositoriesClient) List(ctx context.Context, ref gitprovider.Organi
 		return nil, err
 	}
 
-	// Get all of the repositories in the organization using pagination.
-	var apiObjs []*github.Repository
-	opts := &github.RepositoryListByOrgOptions{}
-	err := allPages(&opts.ListOptions, func() (*github.Response, error) {
-		// GET /orgs/{org}/repos
-		pageObjs, resp, listErr := c.c.Repositories.ListByOrg(ctx, ref.Organization, opts)
-		apiObjs = append(apiObjs, pageObjs...)
-		return resp, listErr
-	})
+	// GET /orgs/{org}/repos
+	apiObjs, err := c.c.ListOrgRepos(ctx, ref.Organization)
 	if err != nil {
 		return nil, err
 	}
 
 	// Traverse the list, and return a list of OrgRepository objects
 	repos := make([]gitprovider.OrgRepository, 0, len(apiObjs))
 	for _, apiObj := range apiObjs {
-		// Make sure apiObj is valid
-		if err := validateRepositoryAPI(apiObj); err != nil {
-			return nil, err
-		}
-
+		// apiObj is already validated at ListOrgRepos
 		repos = append(repos, newOrgRepository(c.clientContext, apiObj, gitprovider.OrgRepositoryRef{
 			OrganizationRef: ref,
 			RepositoryName:  *apiObj.Name,
@@ -130,13 +120,7 @@ func (c *OrgRepositoriesClient) Reconcile(ctx context.Context, ref gitprovider.O
 	return actual, actionTaken, err
 }
 
-func getRepository(ctx context.Context, c *github.Client, ref gitprovider.RepositoryRef) (*github.Repository, error) {
-	// GET /repos/{owner}/{repo}
-	apiObj, _, err := c.Repositories.Get(ctx, ref.GetIdentity(), ref.GetRepository())
-	return validateRepositoryAPIResp(apiObj, err)
-}
-
-func createRepository(ctx context.Context, c *github.Client, ref gitprovider.RepositoryRef, orgName string, req gitprovider.RepositoryInfo, opts ...gitprovider.RepositoryCreateOption) (*github.Repository, error) {
+func createRepository(ctx context.Context, c githubClient, ref gitprovider.RepositoryRef, orgName string, req gitprovider.RepositoryInfo, opts ...gitprovider.RepositoryCreateOption) (*github.Repository, error) {
 	// First thing, validate and default the request to ensure a valid and fully-populated object
 	// (to minimize any possible diffs between desired and actual state)
 	if err := gitprovider.ValidateAndDefaultInfo(&req); err != nil {
@@ -153,15 +137,7 @@ func createRepository(ctx context.Context, c *github.Client, ref gitprovider.Rep
 	data := repositoryToAPI(&req, ref)
 	applyRepoCreateOptions(&data, o)
 
-	return createRepositoryData(ctx, c, orgName, &data)
-}
-
-func createRepositoryData(ctx context.Context, c *github.Client, orgName string, data *github.Repository) (*github.Repository, error) {
-	// POST /user/repos or
-	// POST /orgs/{org}/repos
-	// depending on orgName
-	apiObj, _, err := c.Repositories.Create(ctx, orgName, data)
-	return validateRepositoryAPIResp(apiObj, err)
+	return c.CreateRepo(ctx, orgName, &data)
 }
 
 func reconcileRepository(ctx context.Context, actual gitprovider.UserRepository, req gitprovider.RepositoryInfo) (bool, error) {

github/client_repositories_user.go

@@ -20,8 +20,6 @@ import (
 	"context"
 	"errors"
 
-	"github.com/google/go-github/v32/github"
-
 	"github.com/fluxcd/go-git-providers/gitprovider"
 )
 
@@ -41,7 +39,8 @@ func (c *UserRepositoriesClient) Get(ctx context.Context, ref gitprovider.UserRe
 	if err := validateUserRepositoryRef(ref, c.domain); err != nil {
 		return nil, err
 	}
-	apiObj, err := getRepository(ctx, c.c, ref)
+	// GET /repos/{owner}/{repo}
+	apiObj, err := c.c.GetRepo(ctx, ref.GetIdentity(), ref.GetRepository())
 	if err != nil {
 		return nil, err
 	}
@@ -57,27 +56,16 @@ func (c *UserRepositoriesClient) List(ctx context.Context, ref gitprovider.UserR
 		return nil, err
 	}
 
-	// Get all of the user's repositories using pagination.
-	var apiObjs []*github.Repository
-	opts := &github.RepositoryListOptions{}
-	err := allPages(&opts.ListOptions, func() (*github.Response, error) {
-		// GET /users/{username}/repos
-		pageObjs, resp, listErr := c.c.Repositories.List(ctx, ref.GetIdentity(), opts)
-		apiObjs = append(apiObjs, pageObjs...)
-		return resp, listErr
-	})
+	// GET /users/{username}/repos
+	apiObjs, err := c.c.ListUserRepos(ctx, ref.UserLogin)
 	if err != nil {
 		return nil, err
 	}
 
 	// Traverse the list, and return a list of UserRepository objects
 	repos := make([]gitprovider.UserRepository, 0, len(apiObjs))
 	for _, apiObj := range apiObjs {
-		// Make sure apiObj is valid
-		if err := validateRepositoryAPI(apiObj); err != nil {
-			return nil, err
-		}
-
+		// apiObj is already validated at ListUserRepos
 		repos = append(repos, newUserRepository(c.clientContext, apiObj, gitprovider.UserRepositoryRef{
 			UserRef:        ref,
 			RepositoryName: *apiObj.Name,

github/client_repository_deploykey.go

@@ -64,7 +64,7 @@ func (c *DeployKeyClient) List(ctx context.Context) ([]gitprovider.DeployKey, er
 	if err != nil {
 		return nil, err
 	}
-	// Cast to []gitprovider.DeployKey
+	// Cast to the generic []gitprovider.DeployKey
 	keys := make([]gitprovider.DeployKey, 0, len(dks))
 	for _, dk := range dks {
 		keys = append(keys, dk)
@@ -73,27 +73,17 @@ func (c *DeployKeyClient) List(ctx context.Context) ([]gitprovider.DeployKey, er
 }
 
 func (c *DeployKeyClient) list(ctx context.Context) ([]*deployKey, error) {
-	// List all keys, using pagination.
-	apiObjs := []*github.Key{}
-	opts := &github.ListOptions{}
-	err := allPages(opts, func() (*github.Response, error) {
-		// GET /repos/{owner}/{repo}/keys
-		pageObjs, resp, listErr := c.c.Repositories.ListKeys(ctx, c.ref.GetIdentity(), c.ref.GetRepository(), opts)
-		apiObjs = append(apiObjs, pageObjs...)
-		return resp, listErr
-	})
+	// GET /repos/{owner}/{repo}/keys
+	apiObjs, err := c.c.ListKeys(ctx, c.ref.GetIdentity(), c.ref.GetRepository())
 	if err != nil {
 		return nil, err
 	}
 
 	// Map the api object to our DeployKey type
 	keys := make([]*deployKey, 0, len(apiObjs))
 	for _, apiObj := range apiObjs {
-		k, err := newDeployKey(c, apiObj)
-		if err != nil {
-			return nil, err
-		}
-		keys = append(keys, k)
+		// apiObj is already validated at ListKeys
+		keys = append(keys, newDeployKey(c, apiObj))
 	}
 
 	return keys, nil
@@ -107,7 +97,7 @@ func (c *DeployKeyClient) Create(ctx context.Context, req gitprovider.DeployKeyI
 	if err != nil {
 		return nil, err
 	}
-	return newDeployKey(c, apiObj)
+	return newDeployKey(c, apiObj), nil
 }
 
 // Reconcile makes sure the given desired state (req) becomes the actual state in the backing Git provider.
@@ -148,18 +138,12 @@ func (c *DeployKeyClient) Reconcile(ctx context.Context, req gitprovider.DeployK
 	return actual, true, actual.Update(ctx)
 }
 
-func createDeployKey(ctx context.Context, c *github.Client, ref gitprovider.RepositoryRef, req gitprovider.DeployKeyInfo) (*github.Key, error) {
+func createDeployKey(ctx context.Context, c githubClient, ref gitprovider.RepositoryRef, req gitprovider.DeployKeyInfo) (*github.Key, error) {
 	// First thing, validate and default the request to ensure a valid and fully-populated object
 	// (to minimize any possible diffs between desired and actual state)
 	if err := gitprovider.ValidateAndDefaultInfo(&req); err != nil {
 		return nil, err
 	}
-
-	return createDeployKeyData(ctx, c, ref, deployKeyToAPI(&req))
-}
-
-func createDeployKeyData(ctx context.Context, c *github.Client, ref gitprovider.RepositoryRef, data *github.Key) (*github.Key, error) {
 	// POST /repos/{owner}/{repo}/keys
-	apiObj, _, err := c.Repositories.CreateKey(ctx, ref.GetIdentity(), ref.GetRepository(), data)
-	return apiObj, handleHTTPError(err)
+	return c.CreateKey(ctx, ref.GetIdentity(), ref.GetRepository(), deployKeyToAPI(&req))
 }