fix: Refactored github_repository_collaborators for team id (#2420)

Signed-off-by: Steve Hipwell <[email protected]>

Author: stevehipwell

github/resource_github_repository_collaborators.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"slices"
 	"sort"
 	"strconv"
 
@@ -136,37 +137,46 @@ func flattenUserCollaborators(objs []userCollaborator, invites []invitedCollabor
 
 type teamCollaborator struct {
 	permission string
+	teamID     int64
 	teamSlug   string
 }
 
 func (c teamCollaborator) Empty() bool {
 	return c == teamCollaborator{}
 }
 
-func flattenTeamCollaborator(obj teamCollaborator) interface{} {
+func flattenTeamCollaborator(obj teamCollaborator, teamIDs []int64) interface{} {
 	if obj.Empty() {
 		return nil
 	}
+
+	var teamIDString string
+	if slices.Contains(teamIDs, obj.teamID) {
+		teamIDString = strconv.FormatInt(obj.teamID, 10)
+	} else {
+		teamIDString = obj.teamSlug
+	}
+
 	transformed := map[string]interface{}{
 		"permission": obj.permission,
-		"team_id":    obj.teamSlug,
+		"team_id":    teamIDString,
 	}
 
 	return transformed
 }
 
-func flattenTeamCollaborators(objs []teamCollaborator) []interface{} {
+func flattenTeamCollaborators(objs []teamCollaborator, teamIDs []int64) []interface{} {
 	if objs == nil {
 		return nil
 	}
 
 	sort.SliceStable(objs, func(i, j int) bool {
-		return objs[i].teamSlug < objs[j].teamSlug
+		return objs[i].teamID < objs[j].teamID
 	})
 
 	items := make([]interface{}, len(objs))
 	for i, obj := range objs {
-		items[i] = flattenTeamCollaborator(obj)
+		items[i] = flattenTeamCollaborator(obj, teamIDs)
 	}
 
 	return items
@@ -248,7 +258,7 @@ func listTeams(client *github.Client, isOrg bool, ctx context.Context, owner, re
 		for _, t := range repoTeams {
 			permissionName := getPermission(t.GetPermission())
 
-			teamCollaborators = append(teamCollaborators, teamCollaborator{permissionName, t.GetSlug()})
+			teamCollaborators = append(teamCollaborators, teamCollaborator{permissionName, t.GetID(), t.GetSlug()})
 		}
 
 		if resp.NextPage == 0 {
@@ -376,33 +386,31 @@ func matchUserCollaboratorsAndInvites(
 func matchTeamCollaborators(
 	repoName string, want []interface{}, has []teamCollaborator, meta interface{}) error {
 	client := meta.(*Owner).v3client
+	orgID := meta.(*Owner).id
 	owner := meta.(*Owner).name
 	ctx := context.Background()
 
+	remove := make([]teamCollaborator, 0)
 	for _, hasTeam := range has {
 		var wantPerm string
 		for _, w := range want {
 			teamData := w.(map[string]interface{})
 			teamIDString := teamData["team_id"].(string)
-			teamSlug, err := getTeamSlug(teamIDString, meta)
+			teamID, err := getTeamID(teamIDString, meta)
 			if err != nil {
 				return err
 			}
-			if teamSlug == hasTeam.teamSlug {
+			if teamID == hasTeam.teamID {
 				wantPerm = teamData["permission"].(string)
 				break
 			}
 		}
 		if wantPerm == "" { // user should NOT have permission
-			log.Printf("[DEBUG] Removing team %s from repo: %s.", hasTeam.teamSlug, repoName)
-			_, err := client.Teams.RemoveTeamRepoBySlug(ctx, owner, hasTeam.teamSlug, owner, repoName)
-			if err != nil {
-				return err
-			}
+			remove = append(remove, hasTeam)
 		} else if wantPerm != hasTeam.permission { // permission should be updated
-			log.Printf("[DEBUG] Updating team %s permission from %s to %s for repo: %s.", hasTeam.teamSlug, hasTeam.permission, wantPerm, repoName)
-			_, err := client.Teams.AddTeamRepoBySlug(
-				ctx, owner, hasTeam.teamSlug, owner, repoName, &github.TeamAddTeamRepoOptions{
+			log.Printf("[DEBUG] Updating team %d permission from %s to %s for repo: %s.", hasTeam.teamID, hasTeam.permission, wantPerm, repoName)
+			_, err := client.Teams.AddTeamRepoByID(
+				ctx, orgID, hasTeam.teamID, owner, repoName, &github.TeamAddTeamRepoOptions{
 					Permission: wantPerm,
 				},
 			)
@@ -415,32 +423,41 @@ func matchTeamCollaborators(
 	for _, t := range want {
 		teamData := t.(map[string]interface{})
 		teamIDString := teamData["team_id"].(string)
-		teamSlug, err := getTeamSlug(teamIDString, meta)
+		teamID, err := getTeamID(teamIDString, meta)
 		if err != nil {
 			return err
 		}
-		permission := teamData["permission"].(string)
 		var found bool
 		for _, c := range has {
-			if teamSlug == c.teamSlug {
+			if teamID == c.teamID {
 				found = true
 				break
 			}
 		}
 		if found {
 			continue
 		}
+		permission := teamData["permission"].(string)
 		// team needs to be added
-		log.Printf("[DEBUG] Adding team %s with permission %s for repo: %s.", teamSlug, permission, repoName)
-		_, err = client.Teams.AddTeamRepoBySlug(
-			ctx, owner, teamSlug, owner, repoName, &github.TeamAddTeamRepoOptions{
+		log.Printf("[DEBUG] Adding team %s with permission %s for repo: %s.", teamIDString, permission, repoName)
+		_, err = client.Teams.AddTeamRepoByID(
+			ctx, orgID, teamID, owner, repoName, &github.TeamAddTeamRepoOptions{
 				Permission: permission,
 			},
 		)
 		if err != nil {
 			return err
 		}
 	}
+
+	for _, team := range remove {
+		log.Printf("[DEBUG] Removing team %d from repo: %s.", team.teamID, repoName)
+		_, err := client.Teams.RemoveTeamRepoByID(ctx, orgID, team.teamID, owner, repoName)
+		if err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 
@@ -454,6 +471,14 @@ func resourceGithubRepositoryCollaboratorsCreate(d *schema.ResourceData, meta in
 	repoName := d.Get("repository").(string)
 	ctx := context.Background()
 
+	teamsMap := make(map[string]struct{})
+	for _, team := range teams {
+		teamIDString := team.(map[string]interface{})["team_id"].(string)
+		if _, found := teamsMap[teamIDString]; found {
+			return fmt.Errorf("duplicate set member: %s", teamIDString)
+		}
+		teamsMap[teamIDString] = struct{}{}
+	}
 	usersMap := make(map[string]struct{})
 	for _, user := range users {
 		username := user.(map[string]interface{})["username"].(string)
@@ -462,26 +487,18 @@ func resourceGithubRepositoryCollaboratorsCreate(d *schema.ResourceData, meta in
 		}
 		usersMap[username] = struct{}{}
 	}
-	teamsMap := make(map[string]struct{})
-	for _, team := range teams {
-		teamID := team.(map[string]interface{})["team_id"].(string)
-		if _, found := teamsMap[teamID]; found {
-			return fmt.Errorf("duplicate set member: %s", teamID)
-		}
-		teamsMap[teamID] = struct{}{}
-	}
 
 	userCollaborators, invitations, teamCollaborators, err := listAllCollaborators(client, isOrg, ctx, owner, repoName)
 	if err != nil {
 		return deleteResourceOn404AndSwallow304OtherwiseReturnError(err, d, "repository collaborators (%s/%s)", owner, repoName)
 	}
 
-	err = matchUserCollaboratorsAndInvites(repoName, users, userCollaborators, invitations, meta)
+	err = matchTeamCollaborators(repoName, teams, teamCollaborators, meta)
 	if err != nil {
 		return err
 	}
 
-	err = matchTeamCollaborators(repoName, teams, teamCollaborators, meta)
+	err = matchUserCollaboratorsAndInvites(repoName, users, userCollaborators, invitations, meta)
 	if err != nil {
 		return err
 	}
@@ -509,6 +526,11 @@ func resourceGithubRepositoryCollaboratorsRead(d *schema.ResourceData, meta inte
 		invitationIds[i.username] = strconv.FormatInt(i.invitationID, 10)
 	}
 
+	teamIDs := make([]int64, len(teamCollaborators))
+	for i, t := range teamCollaborators {
+		teamIDs[i] = t.teamID
+	}
+
 	err = d.Set("repository", repoName)
 	if err != nil {
 		return err
@@ -517,7 +539,7 @@ func resourceGithubRepositoryCollaboratorsRead(d *schema.ResourceData, meta inte
 	if err != nil {
 		return err
 	}
-	err = d.Set("team", flattenTeamCollaborators(teamCollaborators))
+	err = d.Set("team", flattenTeamCollaborators(teamCollaborators, teamIDs))
 	if err != nil {
 		return err
 	}

github/resource_github_repository_collaborators_test.go

@@ -64,7 +64,7 @@ func TestAccGithubRepositoryCollaborators(t *testing.T) {
 			}
 
 			resource "github_team" "test" {
-				name = "%[1]s"
+				name = "test"
 			}
 
 			resource "github_repository_collaborators" "test_repo_collaborators" {
@@ -75,7 +75,7 @@ func TestAccGithubRepositoryCollaborators(t *testing.T) {
 					permission = "admin"
 				}
 				team {
-					team_id   = github_team.test.slug
+					team_id   = github_team.test.id
 					permission = "pull"
 				}
 			}
@@ -155,8 +155,8 @@ func TestAccGithubRepositoryCollaborators(t *testing.T) {
 						if strings.HasPrefix(name, "user.") && strings.HasSuffix(name, ".permission") && val != "admin" {
 							return fmt.Errorf("expected user.*.permission to be set to admin, was %s", val)
 						}
-						if strings.HasPrefix(name, "team.") && strings.HasSuffix(name, ".team_id") && val != teamAttrs["slug"] {
-							return fmt.Errorf("expected team.*.team_id to be set to %s, was %s", teamAttrs["slug"], val)
+						if strings.HasPrefix(name, "team.") && strings.HasSuffix(name, ".team_id") && val != teamAttrs["id"] {
+							return fmt.Errorf("expected team.*.team_id to be set to %s, was %s", teamAttrs["id"], val)
 						}
 						if strings.HasPrefix(name, "team.") && strings.HasSuffix(name, ".permission") && val != "pull" {
 							return fmt.Errorf("expected team.*.permission to be set to pull, was %s", val)
@@ -238,7 +238,11 @@ func TestAccGithubRepositoryCollaborators(t *testing.T) {
 			}
 
 			resource "github_team" "test" {
-				name = "%[1]s"
+				name = "test"
+			}
+
+			resource "github_team" "test2" {
+				name = "test2"
 			}
 
 			resource "github_repository_collaborators" "test_repo_collaborators" {
@@ -253,7 +257,11 @@ func TestAccGithubRepositoryCollaborators(t *testing.T) {
 					permission = "admin"
 				}
 				team {
-					team_id   = github_team.test.slug
+					team_id   = github_team.test.id
+					permission = "pull"
+				}
+				team {
+					team_id   = github_team.test2.id
 					permission = "pull"
 				}
 			}
@@ -267,7 +275,11 @@ func TestAccGithubRepositoryCollaborators(t *testing.T) {
 			}
 
 			resource "github_team" "test" {
-				name = "%[1]s"
+				name = "test"
+			}
+
+			resource "github_team" "test2" {
+				name = "test2"
 			}
 
 			resource "github_repository_collaborators" "test_repo_collaborators" {
@@ -278,7 +290,7 @@ func TestAccGithubRepositoryCollaborators(t *testing.T) {
 					permission = "push"
 				}
 				team {
-					team_id   = github_team.test.slug
+					team_id   = github_team.test.id
 					permission = "push"
 				}
 			}
@@ -361,8 +373,8 @@ func TestAccGithubRepositoryCollaborators(t *testing.T) {
 						if strings.HasPrefix(name, "user.") && strings.HasSuffix(name, ".permission") && val != "push" {
 							return fmt.Errorf("expected user.*.permission to be set to push, was %s", val)
 						}
-						if strings.HasPrefix(name, "team.") && strings.HasSuffix(name, ".team_id") && val != teamAttrs["slug"] {
-							return fmt.Errorf("expected team.*.team_id to be set to %s, was %s", teamAttrs["slug"], val)
+						if strings.HasPrefix(name, "team.") && strings.HasSuffix(name, ".team_id") && val != teamAttrs["id"] {
+							return fmt.Errorf("expected team.*.team_id to be set to %s, was %s", teamAttrs["id"], val)
 						}
 						if strings.HasPrefix(name, "team.") && strings.HasSuffix(name, ".permission") && val != "push" {
 							return fmt.Errorf("expected team.*.permission to be set to push, was %s", val)
@@ -436,7 +448,7 @@ func TestAccGithubRepositoryCollaborators(t *testing.T) {
 			}
 
 			resource "github_team" "test" {
-				name = "%[1]s"
+				name = "test"
 			}
 
 			resource "github_repository_collaborators" "test_repo_collaborators" {
@@ -451,7 +463,7 @@ func TestAccGithubRepositoryCollaborators(t *testing.T) {
 					permission = "admin"
 				}
 				team {
-					team_id   = github_team.test.slug
+					team_id   = github_team.test.id
 					permission = "pull"
 				}
 			}
@@ -465,9 +477,9 @@ func TestAccGithubRepositoryCollaborators(t *testing.T) {
 			}
 
 			resource "github_team" "test" {
-				name = "%[1]s"
+				name = "test"
 			}
-		`, repoName, inOrgUser)
+		`, repoName)
 
 		testCase := func(t *testing.T, mode, config, configUpdate string, testCheck func(state *terraform.State) error) {
 			resource.Test(t, resource.TestCase{