Use slug-based APIs for team operations

go.mod

@@ -10,7 +10,6 @@ require (
 	github.com/c2h5oh/datasize v0.0.0-20171227191756-4eba002a5eae
 	github.com/die-net/lrucache v0.0.0-20181227122439-19a39ef22a11
 	github.com/google/go-github/v32 v32.0.0
-	github.com/google/go-querystring v1.0.0
 	github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79
 	github.com/inconshreveable/mousetrap v1.0.0 // indirect
 	github.com/palantir/go-baseapp v0.2.0

pull/github_membership.go

@@ -26,7 +26,6 @@ type GitHubMembershipContext struct {
 	ctx    context.Context
 	client *github.Client
 
-	teamIDs     map[string]int64
 	membership  map[string]bool
 	orgMembers  map[string][]string
 	teamMembers map[string][]string
@@ -36,7 +35,6 @@ func NewGitHubMembershipContext(ctx context.Context, client *github.Client) *Git
 	return &GitHubMembershipContext{
 		ctx:         ctx,
 		client:      client,
-		teamIDs:     make(map[string]int64),
 		membership:  make(map[string]bool),
 		orgMembers:  make(map[string][]string),
 		teamMembers: make(map[string][]string),
@@ -47,68 +45,36 @@ func membershipKey(group, user string) string {
 	return group + ":" + user
 }
 
+func splitTeam(team string) (org, slug string, err error) {
+	parts := strings.Split(team, "/")
+	if len(parts) != 2 {
+		return "", "", errors.Errorf("invalid team format: %s", team)
+	}
+	return parts[0], parts[1], nil
+}
+
 func (mc *GitHubMembershipContext) IsTeamMember(team, user string) (bool, error) {
 	key := membershipKey(team, user)
 
-	id, err := mc.getTeamID(team)
+	org, slug, err := splitTeam(team)
 	if err != nil {
-		return false, errors.Errorf("failed to get ID for team %s", team)
+		return false, err
 	}
 
 	isMember, ok := mc.membership[key]
 	if ok {
 		return isMember, nil
 	}
 
-	membership, _, err := GetTeamMembership(mc.ctx, mc.client, id, user)
+	membership, _, err := mc.client.Teams.GetTeamMembershipBySlug(mc.ctx, org, slug, user)
 	if err != nil && !isNotFound(err) {
 		return false, errors.Wrap(err, "failed to get team membership")
 	}
 
 	isMember = membership != nil && membership.GetState() == "active"
-
 	mc.membership[key] = isMember
-	return isMember, nil
-}
 
-func (mc *GitHubMembershipContext) getTeamID(team string) (int64, error) {
-	org := strings.Split(team, "/")[0]
-	id, ok := mc.teamIDs[team]
-	if !ok {
-		if err := mc.cacheTeamIDs(org); err != nil {
-			return 0, err
-		}
-
-		id, ok = mc.teamIDs[team]
-		if !ok {
-			return 0, errors.Errorf("failed to get ID for team %s", team)
-		}
-	}
-	return id, nil
-}
-
-func (mc *GitHubMembershipContext) cacheTeamIDs(org string) error {
-	opt := github.ListOptions{
-		PerPage: 100,
-	}
-
-	for {
-		teams, res, err := mc.client.Teams.ListTeams(mc.ctx, org, &opt)
-		if err != nil {
-			return errors.Wrap(err, "failed to list organization teams")
-		}
-
-		for _, t := range teams {
-			key := org + "/" + t.GetSlug()
-			mc.teamIDs[key] = t.GetID()
-		}
-
-		if res.NextPage == 0 {
-			break
-		}
-		opt.Page = res.NextPage
-	}
-	return nil
+	return isMember, nil
 }
 
 func (mc *GitHubMembershipContext) IsOrgMember(org, user string) (bool, error) {
@@ -175,13 +141,13 @@ func (mc *GitHubMembershipContext) TeamMembers(team string) ([]string, error) {
 			},
 		}
 
-		teamID, err := mc.getTeamID(team)
+		org, slug, err := splitTeam(team)
 		if err != nil {
-			return nil, errors.Wrapf(err, "Unable to get information for team %s", team)
+			return nil, err
 		}
 
 		for {
-			users, resp, err := ListTeamMembers(mc.ctx, mc.client, teamID, opt)
+			users, resp, err := mc.client.Teams.ListTeamMembersBySlug(mc.ctx, org, slug, opt)
 			if err != nil {
 				return nil, errors.Wrapf(err, "failed to list team %s members page %d", team, opt.Page)
 			}

pull/github_test.go

@@ -276,24 +276,20 @@ func TestNoComments(t *testing.T) {
 
 func TestIsTeamMember(t *testing.T) {
 	rp := &ResponsePlayer{}
-	teamsRule := rp.AddRule(
-		ExactPathMatcher("/orgs/testorg/teams"),
-		"testdata/responses/teams_testorg.yml",
-	)
 	yesRule1 := rp.AddRule(
-		ExactPathMatcher("/teams/123/memberships/mhaypenny"),
+		ExactPathMatcher("/orgs/testorg/teams/yes-team/memberships/mhaypenny"),
 		"testdata/responses/membership_team123_mhaypenny.yml",
 	)
 	yesRule2 := rp.AddRule(
-		ExactPathMatcher("/teams/123/memberships/ttest"),
+		ExactPathMatcher("/orgs/testorg/teams/yes-team/memberships/ttest"),
 		"testdata/responses/membership_team123_ttest.yml",
 	)
 	noRule1 := rp.AddRule(
-		ExactPathMatcher("/teams/456/memberships/mhaypenny"),
+		ExactPathMatcher("/orgs/testorg/teams/no-team/memberships/mhaypenny"),
 		"testdata/responses/membership_team456_mhaypenny.yml",
 	)
 	noRule2 := rp.AddRule(
-		ExactPathMatcher("/teams/456/memberships/ttest"),
+		ExactPathMatcher("/orgs/testorg/teams/no-team/memberships/ttest"),
 		"testdata/responses/membership_team456_ttest.yml",
 	)
 
@@ -303,38 +299,33 @@ func TestIsTeamMember(t *testing.T) {
 	require.NoError(t, err)
 
 	assert.True(t, isMember, "user is not a member")
-	assert.Equal(t, 2, teamsRule.Count, "no http request was made for teams")
 	assert.Equal(t, 1, yesRule1.Count, "no http request was made")
 
 	isMember, err = ctx.IsTeamMember("testorg/yes-team", "ttest")
 	require.NoError(t, err)
 
 	assert.True(t, isMember, "user is not a member")
-	assert.Equal(t, 2, teamsRule.Count, "cached team IDs were not used")
 	assert.Equal(t, 1, yesRule2.Count, "no http request was made")
 
 	// not a member because missing from team
 	isMember, err = ctx.IsTeamMember("testorg/no-team", "mhaypenny")
 	require.NoError(t, err)
 
 	assert.False(t, isMember, "user is a member")
-	assert.Equal(t, 2, teamsRule.Count, "cached team IDs were not used")
 	assert.Equal(t, 1, noRule1.Count, "no http request was made")
 
 	// not a member because membership state is pending
 	isMember, err = ctx.IsTeamMember("testorg/no-team", "ttest")
 	require.NoError(t, err)
 
 	assert.False(t, isMember, "user is a member")
-	assert.Equal(t, 2, teamsRule.Count, "cached team IDs were not used")
 	assert.Equal(t, 1, noRule2.Count, "no http request was made")
 
 	// verify that team membership is cached
 	isMember, err = ctx.IsTeamMember("testorg/yes-team", "mhaypenny")
 	require.NoError(t, err)
 
 	assert.True(t, isMember, "user is not a member")
-	assert.Equal(t, 2, teamsRule.Count, "cached team IDs were not used")
 	assert.Equal(t, 1, yesRule1.Count, "cached membership was not used")
 }