Skip to content
This repository was archived by the owner on Apr 15, 2026. It is now read-only.

Commit 7e93af4

Browse files
vmvarelavmvarela
committed
fix(enterprise-teams): address review findings
- Rename single-char variables: t→team, v→item (Rule #29) - Remove obvious comments that restate the code (Rule #30) - Fix 404 handling with errors.As instead of resp.StatusCode (Rule #13) - Migrate all tests from Check: to ConfigStateChecks: (Rule #31) - Fix docs verb forms: Get→Gets, List→Lists, Check→Checks (Rule #2) - Move listAllEnterpriseTeams to util_enterprise_teams.go for consistency - Add sidebar entries for all 7 resources/data sources - Add unit tests for utility functions (Rule #33)
1 parent d151eab commit 7e93af4

13 files changed

Lines changed: 282 additions & 102 deletions

‎github/data_source_github_enterprise_team_test.go‎

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,12 @@ import (
55
"os"
66
"testing"
77

8+
"github.com/hashicorp/terraform-plugin-testing/compare"
89
"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
910
"github.com/hashicorp/terraform-plugin-testing/helper/resource"
11+
"github.com/hashicorp/terraform-plugin-testing/knownvalue"
12+
"github.com/hashicorp/terraform-plugin-testing/statecheck"
13+
"github.com/hashicorp/terraform-plugin-testing/tfjsonpath"
1014
)
1115

1216
func TestAccGithubEnterpriseTeamDataSource(t *testing.T) {
@@ -33,12 +37,12 @@ func TestAccGithubEnterpriseTeamDataSource(t *testing.T) {
3337
slug = github_enterprise_team.test.slug
3438
}
3539
`, testAccConf.enterpriseSlug, testResourcePrefix, randomID),
36-
Check: resource.ComposeAggregateTestCheckFunc(
37-
resource.TestCheckResourceAttrSet("data.github_enterprise_team.by_slug", "id"),
38-
resource.TestCheckResourceAttrPair("data.github_enterprise_team.by_slug", "team_id", "github_enterprise_team.test", "team_id"),
39-
resource.TestCheckResourceAttrPair("data.github_enterprise_team.by_slug", "slug", "github_enterprise_team.test", "slug"),
40-
resource.TestCheckResourceAttrPair("data.github_enterprise_team.by_slug", "name", "github_enterprise_team.test", "name"),
41-
),
40+
ConfigStateChecks: []statecheck.StateCheck{
41+
statecheck.ExpectKnownValue("data.github_enterprise_team.by_slug", tfjsonpath.New("id"), knownvalue.NotNull()),
42+
statecheck.CompareValuePairs("data.github_enterprise_team.by_slug", tfjsonpath.New("team_id"), "github_enterprise_team.test", tfjsonpath.New("team_id"), compare.ValuesSame()),
43+
statecheck.CompareValuePairs("data.github_enterprise_team.by_slug", tfjsonpath.New("slug"), "github_enterprise_team.test", tfjsonpath.New("slug"), compare.ValuesSame()),
44+
statecheck.CompareValuePairs("data.github_enterprise_team.by_slug", tfjsonpath.New("name"), "github_enterprise_team.test", tfjsonpath.New("name"), compare.ValuesSame()),
45+
},
4246
},
4347
},
4448
})
@@ -67,11 +71,11 @@ func TestAccGithubEnterpriseTeamDataSource(t *testing.T) {
6771
team_id = github_enterprise_team.test.team_id
6872
}
6973
`, testAccConf.enterpriseSlug, testResourcePrefix, randomID),
70-
Check: resource.ComposeAggregateTestCheckFunc(
71-
resource.TestCheckResourceAttrSet("data.github_enterprise_team.by_id", "id"),
72-
resource.TestCheckResourceAttrPair("data.github_enterprise_team.by_id", "team_id", "github_enterprise_team.test", "team_id"),
73-
resource.TestCheckResourceAttrPair("data.github_enterprise_team.by_id", "slug", "github_enterprise_team.test", "slug"),
74-
),
74+
ConfigStateChecks: []statecheck.StateCheck{
75+
statecheck.ExpectKnownValue("data.github_enterprise_team.by_id", tfjsonpath.New("id"), knownvalue.NotNull()),
76+
statecheck.CompareValuePairs("data.github_enterprise_team.by_id", tfjsonpath.New("team_id"), "github_enterprise_team.test", tfjsonpath.New("team_id"), compare.ValuesSame()),
77+
statecheck.CompareValuePairs("data.github_enterprise_team.by_id", tfjsonpath.New("slug"), "github_enterprise_team.test", tfjsonpath.New("slug"), compare.ValuesSame()),
78+
},
7579
},
7680
},
7781
})
@@ -115,11 +119,11 @@ func TestAccGithubEnterpriseTeamOrganizationsDataSource(t *testing.T) {
115119
depends_on = [github_enterprise_team_organizations.assign]
116120
}
117121
`, testAccConf.enterpriseSlug, testResourcePrefix, randomID, orgSlug),
118-
Check: resource.ComposeAggregateTestCheckFunc(
119-
resource.TestCheckResourceAttrSet("data.github_enterprise_team_organizations.test", "id"),
120-
resource.TestCheckResourceAttr("data.github_enterprise_team_organizations.test", "organization_slugs.#", "1"),
121-
resource.TestCheckTypeSetElemAttr("data.github_enterprise_team_organizations.test", "organization_slugs.*", orgSlug),
122-
),
122+
ConfigStateChecks: []statecheck.StateCheck{
123+
statecheck.ExpectKnownValue("data.github_enterprise_team_organizations.test", tfjsonpath.New("id"), knownvalue.NotNull()),
124+
statecheck.ExpectKnownValue("data.github_enterprise_team_organizations.test", tfjsonpath.New("organization_slugs"), knownvalue.SetSizeExact(1)),
125+
statecheck.ExpectKnownValue("data.github_enterprise_team_organizations.test", tfjsonpath.New("organization_slugs"), knownvalue.SetPartial([]knownvalue.Check{knownvalue.StringExact(orgSlug)})),
126+
},
123127
},
124128
},
125129
})
@@ -163,10 +167,10 @@ func TestAccGithubEnterpriseTeamMembershipDataSource(t *testing.T) {
163167
depends_on = [github_enterprise_team_membership.test]
164168
}
165169
`, testAccConf.enterpriseSlug, testResourcePrefix, randomID, username, username),
166-
Check: resource.ComposeAggregateTestCheckFunc(
167-
resource.TestCheckResourceAttrSet("data.github_enterprise_team_membership.test", "id"),
168-
resource.TestCheckResourceAttr("data.github_enterprise_team_membership.test", "username", username),
169-
),
170+
ConfigStateChecks: []statecheck.StateCheck{
171+
statecheck.ExpectKnownValue("data.github_enterprise_team_membership.test", tfjsonpath.New("id"), knownvalue.NotNull()),
172+
statecheck.ExpectKnownValue("data.github_enterprise_team_membership.test", tfjsonpath.New("username"), knownvalue.StringExact(username)),
173+
},
170174
},
171175
},
172176
})

