From aaa05d464e272fdd1007410dc8894aa89e1d8fc3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= <lucas@weave.works>
Date: Mon, 17 Aug 2020 20:08:14 +0300
Subject: [PATCH 1/2] Carving out a high-level githubClient interface for all
 operations.

---
 github/auth.go                         |   3 +
 github/client.go                       |   5 +-
 github/client_organization_teams.go    |  36 +--
 github/client_organizations.go         |  25 +-
 github/client_repositories_org.go      |  38 +--
 github/client_repositories_user.go     |  22 +-
 github/client_repository_deploykey.go  |  31 +--
 github/client_repository_teamaccess.go |  43 +--
 github/githubclient.go                 | 359 +++++++++++++++++++++++++
 github/resource_deploykey.go           |  17 +-
 github/resource_organization.go        |  11 +
 github/resource_repository.go          |  31 +--
 github/resource_teamaccess.go          |  17 +-
 13 files changed, 430 insertions(+), 208 deletions(-)
 create mode 100644 github/githubclient.go

diff --git a/github/auth.go b/github/auth.go
index 76f093a1..8c5fbcac 100644
--- a/github/auth.go
+++ b/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
diff --git a/github/client.go b/github/client.go
index 0ae5e70f..d5ff5f51 100644
--- a/github/client.go
+++ b/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
 }
diff --git a/github/client_organization_teams.go b/github/client_organization_teams.go
index b56edf96..7df8e679 100644
--- a/github/client_organization_teams.go
+++ b/github/client_organization_teams.go
@@ -18,7 +18,6 @@ package github
 
 import (
 	"context"
-	"fmt"
 
 	"github.com/google/go-github/v32/github"
 
@@ -41,25 +40,15 @@ 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)
-		}
-
 		logins = append(logins, *apiObj.Login)
 	}
 
@@ -77,15 +66,8 @@ 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
 	}
@@ -93,12 +75,8 @@ func (c *TeamsClient) List(ctx context.Context) ([]gitprovider.Team, error) {
 	// 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
diff --git a/github/client_organizations.go b/github/client_organizations.go
index 0ef5634a..fead1533 100644
--- a/github/client_organizations.go
+++ b/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,
diff --git a/github/client_repositories_org.go b/github/client_repositories_org.go
index 04dca56f..11114ef3 100644
--- a/github/client_repositories_org.go
+++ b/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,15 +58,8 @@ 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
 	}
@@ -73,11 +67,7 @@ func (c *OrgRepositoriesClient) List(ctx context.Context, ref gitprovider.Organi
 	// 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) {
diff --git a/github/client_repositories_user.go b/github/client_repositories_user.go
index 16f0b063..5dff74f9 100644
--- a/github/client_repositories_user.go
+++ b/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,15 +56,8 @@ 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
 	}
@@ -73,11 +65,7 @@ func (c *UserRepositoriesClient) List(ctx context.Context, ref gitprovider.UserR
 	// 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,
diff --git a/github/client_repository_deploykey.go b/github/client_repository_deploykey.go
index 24b87f0c..454403c8 100644
--- a/github/client_repository_deploykey.go
+++ b/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,15 +73,8 @@ 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
 	}
@@ -89,11 +82,7 @@ func (c *DeployKeyClient) list(ctx context.Context) ([]*deployKey, error) {
 	// 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)
+		keys = append(keys, newDeployKey(c, apiObj))
 	}
 
 	return keys, nil
@@ -107,7 +96,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 req is the actual state in the backing Git provider.
@@ -148,18 +137,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))
 }
diff --git a/github/client_repository_teamaccess.go b/github/client_repository_teamaccess.go
index 97b3a437..bf52a806 100644
--- a/github/client_repository_teamaccess.go
+++ b/github/client_repository_teamaccess.go
@@ -19,9 +19,6 @@ package github
 import (
 	"context"
 	"errors"
-	"fmt"
-
-	"github.com/google/go-github/v32/github"
 
 	"github.com/fluxcd/go-git-providers/gitprovider"
 )
