Skip to content

Commit 9969a2f

Browse files
feat(enterprise_team): address PR review feedback and improve code quality
Address code review feedback from @deiga and improve enterprise team resources. ## Changes ### Schema improvements - Separate enterprise_team field into team_slug and team_id with ExactlyOneOf validation for membership and organizations resources - Align data source naming with resources (enterprise_team → team_slug) - Fix ExactlyOneOf in data_source_github_enterprise_team (must include both fields) - Add validation for slug field in data source ### Code quality - Remove Read-after-Create/Update pattern, set computed fields directly from API response - Remove unused findEnterpriseTeamBySlugOrID function - Move shared helpers to util_enterprise_teams.go - Use "/" as ID separator instead of ":" (team slugs contain ":") - Handle errors properly in Import function ### Bug fixes - Add 404 handling in Delete for membership and organizations resources - Fix group_id clearing (now sends empty string to API) - Preserve team_id in state when user configures via team_id - Fix Read drift when user configures team_id instead of team_slug ### Testing - Eliminate redundant testCase wrapper pattern in tests - Add MinItems validation test for organization_slugs - All 8 acceptance tests passing ### Documentation - Update docs for new team_slug/team_id schema
1 parent 4103e9d commit 9969a2f

14 files changed

+303
-218
lines changed

github/data_source_github_enterprise_team.go

Lines changed: 7 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package github
22