‎github/data_source_github_enterprise_teams.go‎

Lines changed: 10 additions & 31 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/v82/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"
@@ -83,27 +82,27 @@ func dataSourceGithubEnterpriseTeamsRead(ctx context.Context, d *schema.Resource
8382
}
8483

8584
flat := make([]any, 0, len(teams))
86-
for _, t := range teams {
85+
for _, team := range teams {
8786
m := map[string]any{
88-
teamIDKey: int(t.ID),
89-
teamSlugKey: t.Slug,
90-
teamNameKey: t.Name,
87+
teamIDKey: int(team.ID),
88+
teamSlugKey: team.Slug,
89+
teamNameKey: team.Name,
9190
}
92-
if t.Description != nil {
93-
m[teamDescriptionKey] = *t.Description
91+
if team.Description != nil {
92+
m[teamDescriptionKey] = *team.Description
9493
} else {
9594
m[teamDescriptionKey] = ""
9695
}
9796
orgSel := ""
98-
if t.OrganizationSelectionType != nil {
99-
orgSel = *t.OrganizationSelectionType
97+
if team.OrganizationSelectionType != nil {
98+
orgSel = *team.OrganizationSelectionType
10099
}
101100
if orgSel == "" {
102101
orgSel = "disabled"
103102
}
104103
m[teamOrganizationSelectionKey] = orgSel
105-
if t.GroupID != "" {
106-
m[teamGroupIDKey] = t.GroupID
104+
if team.GroupID != "" {
105+
m[teamGroupIDKey] = team.GroupID
107106
} else {
108107
m[teamGroupIDKey] = ""
109108
}
@@ -119,23 +118,3 @@ func dataSourceGithubEnterpriseTeamsRead(ctx context.Context, d *schema.Resource
119118
}
120119
return nil
121120
}
122-
123-
// listAllEnterpriseTeams returns all enterprise teams with pagination handled.
124-
func listAllEnterpriseTeams(ctx context.Context, client *github.Client, enterpriseSlug string) ([]*github.EnterpriseTeam, error) {
125-
var all []*github.EnterpriseTeam
126-
opt := &github.ListOptions{PerPage: maxPerPage}
127-
128-
for {
129-
teams, resp, err := client.Enterprise.ListTeams(ctx, enterpriseSlug, opt)
130-
if err != nil {
131-
return nil, err
132-
}
133-
all = append(all, teams...)
134-
if resp.NextPage == 0 {
135-
break
136-
}
137-
opt.Page = resp.NextPage
138-
}
139-
140-
return all, nil
141-
}

‎github/data_source_github_enterprise_teams_test.go‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ import (
66

77
"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
88
"github.com/hashicorp/terraform-plugin-testing/helper/resource"
9+
"github.com/hashicorp/terraform-plugin-testing/knownvalue"
10+
"github.com/hashicorp/terraform-plugin-testing/statecheck"
11+
"github.com/hashicorp/terraform-plugin-testing/tfjsonpath"
912
)
1013

1114
func TestAccGithubEnterpriseTeamsDataSource(t *testing.T) {
@@ -32,12 +35,16 @@ func TestAccGithubEnterpriseTeamsDataSource(t *testing.T) {
3235
depends_on = [github_enterprise_team.test]
3336
}
3437
`, testAccConf.enterpriseSlug, testResourcePrefix, randomID),
35-
Check: resource.ComposeAggregateTestCheckFunc(
36-
resource.TestCheckResourceAttrSet("data.github_enterprise_teams.all", "id"),
37-
resource.TestCheckResourceAttrSet("data.github_enterprise_teams.all", "teams.0.team_id"),
38-
resource.TestCheckResourceAttrSet("data.github_enterprise_teams.all", "teams.0.slug"),
39-
resource.TestCheckResourceAttrSet("data.github_enterprise_teams.all", "teams.0.name"),
40-
),
38+
ConfigStateChecks: []statecheck.StateCheck{
39+
statecheck.ExpectKnownValue("data.github_enterprise_teams.all", tfjsonpath.New("id"), knownvalue.NotNull()),
40+
statecheck.ExpectKnownValue("data.github_enterprise_teams.all", tfjsonpath.New("teams"), knownvalue.ListPartial(map[int]knownvalue.Check{
41+
0: knownvalue.ObjectPartial(map[string]knownvalue.Check{
42+
"team_id": knownvalue.NotNull(),
43+
"slug": knownvalue.NotNull(),
44+
"name": knownvalue.NotNull(),
45+
}),
46+
})),
47+
},
4148
},
4249
},
4350
})

‎github/resource_github_enterprise_team_membership.go‎

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

33
import (
44
"context"
5+
"errors"
6+
"net/http"
57
"strings"
68

79
"github.com/google/go-github/v82/github"
@@ -64,7 +66,6 @@ func resourceGithubEnterpriseTeamMembershipCreate(ctx context.Context, d *schema
6466
enterpriseSlug := strings.TrimSpace(d.Get("enterprise_slug").(string))
6567
username := strings.TrimSpace(d.Get("username").(string))
6668

67-
// Get team by slug or ID
6869
var team *github.EnterpriseTeam
6970
var err error
7071
if v, ok := d.GetOk("team_slug"); ok {
@@ -80,7 +81,6 @@ func resourceGithubEnterpriseTeamMembershipCreate(ctx context.Context, d *schema
8081
return diag.Errorf("enterprise team not found")
8182
}
8283

83-
// Add the user to the team using the SDK
8484
user, _, err := client.Enterprise.AddTeamMember(ctx, enterpriseSlug, team.Slug, username)
8585
if err != nil {
8686
return diag.FromErr(err)
@@ -129,10 +129,14 @@ func resourceGithubEnterpriseTeamMembershipRead(ctx context.Context, d *schema.R
129129
}
130130
}
131131

132-
// Get the membership using the SDK
133132
user, resp, err := client.Enterprise.GetTeamMembership(ctx, enterpriseSlug, teamSlug, username)
134133
if err != nil {
135-
if resp != nil && resp.StatusCode == 404 {
134+
var ghErr *github.ErrorResponse
135+
if errors.As(err, &ghErr) && ghErr.Response.StatusCode == http.StatusNotFound {
136+
d.SetId("")
137+
return nil
138+
}
139+
if resp != nil && resp.StatusCode == http.StatusNotFound {
136140
d.SetId("")
137141
return nil
138142
}
@@ -186,11 +190,13 @@ func resourceGithubEnterpriseTeamMembershipDelete(ctx context.Context, d *schema
186190
}
187191
}
188192

189-
// Remove the user from the team using the SDK
190193
resp, err := client.Enterprise.RemoveTeamMember(ctx, enterpriseSlug, teamSlug, username)
191194
if err != nil {
192-
// Already gone? That's fine, we wanted it deleted anyway.
193-
if resp != nil && resp.StatusCode == 404 {
195+
var ghErr *github.ErrorResponse
196+
if errors.As(err, &ghErr) && ghErr.Response.StatusCode == http.StatusNotFound {
197+
return nil
198+
}
199+
if resp != nil && resp.StatusCode == http.StatusNotFound {
194200
return nil
195201
}
196202
return diag.FromErr(err)

‎github/resource_github_enterprise_team_organizations.go‎

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

33
import (
44
"context"
5+
"errors"
6+
"net/http"
57
"strings"
68

79
"github.com/google/go-github/v82/github"
@@ -60,7 +62,6 @@ func resourceGithubEnterpriseTeamOrganizationsCreate(ctx context.Context, d *sch
6062
client := meta.(*Owner).v3client
6163
enterpriseSlug := strings.TrimSpace(d.Get("enterprise_slug").(string))
6264

63-
// Get team by slug or ID
6465
var team *github.EnterpriseTeam
6566
var err error
6667
if v, ok := d.GetOk("team_slug"); ok {
@@ -78,11 +79,10 @@ func resourceGithubEnterpriseTeamOrganizationsCreate(ctx context.Context, d *sch
7879

7980
orgSlugsSet := d.Get("organization_slugs").(*schema.Set)
8081
orgSlugs := make([]string, 0, orgSlugsSet.Len())
81-
for _, v := range orgSlugsSet.List() {
82-
orgSlugs = append(orgSlugs, v.(string))
82+
for _, item := range orgSlugsSet.List() {
83+
orgSlugs = append(orgSlugs, item.(string))
8384
}
8485

85-
// Add organizations to the team using the SDK
8686
_, _, err = client.Enterprise.AddMultipleAssignments(ctx, enterpriseSlug, team.Slug, orgSlugs)
8787
if err != nil {
8888
return diag.FromErr(err)
@@ -160,23 +160,21 @@ func resourceGithubEnterpriseTeamOrganizationsUpdate(ctx context.Context, d *sch
160160
toAdd := newSet.Difference(oldSet)
161161
toRemove := oldSet.Difference(newSet)
162162

163-
// Add new organizations
164163
if toAdd.Len() > 0 {
165164
addSlugs := make([]string, 0, toAdd.Len())
166-
for _, v := range toAdd.List() {
167-
addSlugs = append(addSlugs, v.(string))
165+
for _, item := range toAdd.List() {
166+
addSlugs = append(addSlugs, item.(string))
168167
}
169168
_, _, err = client.Enterprise.AddMultipleAssignments(ctx, enterpriseSlug, teamSlug, addSlugs)
170169
if err != nil {
171170
return diag.FromErr(err)
172171
}
173172
}
174173

175-
// Remove old organizations
176174
if toRemove.Len() > 0 {
177175
removeSlugs := make([]string, 0, toRemove.Len())
178-
for _, v := range toRemove.List() {
179-
removeSlugs = append(removeSlugs, v.(string))
176+
for _, item := range toRemove.List() {
177+
removeSlugs = append(removeSlugs, item.(string))
180178
}
181179
_, _, err = client.Enterprise.RemoveMultipleAssignments(ctx, enterpriseSlug, teamSlug, removeSlugs)
182180
if err != nil {
@@ -195,17 +193,19 @@ func resourceGithubEnterpriseTeamOrganizationsDelete(ctx context.Context, d *sch
195193
return diag.FromErr(err)
196194
}
197195

198-
// Get organizations from state
199196
orgSlugsSet := d.Get("organization_slugs").(*schema.Set)
200197
if orgSlugsSet.Len() > 0 {
201198
removeSlugs := make([]string, 0, orgSlugsSet.Len())
202-
for _, v := range orgSlugsSet.List() {
203-
removeSlugs = append(removeSlugs, v.(string))
199+
for _, item := range orgSlugsSet.List() {
200+
removeSlugs = append(removeSlugs, item.(string))
204201
}
205202
_, resp, err := client.Enterprise.RemoveMultipleAssignments(ctx, enterpriseSlug, teamSlug, removeSlugs)
206203
if err != nil {
207-
// Already gone? That's fine, we wanted it deleted anyway.
208-
if resp != nil && resp.StatusCode == 404 {
204+
var ghErr *github.ErrorResponse
205+
if errors.As(err, &ghErr) && ghErr.Response.StatusCode == http.StatusNotFound {
206+
return nil
207+
}
208+
if resp != nil && resp.StatusCode == http.StatusNotFound {
209209
return nil
210210
}
211211
return diag.FromErr(err)

‎github/resource_github_enterprise_team_test.go‎

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ import (
88

99
"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
1010
"github.com/hashicorp/terraform-plugin-testing/helper/resource"
11+
"github.com/hashicorp/terraform-plugin-testing/knownvalue"
12+
"github.com/hashicorp/terraform-plugin-testing/statecheck"
13+
"github.com/hashicorp/terraform-plugin-testing/tfjsonpath"
1114
)
1215

1316
func TestAccGithubEnterpriseTeam(t *testing.T) {
@@ -31,11 +34,11 @@ func TestAccGithubEnterpriseTeam(t *testing.T) {
3134
organization_selection_type = "disabled"
3235
}
3336
`, testAccConf.enterpriseSlug, testResourcePrefix, randomID),
34-
Check: resource.ComposeAggregateTestCheckFunc(
35-
resource.TestCheckResourceAttrSet("github_enterprise_team.test", "slug"),
36-
resource.TestCheckResourceAttrSet("github_enterprise_team.test", "team_id"),
37-
resource.TestCheckResourceAttr("github_enterprise_team.test", "organization_selection_type", "disabled"),
38-
),
37+
ConfigStateChecks: []statecheck.StateCheck{
38+
statecheck.ExpectKnownValue("github_enterprise_team.test", tfjsonpath.New("slug"), knownvalue.NotNull()),
39+
statecheck.ExpectKnownValue("github_enterprise_team.test", tfjsonpath.New("team_id"), knownvalue.NotNull()),
40+
statecheck.ExpectKnownValue("github_enterprise_team.test", tfjsonpath.New("organization_selection_type"), knownvalue.StringExact("disabled")),
41+
},
3942
},
4043
{
4144
Config: fmt.Sprintf(`
@@ -50,10 +53,10 @@ func TestAccGithubEnterpriseTeam(t *testing.T) {
5053
organization_selection_type = "selected"
5154
}
5255
`, testAccConf.enterpriseSlug, testResourcePrefix, randomID),
53-
Check: resource.ComposeAggregateTestCheckFunc(
54-
resource.TestCheckResourceAttr("github_enterprise_team.test", "description", "updated description"),
55-
resource.TestCheckResourceAttr("github_enterprise_team.test", "organization_selection_type", "selected"),
56-
),
56+
ConfigStateChecks: []statecheck.StateCheck{
57+
statecheck.ExpectKnownValue("github_enterprise_team.test", tfjsonpath.New("description"), knownvalue.StringExact("updated description")),
58+
statecheck.ExpectKnownValue("github_enterprise_team.test", tfjsonpath.New("organization_selection_type"), knownvalue.StringExact("selected")),
59+
},
5760
},
5861
},
5962
})
@@ -123,10 +126,10 @@ func TestAccGithubEnterpriseTeamOrganizations(t *testing.T) {
123126
organization_slugs = [%q]
124127
}
125128
`, testAccConf.enterpriseSlug, testResourcePrefix, randomID, orgSlug),
126-
Check: resource.ComposeAggregateTestCheckFunc(
127-
resource.TestCheckResourceAttr("github_enterprise_team_organizations.test", "organization_slugs.#", "1"),
128-
resource.TestCheckTypeSetElemAttr("github_enterprise_team_organizations.test", "organization_slugs.*", orgSlug),
129-
),
129+
ConfigStateChecks: []statecheck.StateCheck{
130+
statecheck.ExpectKnownValue("github_enterprise_team_organizations.test", tfjsonpath.New("organization_slugs"), knownvalue.SetSizeExact(1)),
131+
statecheck.ExpectKnownValue("github_enterprise_team_organizations.test", tfjsonpath.New("organization_slugs"), knownvalue.SetPartial([]knownvalue.Check{knownvalue.StringExact(orgSlug)})),
132+
},
130133
},
131134
},
132135
})
@@ -229,9 +232,9 @@ func TestAccGithubEnterpriseTeamMembership(t *testing.T) {
229232
username = %q
230233
}
231234
`, testAccConf.enterpriseSlug, testResourcePrefix, randomID, username),
232-
Check: resource.ComposeAggregateTestCheckFunc(
233-
resource.TestCheckResourceAttr("github_enterprise_team_membership.test", "username", username),
234-
),
235+
ConfigStateChecks: []statecheck.StateCheck{
236+
statecheck.ExpectKnownValue("github_enterprise_team_membership.test", tfjsonpath.New("username"), knownvalue.StringExact(username)),
237+
},
235238
},
236239
},
237240
})

0 commit comments

Comments
 (0)