@@ -41,19 +38,19 @@ type TeamAccessClient struct {
 // Teams are sub-groups in GitLab.
 //
 // ErrNotFound is returned if the resource does not exist.
+//
+// TeamAccess.APIObject will be nil, because there's no underlying Github struct.
 func (c *TeamAccessClient) Get(ctx context.Context, name string) (gitprovider.TeamAccess, error) {
 	// GET /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}
-	apiObj, _, err := c.c.Teams.IsTeamRepoBySlug(ctx, c.ref.GetIdentity(), name, c.ref.GetIdentity(), c.ref.GetRepository())
+	permissionMap, err := c.c.GetTeamPermissions(ctx, c.ref.GetIdentity(), c.ref.GetRepository(), name)
 	if err != nil {
-		return nil, handleHTTPError(err)
-	}
-
-	// Make sure name isn't nil
-	if apiObj.Permissions == nil {
-		return nil, fmt.Errorf("didn't expect permissions to be nil for team access object: %+v: %w", apiObj, gitprovider.ErrInvalidServerData)
+		return nil, err
 	}
 
-	return newTeamAccess(c, teamAccessFromAPI(apiObj, name)), nil
+	return newTeamAccess(c, gitprovider.TeamAccessInfo{
+		Name:       name,
+		Permission: getPermissionFromMap(permissionMap),
+	}), nil
 }
 
 // List lists the team access control list for this repository.
@@ -61,26 +58,14 @@ func (c *TeamAccessClient) Get(ctx context.Context, name string) (gitprovider.Te
 // List returns all available team access lists, using multiple paginated requests if needed.
 func (c *TeamAccessClient) List(ctx context.Context) ([]gitprovider.TeamAccess, 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 /repos/{owner}/{repo}/teams
-		pageObjs, resp, listErr := c.c.Repositories.ListTeams(ctx, c.ref.GetIdentity(), c.ref.GetRepository(), opts)
-		apiObjs = append(apiObjs, pageObjs...)
-		return resp, listErr
-	})
+	apiObjs, err := c.c.ListRepoTeams(ctx, c.ref.GetIdentity(), c.ref.GetRepository())
 	if err != nil {
 		return nil, err
 	}
 
 	teamAccess := make([]gitprovider.TeamAccess, 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 access: %+v: %w", apiObj, gitprovider.ErrInvalidServerData)
-		}
-
-		// Get more detailed info about the team
+		// Get more detailed info about the team, we know that Slug is non-nil as of ListTeams.
 		ta, err := c.Get(ctx, *apiObj.Slug)
 		if err != nil {
 			return nil, err
@@ -101,13 +86,9 @@ func (c *TeamAccessClient) Create(ctx context.Context, req gitprovider.TeamAcces
 		return nil, err
 	}
 
-	opts := &github.TeamAddTeamRepoOptions{
-		Permission: string(*req.Permission),
-	}
 	// PUT /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}
-	_, err := c.c.Teams.AddTeamRepoBySlug(ctx, c.ref.GetIdentity(), req.Name, c.ref.GetIdentity(), c.ref.GetRepository(), opts)
-	if err != nil {
-		return nil, handleHTTPError(err)
+	if err := c.c.AddTeam(ctx, c.ref.GetIdentity(), c.ref.GetRepository(), req.Name, *req.Permission); err != nil {
+		return nil, err
 	}
 
 	return newTeamAccess(c, req), nil
diff --git a/github/githubclient.go b/github/githubclient.go
new file mode 100644
index 00000000..ad5fb224
--- /dev/null
+++ b/github/githubclient.go
@@ -0,0 +1,359 @@
+/*
+Copyright 2020 The Flux CD contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package github
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/fluxcd/go-git-providers/gitprovider"
+	"github.com/google/go-github/v32/github"
+)
+
+// githubClientImpl is a wrapper around *github.Client, which implements higher-level methods,
+// operating on the go-github structs. Pagination is implemented for all List* methods, all returned
+// objects are validated, and HTTP errors are handled/wrapped using handleHTTPError.
+// This interface is also fakeable, in order to unit-test the client.
+type githubClient interface {
+	// Client returns the underlying *github.Client
+	Client() *github.Client
+
+	// GetOrg is a wrapper for "GET /orgs/{org}".
+	// This function HTTP error wrapping, and validates the server result.
+	GetOrg(ctx context.Context, orgName string) (*github.Organization, error)
+	// ListOrgs is a wrapper for "GET /user/orgs".
+	// This function handles pagination, HTTP error wrapping, and validates the server result.
+	ListOrgs(ctx context.Context) ([]*github.Organization, error)
+
+	// ListOrgTeamMembers is a wrapper for "GET /orgs/{org}/teams/{team_slug}/members".
+	// This function handles pagination, HTTP error wrapping, and validates the server result.
+	ListOrgTeamMembers(ctx context.Context, orgName, teamName string) ([]*github.User, error)
+	// ListOrgTeams is a wrapper for "GET /orgs/{org}/teams".
+	// This function handles pagination, HTTP error wrapping, and validates the server result.
+	ListOrgTeams(ctx context.Context, orgName string) ([]*github.Team, error)
+
+	// GetRepo is a wrapper for "GET /repos/{owner}/{repo}".
+	// This function handles HTTP error wrapping, and validates the server result.
+	GetRepo(ctx context.Context, owner, repo string) (*github.Repository, error)
+	// ListOrgRepos is a wrapper for "GET /orgs/{org}/repos".
+	// This function handles pagination, HTTP error wrapping, and validates the server result.
+	ListOrgRepos(ctx context.Context, org string) ([]*github.Repository, error)
+	// ListUserRepos is a wrapper for "GET /users/{username}/repos".
+	// This function handles pagination, HTTP error wrapping, and validates the server result.
+	ListUserRepos(ctx context.Context, username string) ([]*github.Repository, error)
+	// CreateRepo is a wrapper for "POST /user/repos" (if orgName == "")
+	// or "POST /orgs/{org}/repos" (if orgName != "").
+	// This function handles HTTP error wrapping, and validates the server result.
+	CreateRepo(ctx context.Context, orgName string, req *github.Repository) (*github.Repository, error)
+	// UpdateRepo is a wrapper for "PATCH /repos/{owner}/{repo}".
+	// This function handles HTTP error wrapping, and validates the server result.
+	UpdateRepo(ctx context.Context, owner, repo string, req *github.Repository) (*github.Repository, error)
+	// DeleteRepo is a wrapper for "DELETE /repos/{owner}/{repo}".
+	// This function handles HTTP error wrapping.
+	// DANGEROUS COMMAND: In order to use this, you must set destructiveActions to true.
+	DeleteRepo(ctx context.Context, owner, repo string) error
+
+	// ListKeys is a wrapper for "GET /repos/{owner}/{repo}/keys".
+	// This function handles pagination, HTTP error wrapping, and validates the server result.
+	ListKeys(ctx context.Context, owner, repo string) ([]*github.Key, error)
+	// CreateKey is a wrapper for "POST /repos/{owner}/{repo}/keys".
+	// This function handles HTTP error wrapping, and validates the server result.
+	CreateKey(ctx context.Context, owner, repo string, req *github.Key) (*github.Key, error)
+	// DeleteKey is a wrapper for "DELETE /repos/{owner}/{repo}/keys/{key_id}".
+	// This function handles HTTP error wrapping.
+	DeleteKey(ctx context.Context, owner, repo string, id int64) error
+
+	// GetTeamPermissions is a wrapper for "GET /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}".
+	// This function handles HTTP error wrapping, and validates the server result.
+	GetTeamPermissions(ctx context.Context, orgName, repo, teamName string) (map[string]bool, error)
+	// ListRepoTeams is a wrapper for "GET /repos/{owner}/{repo}/teams".
+	// This function handles pagination, HTTP error wrapping, and validates the server result.
+	ListRepoTeams(ctx context.Context, orgName, repo string) ([]*github.Team, error)
+	// AddTeam is a wrapper for "PUT /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}".
+	// This function handles HTTP error wrapping.
+	AddTeam(ctx context.Context, orgName, repo, teamName string, permission gitprovider.RepositoryPermission) error
+	// RemoveTeam is a wrapper for "DELETE /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}".
+	// This function handles HTTP error wrapping.
+	RemoveTeam(ctx context.Context, orgName, repo, teamName string) error
+}
+
+// githubClientImpl is a wrapper around *github.Client, which implements higher-level methods,
+// operating on the go-github structs. See the githubClient interface for method documentation.
+// Pagination is implemented for all List* methods, all returned
+// objects are validated, and HTTP errors are handled/wrapped using handleHTTPError.
+type githubClientImpl struct {
+	c                  *github.Client
+	destructiveActions bool
+}
+
+// githubClientImpl implements githubClient.
+var _ githubClient = &githubClientImpl{}
+
+func (c *githubClientImpl) Client() *github.Client {
+	return c.c
+}
+
+func (c *githubClientImpl) GetOrg(ctx context.Context, orgName string) (*github.Organization, error) {
+	// GET /orgs/{org}
+	apiObj, _, err := c.c.Organizations.Get(ctx, orgName)
+	if err != nil {
+		return nil, handleHTTPError(err)
+	}
+	// Validate the API object
+	if err := validateOrganizationAPI(apiObj); err != nil {
+		return nil, err
+	}
+	return apiObj, nil
+}
+
+func (c *githubClientImpl) ListOrgs(ctx context.Context) ([]*github.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
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	// Validate the API objects
+	for _, apiObj := range apiObjs {
+		if err := validateOrganizationAPI(apiObj); err != nil {
+			return nil, err
+		}
+	}
+	return apiObjs, nil
+}
+
+func (c *githubClientImpl) ListOrgTeamMembers(ctx context.Context, orgName, teamName string) ([]*github.User, 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, orgName, teamName, opts)
+		apiObjs = append(apiObjs, pageObjs...)
+		return resp, listErr
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	// Make sure the Login field is set.
+	for _, apiObj := range apiObjs {
+		if apiObj.Login == nil {
+			return nil, fmt.Errorf("didn't expect login to be nil for user: %+v: %w", apiObj, gitprovider.ErrInvalidServerData)
+		}
+	}
+
+	return apiObjs, nil
+}
+
+func (c *githubClientImpl) ListOrgTeams(ctx context.Context, orgName string) ([]*github.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, orgName, opts)
+		apiObjs = append(apiObjs, pageObjs...)
+		return resp, listErr
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	// Make sure the Slug field is set.
+	for _, apiObj := range apiObjs {
+		if apiObj.Slug == nil {
+			return nil, fmt.Errorf("didn't expect slug to be nil for team: %+v: %w", apiObj, gitprovider.ErrInvalidServerData)
+		}
+	}
+	return apiObjs, nil
+}
+
+func (c *githubClientImpl) GetRepo(ctx context.Context, owner, repo string) (*github.Repository, error) {
+	// GET /repos/{owner}/{repo}
+	apiObj, _, err := c.c.Repositories.Get(ctx, owner, repo)
+	return validateRepositoryAPIResp(apiObj, err)
+}
+
+func validateRepositoryAPIResp(apiObj *github.Repository, err error) (*github.Repository, error) {
+	// If the response contained an error, return
+	if err != nil {
+		return nil, handleHTTPError(err)
+	}
+	// Make sure apiObj is valid
+	if err := validateRepositoryAPI(apiObj); err != nil {
+		return nil, err
+	}
+	return apiObj, nil
+}
+
+func (c *githubClientImpl) ListOrgRepos(ctx context.Context, org string) ([]*github.Repository, error) {
+	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, org, opts)
+		apiObjs = append(apiObjs, pageObjs...)
+		return resp, listErr
+	})
+	if err != nil {
+		return nil, err
+	}
+	return validateRepositoryObjects(apiObjs)
+}
+
+func validateRepositoryObjects(apiObjs []*github.Repository) ([]*github.Repository, error) {
+	for _, apiObj := range apiObjs {
+		// Make sure apiObj is valid
+		if err := validateRepositoryAPI(apiObj); err != nil {
+			return nil, err
+		}
+	}
+	return apiObjs, nil
+}
+
+func (c *githubClientImpl) ListUserRepos(ctx context.Context, username string) ([]*github.Repository, error) {
+	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, username, opts)
+		apiObjs = append(apiObjs, pageObjs...)
+		return resp, listErr
+	})
+	if err != nil {
+		return nil, err
+	}
+	return validateRepositoryObjects(apiObjs)
+}
+
+func (c *githubClientImpl) CreateRepo(ctx context.Context, orgName string, req *github.Repository) (*github.Repository, error) {
+	// POST /user/repos (if orgName == "")
+	// POST /orgs/{org}/repos (if orgName != "")
+	apiObj, _, err := c.c.Repositories.Create(ctx, orgName, req)
+	return validateRepositoryAPIResp(apiObj, err)
+}
+
+func (c *githubClientImpl) UpdateRepo(ctx context.Context, owner, repo string, req *github.Repository) (*github.Repository, error) {
+	// PATCH /repos/{owner}/{repo}
+	apiObj, _, err := c.c.Repositories.Edit(ctx, owner, repo, req)
+	return validateRepositoryAPIResp(apiObj, err)
+}
+
+func (c *githubClientImpl) DeleteRepo(ctx context.Context, owner, repo string) error {
+	// Don't allow deleting repositories if the user didn't explicitly allow dangerous API calls.
+	if !c.destructiveActions {
+		return fmt.Errorf("cannot delete repository: %w", ErrDestructiveCallDisallowed)
+	}
+	// DELETE /repos/{owner}/{repo}
+	_, err := c.c.Repositories.Delete(ctx, owner, repo)
+	return handleHTTPError(err)
+}
+
+func (c *githubClientImpl) ListKeys(ctx context.Context, owner, repo string) ([]*github.Key, error) {
+	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, owner, repo, opts)
+		apiObjs = append(apiObjs, pageObjs...)
+		return resp, listErr
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	for _, apiObj := range apiObjs {
+		if err := validateDeployKeyAPI(apiObj); err != nil {
+			return nil, err
+		}
+	}
+	return apiObjs, nil
+}
+
+func (c *githubClientImpl) CreateKey(ctx context.Context, owner, repo string, req *github.Key) (*github.Key, error) {
+	// POST /repos/{owner}/{repo}/keys
+	apiObj, _, err := c.c.Repositories.CreateKey(ctx, owner, repo, req)
+	if err != nil {
+		return nil, handleHTTPError(err)
+	}
+	if err := validateDeployKeyAPI(apiObj); err != nil {
+		return nil, err
+	}
+	return apiObj, nil
+}
+
+func (c *githubClientImpl) DeleteKey(ctx context.Context, owner, repo string, id int64) error {
+	// DELETE /repos/{owner}/{repo}/keys/{key_id}
+	_, err := c.c.Repositories.DeleteKey(ctx, owner, repo, id)
+	return handleHTTPError(err)
+}
+
+func (c *githubClientImpl) GetTeamPermissions(ctx context.Context, orgName, repo, teamName string) (map[string]bool, error) {
+	// GET /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}
+	apiObj, _, err := c.c.Teams.IsTeamRepoBySlug(ctx, orgName, teamName, orgName, repo)
+	if err != nil {
+		return nil, handleHTTPError(err)
+	}
+
+	// Make sure permissions isn't nil
+	if apiObj.Permissions == nil {
+		return nil, fmt.Errorf("didn't expect permissions to be nil for team: %+v: %w", apiObj, gitprovider.ErrInvalidServerData)
+	}
+	return *apiObj.Permissions, nil
+}
+
+func (c *githubClientImpl) ListRepoTeams(ctx context.Context, orgName, repo string) ([]*github.Team, error) {
+	apiObjs := []*github.Team{}
+	opts := &github.ListOptions{}
+	err := allPages(opts, func() (*github.Response, error) {
+		// GET /repos/{owner}/{repo}/teams
+		pageObjs, resp, listErr := c.c.Repositories.ListTeams(ctx, orgName, repo, opts)
+		apiObjs = append(apiObjs, pageObjs...)
+		return resp, listErr
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	// Make sure the Slug field isn't nil
+	for _, apiObj := range apiObjs {
+		if apiObj.Slug == nil {
+			return nil, fmt.Errorf("didn't expect slug to be nil for team: %+v: %w", apiObj, gitprovider.ErrInvalidServerData)
+		}
+	}
+	return apiObjs, nil
+}
+
+func (c *githubClientImpl) AddTeam(ctx context.Context, orgName, repo, teamName string, permission gitprovider.RepositoryPermission) error {
+	// PUT /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}
+	_, err := c.c.Teams.AddTeamRepoBySlug(ctx, orgName, teamName, orgName, repo, &github.TeamAddTeamRepoOptions{
+		Permission: string(permission),
+	})
+	return handleHTTPError(err)
+}
+
+func (c *githubClientImpl) RemoveTeam(ctx context.Context, orgName, repo, teamName string) error {
+	// DELETE /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}
+	_, err := c.c.Teams.RemoveTeamRepoBySlug(ctx, orgName, teamName, orgName, repo)
+	return handleHTTPError(err)
+}
diff --git a/github/resource_deploykey.go b/github/resource_deploykey.go
index 43338905..ec578ffa 100644
--- a/github/resource_deploykey.go
+++ b/github/resource_deploykey.go
@@ -28,14 +28,11 @@ import (
 	"github.com/fluxcd/go-git-providers/validation"
 )
 
-func newDeployKey(c *DeployKeyClient, key *github.Key) (*deployKey, error) {
-	if err := validateDeployKeyAPI(key); err != nil {
-		return nil, err
-	}
+func newDeployKey(c *DeployKeyClient, key *github.Key) *deployKey {
 	return &deployKey{
 		k: *key,
 		c: c,
-	}, nil
+	}
 }
 
 var _ gitprovider.DeployKey = &deployKey{}
@@ -92,9 +89,7 @@ func (dk *deployKey) Delete(ctx context.Context) error {
 		return fmt.Errorf("didn't expect ID to be nil: %w", gitprovider.ErrUnexpectedEvent)
 	}
 
-	// DELETE /repos/{owner}/{repo}/keys/{key_id}
-	_, err := dk.c.c.Repositories.DeleteKey(ctx, dk.c.ref.GetIdentity(), dk.c.ref.GetRepository(), *dk.k.ID)
-	return handleHTTPError(err)
+	return dk.c.c.DeleteKey(ctx, dk.c.ref.GetIdentity(), dk.c.ref.GetRepository(), *dk.k.ID)
 }
 
 // Reconcile makes sure the desired state in this object (called "req" here) becomes
@@ -130,13 +125,11 @@ func (dk *deployKey) Reconcile(ctx context.Context) (bool, error) {
 }
 
 func (dk *deployKey) createIntoSelf(ctx context.Context) error {
-	apiObj, err := createDeployKeyData(ctx, dk.c.c, dk.c.ref, &dk.k)
+	// POST /repos/{owner}/{repo}/keys
+	apiObj, err := dk.c.c.CreateKey(ctx, dk.c.ref.GetIdentity(), dk.c.ref.GetRepository(), &dk.k)
 	if err != nil {
 		return err
 	}
-	if err := validateDeployKeyAPI(apiObj); err != nil {
-		return err
-	}
 	dk.k = *apiObj
 	return nil
 }
diff --git a/github/resource_organization.go b/github/resource_organization.go
index c9562c57..86010ca7 100644
--- a/github/resource_organization.go
+++ b/github/resource_organization.go
@@ -17,6 +17,8 @@ limitations under the License.
 package github
 
 import (
+	"fmt"
+
 	"github.com/google/go-github/v32/github"
 
 	"github.com/fluxcd/go-git-providers/gitprovider"
@@ -67,3 +69,12 @@ func organizationFromAPI(apiObj *github.Organization) gitprovider.OrganizationIn
 		Description: apiObj.Description,
 	}
 }
+
+// validateOrganizationAPI validates the apiObj received from the server, to make sure that it is
+// valid for our use.
+func validateOrganizationAPI(apiObj *github.Organization) error {
+	if apiObj.Login == nil {
+		return fmt.Errorf("didn't expect login to be nil for org: %+v: %w", apiObj, gitprovider.ErrInvalidServerData)
+	}
+	return nil
+}
diff --git a/github/resource_repository.go b/github/resource_repository.go
index 2f6787ae..63175488 100644
--- a/github/resource_repository.go
+++ b/github/resource_repository.go
@@ -19,7 +19,6 @@ package github
 import (
 	"context"
 	"errors"
-	"fmt"
 	"reflect"
 
 	"github.com/google/go-github/v32/github"
@@ -45,7 +44,7 @@ var _ gitprovider.UserRepository = &userRepository{}
 type userRepository struct {
 	*clientContext
 
-	r   github.Repository
+	r   github.Repository // go-github
 	ref gitprovider.RepositoryRef
 
 	deployKeys *DeployKeyClient
@@ -86,9 +85,7 @@ func (r *userRepository) DeployKeys() gitprovider.DeployKeyClient {
 // The internal API object will be overridden with the received server data.
 func (r *userRepository) Update(ctx context.Context) error {
 	// PATCH /repos/{owner}/{repo}
-	apiObj, _, err := r.c.Repositories.Edit(ctx, r.ref.GetIdentity(), r.ref.GetRepository(), &r.r)
-	// Run through validation
-	apiObj, err = validateRepositoryAPIResp(apiObj, err)
+	apiObj, err := r.c.UpdateRepo(ctx, r.ref.GetIdentity(), r.ref.GetRepository(), &r.r)
 	if err != nil {
 		return err
 	}
@@ -105,7 +102,7 @@ func (r *userRepository) Update(ctx context.Context) error {
 //
 // The internal API object will be overridden with the received server data if actionTaken == true.
 func (r *userRepository) Reconcile(ctx context.Context) (bool, error) {
-	apiObj, err := getRepository(ctx, r.c, r.ref)
+	apiObj, err := r.c.GetRepo(ctx, r.ref.GetIdentity(), r.ref.GetRepository())
 	if err != nil {
 		// Create if not found
 		if errors.Is(err, gitprovider.ErrNotFound) {
@@ -113,7 +110,7 @@ func (r *userRepository) Reconcile(ctx context.Context) (bool, error) {
 			if orgRef, ok := r.ref.(gitprovider.OrgRepositoryRef); ok {
 				orgName = orgRef.Organization
 			}
-			repo, err := createRepositoryData(ctx, r.c, orgName, &r.r)
+			repo, err := r.c.CreateRepo(ctx, orgName, &r.r)
 			if err != nil {
 				return true, err
 			}
@@ -140,13 +137,7 @@ func (r *userRepository) Reconcile(ctx context.Context) (bool, error) {
 //
 // ErrNotFound is returned if the resource doesn't exist anymore.
 func (r *userRepository) Delete(ctx context.Context) error {
-	// Don't allow deleting repositories if the user didn't explicitly allow dangerous API calls.
-	if !r.destructiveActions {
-		return fmt.Errorf("cannot delete repository: %w", ErrDestructiveCallDisallowed)
-	}
-
-	_, err := r.c.Repositories.Delete(ctx, r.ref.GetIdentity(), r.ref.GetRepository())
-	return handleHTTPError(err)
+	return r.c.DeleteRepo(ctx, r.ref.GetIdentity(), r.ref.GetRepository())
 }
 
 func newOrgRepository(ctx *clientContext, apiObj *github.Repository, ref gitprovider.RepositoryRef) *orgRepository {
@@ -198,18 +189,6 @@ func repositoryFromAPI(apiObj *github.Repository) gitprovider.RepositoryInfo {
 	return repo
 }
 
-func validateRepositoryAPIResp(apiObj *github.Repository, err error) (*github.Repository, error) {
-	// If the response contained an error, return
-	if err != nil {
-		return nil, handleHTTPError(err)
-	}
-	// Make sure apiObj is valid
-	if err := validateRepositoryAPI(apiObj); err != nil {
-		return nil, err
-	}
-	return apiObj, nil
-}
-
 func repositoryToAPI(repo *gitprovider.RepositoryInfo, ref gitprovider.RepositoryRef) github.Repository {
 	apiObj := github.Repository{
 		Name: gitprovider.StringVar(ref.GetRepository()),
diff --git a/github/resource_teamaccess.go b/github/resource_teamaccess.go
index 3fd6c386..57d17bbb 100644
--- a/github/resource_teamaccess.go
+++ b/github/resource_teamaccess.go
@@ -20,8 +20,6 @@ import (
 	"context"
 	"errors"
 
-	"github.com/google/go-github/v32/github"
-
 	"github.com/fluxcd/go-git-providers/gitprovider"
 )
 
@@ -64,13 +62,7 @@ func (ta *teamAccess) Repository() gitprovider.RepositoryRef {
 // ErrNotFound is returned if the resource does not exist.
 func (ta *teamAccess) Delete(ctx context.Context) error {
 	// DELETE /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}
-	_, err := ta.c.c.Teams.RemoveTeamRepoBySlug(ctx,
-		ta.c.ref.GetIdentity(),
-		ta.ta.Name,
-		ta.c.ref.GetIdentity(),
-		ta.c.ref.GetRepository(),
-	)
-	return handleHTTPError(err)
+	return ta.c.c.RemoveTeam(ctx, ta.c.ref.GetIdentity(), ta.c.ref.GetRepository(), ta.ta.Name)
 }
 
 func (ta *teamAccess) Update(ctx context.Context) error {
@@ -113,13 +105,6 @@ func (ta *teamAccess) Reconcile(ctx context.Context) (bool, error) {
 	return true, ta.Update(ctx)
 }
 
-func teamAccessFromAPI(apiObj *github.Repository, teamName string) gitprovider.TeamAccessInfo {
-	return gitprovider.TeamAccessInfo{
-		Name:       teamName,
-		Permission: getPermissionFromMap(*apiObj.Permissions),
-	}
-}
-
 //nolint:gochecknoglobals,gomnd
 var permissionPriority = map[gitprovider.RepositoryPermission]int{
 	gitprovider.RepositoryPermissionPull:     1,

From f33c177324b28aa670c27df2a48fad7417ab490c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= <lucas@weave.works>
Date: Tue, 18 Aug 2020 17:06:49 +0300
Subject: [PATCH 2/2] Address review feedback

---
 github/client_organization_teams.go   |  1 +
 github/client_repository_deploykey.go |  1 +
 github/resource_organization.go       | 12 ++++++------
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/github/client_organization_teams.go b/github/client_organization_teams.go
index 7df8e679..4eae727d 100644
--- a/github/client_organization_teams.go
+++ b/github/client_organization_teams.go
@@ -49,6 +49,7 @@ func (c *TeamsClient) Get(ctx context.Context, teamName string) (gitprovider.Tea
 	// 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 {
+		// Login is validated to be non-nil in ListOrgTeamMembers
 		logins = append(logins, *apiObj.Login)
 	}
 
diff --git a/github/client_repository_deploykey.go b/github/client_repository_deploykey.go
index 454403c8..e4ec5919 100644
--- a/github/client_repository_deploykey.go
+++ b/github/client_repository_deploykey.go
@@ -82,6 +82,7 @@ func (c *DeployKeyClient) list(ctx context.Context) ([]*deployKey, error) {
 	// Map the api object to our DeployKey type
 	keys := make([]*deployKey, 0, len(apiObjs))
 	for _, apiObj := range apiObjs {
+		// apiObj is already validated at ListKeys
 		keys = append(keys, newDeployKey(c, apiObj))
 	}
 
diff --git a/github/resource_organization.go b/github/resource_organization.go
index 86010ca7..9dd35ed1 100644
--- a/github/resource_organization.go
+++ b/github/resource_organization.go
@@ -17,11 +17,10 @@ limitations under the License.
 package github
 
 import (
-	"fmt"
-
 	"github.com/google/go-github/v32/github"
 
 	"github.com/fluxcd/go-git-providers/gitprovider"
+	"github.com/fluxcd/go-git-providers/validation"
 )
 
 func newOrganization(ctx *clientContext, apiObj *github.Organization, ref gitprovider.OrganizationRef) *organization {
@@ -73,8 +72,9 @@ func organizationFromAPI(apiObj *github.Organization) gitprovider.OrganizationIn
 // validateOrganizationAPI validates the apiObj received from the server, to make sure that it is
 // valid for our use.
 func validateOrganizationAPI(apiObj *github.Organization) error {
-	if apiObj.Login == nil {
-		return fmt.Errorf("didn't expect login to be nil for org: %+v: %w", apiObj, gitprovider.ErrInvalidServerData)
-	}
-	return nil
+	return validateAPIObject("GitHub.Organization", func(validator validation.Validator) {
+		if apiObj.Login == nil {
+			validator.Required("Login")
+		}
+	})
 }