33
import (
44
"context"
5-
"fmt"
65
"strconv"
76
"strings"
87

@@ -25,16 +24,18 @@ func dataSourceGithubEnterpriseTeam() *schema.Resource {
2524
ValidateDiagFunc: validation.ToDiagFunc(validation.All(validation.StringIsNotWhiteSpace, validation.StringIsNotEmpty)),
2625
},
2726
"slug": {
28-
Type: schema.TypeString,
29-
Optional: true,
30-
Computed: true,
31-
ExactlyOneOf: []string{"team_id"},
32-
Description: "The slug of the enterprise team.",
27+
Type: schema.TypeString,
28+
Optional: true,
29+
Computed: true,
30+
ExactlyOneOf: []string{"slug", "team_id"},
31+
Description: "The slug of the enterprise team.",
32+
ValidateDiagFunc: validation.ToDiagFunc(validation.All(validation.StringIsNotWhiteSpace, validation.StringIsNotEmpty)),
3333
},
3434
"team_id": {
3535
Type: schema.TypeInt,
3636
Optional: true,
3737
Computed: true,
38+
ExactlyOneOf: []string{"slug", "team_id"},
3839
Description: "The numeric ID of the enterprise team.",
3940
ValidateDiagFunc: validation.ToDiagFunc(validation.IntAtLeast(1)),
4041
},
@@ -137,22 +138,3 @@ func dataSourceGithubEnterpriseTeamRead(ctx context.Context, d *schema.ResourceD
137138

138139
return nil
139140
}
140-
141-
// findEnterpriseTeamBySlugOrID finds a team by slug. If the slug looks like a numeric ID,
142-
// it will search for a team with that ID. Otherwise, it uses GetTeam to fetch directly by slug.
143-
func findEnterpriseTeamBySlugOrID(ctx context.Context, client *github.Client, enterpriseSlug, teamSlugOrID string) (*github.EnterpriseTeam, error) {
144-
// First, try to get the team directly by slug
145-
team, resp, err := client.Enterprise.GetTeam(ctx, enterpriseSlug, teamSlugOrID)
146-
if err == nil {
147-
return team, nil
148-
}
149-
// If we got a 404, try searching by ID in case it's a numeric ID
150-
if resp != nil && resp.StatusCode == 404 {
151-
// Try to parse as int64 and search by ID
152-
var id int64
153-
if _, scanErr := fmt.Sscanf(teamSlugOrID, "%d", &id); scanErr == nil {
154-
return findEnterpriseTeamByID(ctx, client, enterpriseSlug, id)
155-
}
156-
}
157-
return nil, err
158-
}

github/data_source_github_enterprise_team_membership.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,10 @@ func dataSourceGithubEnterpriseTeamMembership() *schema.Resource {
2121
Description: "The slug of the enterprise.",
2222
ValidateDiagFunc: validation.ToDiagFunc(validation.All(validation.StringIsNotWhiteSpace, validation.StringIsNotEmpty)),
2323
},
24-
"enterprise_team": {
24+
"team_slug": {
2525
Type: schema.TypeString,
2626
Required: true,
27-
Description: "The slug or ID of the enterprise team.",
27+
Description: "The slug of the enterprise team.",
2828
ValidateDiagFunc: validation.ToDiagFunc(validation.All(validation.StringIsNotWhiteSpace, validation.StringIsNotEmpty)),
2929
},
3030
"username": {
@@ -45,20 +45,20 @@ func dataSourceGithubEnterpriseTeamMembership() *schema.Resource {
4545
func dataSourceGithubEnterpriseTeamMembershipRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
4646
client := meta.(*Owner).v3client
4747
enterpriseSlug := strings.TrimSpace(d.Get("enterprise_slug").(string))
48-
enterpriseTeam := strings.TrimSpace(d.Get("enterprise_team").(string))
48+
teamSlug := strings.TrimSpace(d.Get("team_slug").(string))
4949
username := strings.TrimSpace(d.Get("username").(string))
5050

5151
// Get the membership using the SDK
52-
user, _, err := client.Enterprise.GetTeamMembership(ctx, enterpriseSlug, enterpriseTeam, username)
52+
user, _, err := client.Enterprise.GetTeamMembership(ctx, enterpriseSlug, teamSlug, username)
5353
if err != nil {
5454
return diag.FromErr(err)
5555
}
5656

57-
d.SetId(buildThreePartID(enterpriseSlug, enterpriseTeam, username))
57+
d.SetId(buildEnterpriseTeamMembershipID(enterpriseSlug, teamSlug, username))
5858
if err := d.Set("enterprise_slug", enterpriseSlug); err != nil {
5959
return diag.FromErr(err)
6060
}
61-
if err := d.Set("enterprise_team", enterpriseTeam); err != nil {
61+
if err := d.Set("team_slug", teamSlug); err != nil {
6262
return diag.FromErr(err)
6363
}
6464
if err := d.Set("username", username); err != nil {

github/data_source_github_enterprise_team_organizations.go

Lines changed: 6 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"strings"
66

7-
"github.com/google/go-github/v81/github"
87
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
98
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
109
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
@@ -22,10 +21,10 @@ func dataSourceGithubEnterpriseTeamOrganizations() *schema.Resource {
2221
Description: "The slug of the enterprise.",
2322
ValidateDiagFunc: validation.ToDiagFunc(validation.All(validation.StringIsNotWhiteSpace, validation.StringIsNotEmpty)),
2423
},
25-
"enterprise_team": {
24+
"team_slug": {
2625
Type: schema.TypeString,
2726
Required: true,
28-
Description: "The slug or ID of the enterprise team.",
27+
Description: "The slug of the enterprise team.",
2928
ValidateDiagFunc: validation.ToDiagFunc(validation.All(validation.StringIsNotWhiteSpace, validation.StringIsNotEmpty)),
3029
},
3130
"organization_slugs": {
@@ -42,8 +41,8 @@ func dataSourceGithubEnterpriseTeamOrganizations() *schema.Resource {
4241
func dataSourceGithubEnterpriseTeamOrganizationsRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
4342
client := meta.(*Owner).v3client
4443
enterpriseSlug := strings.TrimSpace(d.Get("enterprise_slug").(string))
45-
enterpriseTeam := strings.TrimSpace(d.Get("enterprise_team").(string))
46-
orgs, err := listAllEnterpriseTeamOrganizations(ctx, client, enterpriseSlug, enterpriseTeam)
44+
teamSlug := strings.TrimSpace(d.Get("team_slug").(string))
45+
orgs, err := listAllEnterpriseTeamOrganizations(ctx, client, enterpriseSlug, teamSlug)
4746
if err != nil {
4847
return diag.FromErr(err)
4948
}
@@ -55,35 +54,15 @@ func dataSourceGithubEnterpriseTeamOrganizationsRead(ctx context.Context, d *sch
5554
}
5655
}
5756

58-
d.SetId(buildTwoPartID(enterpriseSlug, enterpriseTeam))
57+
d.SetId(buildEnterpriseTeamOrganizationsID(enterpriseSlug, teamSlug))
5958
if err := d.Set("enterprise_slug", enterpriseSlug); err != nil {
6059
return diag.FromErr(err)
6160
}
62-
if err := d.Set("enterprise_team", enterpriseTeam); err != nil {
61+
if err := d.Set("team_slug", teamSlug); err != nil {
6362
return diag.FromErr(err)
6463
}
6564
if err := d.Set("organization_slugs", slugs); err != nil {
6665
return diag.FromErr(err)
6766
}
6867
return nil
6968
}
70-
71-
// listAllEnterpriseTeamOrganizations returns all organizations assigned to an enterprise team with pagination handled.
72-
func listAllEnterpriseTeamOrganizations(ctx context.Context, client *github.Client, enterpriseSlug, enterpriseTeam string) ([]*github.Organization, error) {
73-
var all []*github.Organization
74-
opt := &github.ListOptions{PerPage: maxPerPage}
75-
76-
for {
77-
orgs, resp, err := client.Enterprise.ListAssignments(ctx, enterpriseSlug, enterpriseTeam, opt)
78-
if err != nil {
79-
return nil, err
80-
}
81-
all = append(all, orgs...)
82-
if resp.NextPage == 0 {
83-
break
84-
}
85-
opt.Page = resp.NextPage
86-
}
87-
88-
return all, nil
89-
}

github/data_source_github_enterprise_team_test.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ func TestAccGithubEnterpriseTeamDataSource(t *testing.T) {
3333
`, testAccConf.enterpriseSlug, randomID)
3434

3535
resource.Test(t, resource.TestCase{
36-
PreCheck: func() { skipUnlessEnterprise(t) },
37-
Providers: testAccProviders,
36+
PreCheck: func() { skipUnlessMode(t, enterprise) },
37+
ProviderFactories: providerFactories,
3838
Steps: []resource.TestStep{
3939
{
4040
Config: config,
@@ -68,20 +68,20 @@ func TestAccGithubEnterpriseTeamOrganizationsDataSource(t *testing.T) {
6868
6969
resource "github_enterprise_team_organizations" "assign" {
7070
enterprise_slug = data.github_enterprise.enterprise.slug
71-
enterprise_team = github_enterprise_team.test.slug
71+
team_slug = github_enterprise_team.test.slug
7272
organization_slugs = ["%s"]
7373
}
7474
7575
data "github_enterprise_team_organizations" "test" {
7676
enterprise_slug = data.github_enterprise.enterprise.slug
77-
enterprise_team = github_enterprise_team.test.slug
77+
team_slug = github_enterprise_team.test.slug
7878
depends_on = [github_enterprise_team_organizations.assign]
7979
}
8080
`, testAccConf.enterpriseSlug, testResourcePrefix, randomID, testAccConf.owner)
8181

8282
resource.Test(t, resource.TestCase{
83-
PreCheck: func() { skipUnlessEnterprise(t) },
84-
Providers: testAccProviders,
83+
PreCheck: func() { skipUnlessMode(t, enterprise) },
84+
ProviderFactories: providerFactories,
8585
Steps: []resource.TestStep{
8686
{
8787
Config: config,
@@ -111,21 +111,21 @@ func TestAccGithubEnterpriseTeamMembershipDataSource(t *testing.T) {
111111
112112
resource "github_enterprise_team_membership" "test" {
113113
enterprise_slug = data.github_enterprise.enterprise.slug
114-
enterprise_team = github_enterprise_team.test.slug
114+
team_slug = github_enterprise_team.test.slug
115115
username = "%s"
116116
}
117117
118118
data "github_enterprise_team_membership" "test" {
119119
enterprise_slug = data.github_enterprise.enterprise.slug
120-
enterprise_team = github_enterprise_team.test.slug
120+
team_slug = github_enterprise_team.test.slug
121121
username = "%s"
122122
depends_on = [github_enterprise_team_membership.test]
123123
}
124124
`, testAccConf.enterpriseSlug, testResourcePrefix, randomID, username, username)
125125

126126
resource.Test(t, resource.TestCase{
127-
PreCheck: func() { skipUnlessEnterprise(t) },
128-
Providers: testAccProviders,
127+
PreCheck: func() { skipUnlessMode(t, enterprise) },
128+
ProviderFactories: providerFactories,
129129
Steps: []resource.TestStep{
130130
{
131131
Config: config,

github/data_source_github_enterprise_teams_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ func TestAccGithubEnterpriseTeamsDataSource(t *testing.T) {
2828
`, testAccConf.enterpriseSlug, testResourcePrefix, randomID)
2929

3030
resource.Test(t, resource.TestCase{
31-
PreCheck: func() { skipUnlessEnterprise(t) },
32-
Providers: testAccProviders,
31+
PreCheck: func() { skipUnlessMode(t, enterprise) },
32+
ProviderFactories: providerFactories,
3333
Steps: []resource.TestStep{
3434
{
3535
Config: config,

github/resource_github_enterprise_team.go

Lines changed: 24 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,7 @@ func resourceGithubEnterpriseTeamCreate(ctx context.Context, d *schema.ResourceD
9292
Name: name,
9393
Description: github.Ptr(description),
9494
OrganizationSelectionType: github.Ptr(orgSelection),
95-
}
96-
if groupID != "" {
97-
req.GroupID = github.Ptr(groupID)
95+
GroupID: github.Ptr(groupID), // Empty string is valid for no group
9896
}
9997

10098
ctx = context.WithValue(ctx, ctxId, d.Id())
@@ -104,7 +102,16 @@ func resourceGithubEnterpriseTeamCreate(ctx context.Context, d *schema.ResourceD
104102
}
105103

106104
d.SetId(strconv.FormatInt(te.ID, 10))
107-
return resourceGithubEnterpriseTeamRead(context.WithValue(ctx, ctxId, d.Id()), d, meta)
105+
106+
// Set computed fields directly from API response
107+
if err := d.Set("slug", te.Slug); err != nil {
108+
return diag.FromErr(err)
109+
}
110+
if err := d.Set("team_id", int(te.ID)); err != nil {
111+
return diag.FromErr(err)
112+
}
113+
114+
return nil
108115
}
109116

110117
func resourceGithubEnterpriseTeamRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
@@ -194,24 +201,7 @@ func resourceGithubEnterpriseTeamRead(ctx context.Context, d *schema.ResourceDat
194201
func resourceGithubEnterpriseTeamUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
195202
client := meta.(*Owner).v3client
196203
enterpriseSlug := d.Get("enterprise_slug").(string)
197-
198-
// We need a team slug for the API. If state is missing, re-discover it by ID.
199-
teamSlug := strings.TrimSpace(d.Get("slug").(string))
200-
if teamSlug == "" {
201-
teamID, err := strconv.ParseInt(d.Id(), 10, 64)
202-
if err != nil {
203-
return diag.FromErr(unconvertibleIdErr(d.Id(), err))
204-
}
205-
ctx = context.WithValue(ctx, ctxId, d.Id())
206-
te, err := findEnterpriseTeamByID(ctx, client, enterpriseSlug, teamID)
207-
if err != nil {
208-
return diag.FromErr(err)
209-
}
210-
if te == nil {
211-
return diag.FromErr(fmt.Errorf("enterprise team %s no longer exists", d.Id()))
212-
}
213-
teamSlug = te.Slug
214-
}
204+
teamSlug := d.Get("slug").(string)
215205

216206
name := d.Get("name").(string)
217207
description := d.Get("description").(string)
@@ -222,18 +212,21 @@ func resourceGithubEnterpriseTeamUpdate(ctx context.Context, d *schema.ResourceD
222212
Name: name,
223213
Description: github.Ptr(description),
224214
OrganizationSelectionType: github.Ptr(orgSelection),
225-
}
226-
if groupID != "" {
227-
req.GroupID = github.Ptr(groupID)
215+
GroupID: github.Ptr(groupID), // Empty string clears the group
228216
}
229217

230218
ctx = context.WithValue(ctx, ctxId, d.Id())
231-
_, _, err := client.Enterprise.UpdateTeam(ctx, enterpriseSlug, teamSlug, req)
219+
te, _, err := client.Enterprise.UpdateTeam(ctx, enterpriseSlug, teamSlug, req)
232220
if err != nil {
233221
return diag.FromErr(err)
234222
}
235223

236-
return resourceGithubEnterpriseTeamRead(ctx, d, meta)
224+
// Update slug in case it changed (e.g., team was renamed)
225+
if err := d.Set("slug", te.Slug); err != nil {
226+
return diag.FromErr(err)
227+
}
228+
229+
return nil
237230
}
238231

239232
func resourceGithubEnterpriseTeamDelete(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
@@ -258,14 +251,13 @@ func resourceGithubEnterpriseTeamDelete(ctx context.Context, d *schema.ResourceD
258251
}
259252

260253
log.Printf("[INFO] Deleting enterprise team: %s/%s (%s)", enterpriseSlug, teamSlug, d.Id())
261-
resp, err := client.Enterprise.DeleteTeam(ctx, enterpriseSlug, teamSlug)
254+
_, err := client.Enterprise.DeleteTeam(ctx, enterpriseSlug, teamSlug)
262255
if err != nil {
263256
// Already gone? That's fine, we wanted it deleted anyway.
264257
ghErr := &github.ErrorResponse{}
265258
if errors.As(err, &ghErr) && ghErr.Response.StatusCode == http.StatusNotFound {
266259
return nil
267260
}
268-
_ = resp
269261
return diag.FromErr(err)
270262
}
271263

@@ -281,6 +273,8 @@ func resourceGithubEnterpriseTeamImport(_ context.Context, d *schema.ResourceDat
281273

282274
enterpriseSlug, teamID := parts[0], parts[1]
283275
d.SetId(teamID)
284-
_ = d.Set("enterprise_slug", enterpriseSlug)
276+
if err := d.Set("enterprise_slug", enterpriseSlug); err != nil {
277+
return nil, err
278+
}
285279
return []*schema.ResourceData{d}, nil
286280
}

0 commit comments

Comments
 (0)