diff --git a/github/acc_test.go b/github/acc_test.go index f984be6c76..98df8d069d 100644 --- a/github/acc_test.go +++ b/github/acc_test.go @@ -407,8 +407,8 @@ func skipUnlessHasOrgUser2(t *testing.T) { } } -// func skipUnlessHasOrgUser3(t *testing.T) { -// if testAccConf.testOrgUser3 == "" { -// t.Skip("Skipping as no test org user 3 is configured") -// } -// } +func skipUnlessHasOrgUser3(t *testing.T) { + if testAccConf.testOrgUser3 == "" { + t.Skip("Skipping as no test org user 3 is configured") + } +} diff --git a/github/resource_github_repository_collaborators.go b/github/resource_github_repository_collaborators.go index b82322c4f8..4c6253dca5 100644 --- a/github/resource_github_repository_collaborators.go +++ b/github/resource_github_repository_collaborators.go @@ -137,29 +137,8 @@ func resourceGithubRepositoryCollaboratorsDiff(ctx context.Context, d *schema.Re meta, _ := m.(*Owner) - if d.HasChange("user") && d.NewValueKnown("user") { - v, diags := d.GetRawConfigAt(cty.GetAttrPath("user")) - if diags.HasError() { - return fmt.Errorf("error reading user config: %v", diags) - } - - if !v.IsNull() && v.IsKnown() { - seen := make(map[string]struct{}) - it := v.ElementIterator() - for it.Next() { - _, elem := it.Element() - val := elem.GetAttr("username") - if val.IsNull() || !val.IsKnown() { - continue - } - - username := strings.ToLower(val.AsString()) - if _, ok := seen[username]; ok { - return fmt.Errorf("duplicate user %s found in user collaborators", username) - } - seen[username] = struct{}{} - } - } + if err := diffDuplicateUsernameCheck(ctx, d, "user"); err != nil { + return fmt.Errorf("error diffing user config: %w", err) } if d.HasChange("team") && d.NewValueKnown("team") { diff --git a/github/resource_github_team_members.go b/github/resource_github_team_members.go index e56bf0ac38..d08efc98ba 100644 --- a/github/resource_github_team_members.go +++ b/github/resource_github_team_members.go @@ -12,7 +12,6 @@ import ( "github.com/google/go-github/v89/github" "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/shurcooL/githubv4" @@ -28,7 +27,7 @@ func resourceGithubTeamMembers() *schema.Resource { StateContext: resourceGithubTeamMembersImport, }, - CustomizeDiff: customdiff.Sequence(diffLegacyTeamID, diffLegacyTeam), + CustomizeDiff: resourceGithubTeamMembersDiff, SchemaVersion: 1, StateUpgraders: []schema.StateUpgrader{ @@ -85,6 +84,24 @@ func resourceGithubTeamMembers() *schema.Resource { } } +func resourceGithubTeamMembersDiff(ctx context.Context, d *schema.ResourceDiff, m any) error { + tflog.Debug(ctx, "diffing team members") + + if err := diffDuplicateUsernameCheck(ctx, d, "members"); err != nil { + return fmt.Errorf("error diffing members config: %w", err) + } + + if err := diffLegacyTeamID(ctx, d, m); err != nil { + return fmt.Errorf("error diffing legacy team ID: %w", err) + } + + if err := diffLegacyTeam(ctx, d, m); err != nil { + return fmt.Errorf("error diffing legacy team: %w", err) + } + + return nil +} + func resourceGithubTeamMembersCreate(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnostics { meta, _ := m.(*Owner) client := meta.v3client @@ -400,11 +417,12 @@ func updateTeamMembers(ctx context.Context, meta *Owner, slug string, wantMember } for _, member := range currentMembers { - if _, ok := want[member.login]; !ok { - tflog.Debug(ctx, "Removing team member.", map[string]any{"team_slug": slug, "username": member.login}) + login := strings.ToLower(member.login) + if _, ok := want[login]; !ok { + tflog.Debug(ctx, "Removing team member.", map[string]any{"team_slug": slug, "username": login}) - if _, err := client.Teams.RemoveTeamMembershipBySlug(ctx, orgName, slug, member.login); err != nil { - return fmt.Errorf("could not remove existing team member %q: %w", member.login, err) + if _, err := client.Teams.RemoveTeamMembershipBySlug(ctx, orgName, slug, login); err != nil { + return fmt.Errorf("could not remove existing team member %q: %w", login, err) } } } diff --git a/github/resource_github_team_members_test.go b/github/resource_github_team_members_test.go index 3911bce9fa..4c1a6acdec 100644 --- a/github/resource_github_team_members_test.go +++ b/github/resource_github_team_members_test.go @@ -2,6 +2,7 @@ package github import ( "fmt" + "regexp" "strconv" "testing" @@ -19,7 +20,7 @@ func TestAccGithubTeamMembers(t *testing.T) { skipUnlessHasOrgs(t) skipUnlessHasOrgUser1(t) - t.Run("team_by_slug", func(t *testing.T) { + t.Run("imports_team_by_slug", func(t *testing.T) { t.Parallel() team := mustCreateTestTeam(t) @@ -57,7 +58,42 @@ resource "github_team_members" "test" { }) }) - t.Run("team_by_id_as_slug", func(t *testing.T) { + t.Run("updates_team_member_role", func(t *testing.T) { + t.Parallel() + + team := mustCreateTestTeam(t, nil) + + config := fmt.Sprintf(` +resource "github_team_members" "test" { + team_slug = "%s" + + members { + username = "%s" + role = "%%s" + } +} +`, team.GetSlug(), testAccConf.testOrgUser1) + + resource.Test(t, resource.TestCase{ + ProviderFactories: providerFactories, + Steps: []resource.TestStep{ + { + Config: fmt.Sprintf(config, "maintainer"), + }, + { + Config: fmt.Sprintf(config, "member"), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("github_team_members.test", plancheck.ResourceActionUpdate), + plancheck.ExpectKnownValue("github_team_members.test", tfjsonpath.New("members").AtSliceIndex(0).AtMapKey("role"), knownvalue.StringExact("member")), + }, + }, + }, + }, + }) + }) + + t.Run("imports_team_by_id_as_slug", func(t *testing.T) { t.Parallel() team := mustCreateTestTeam(t) @@ -96,7 +132,7 @@ resource "github_team_members" "test" { }) }) - t.Run("team_by_id", func(t *testing.T) { + t.Run("imports_team_by_id", func(t *testing.T) { t.Parallel() team := mustCreateTestTeam(t) @@ -266,12 +302,94 @@ resource "github_team_members" "test" { Config: config, ConfigStateChecks: []statecheck.StateCheck{ statecheck.ExpectKnownValue("github_team_members.test", tfjsonpath.New("members"), knownvalue.SetSizeExact(1)), + statecheck.ExpectKnownValue("github_team_members.test", tfjsonpath.New("members"), SetAbsent([]knownvalue.Check{ + knownvalue.MapPartial(map[string]knownvalue.Check{ + "username": knownvalue.StringExact(testAccConf.testOrgUser2), + }), + })), + statecheck.ExpectKnownValue("github_team_members.test", tfjsonpath.New("members"), knownvalue.SetExact([]knownvalue.Check{ + knownvalue.MapPartial(map[string]knownvalue.Check{ + "username": knownvalue.StringExact(testAccConf.testOrgUser1), + }), + })), }, }, { PreConfig: func() { mustAddTeamMember(t, team, testAccConf.testOrgUser2) }, Config: config, ConfigStateChecks: []statecheck.StateCheck{ statecheck.ExpectKnownValue("github_team_members.test", tfjsonpath.New("members"), knownvalue.SetSizeExact(1)), + statecheck.ExpectKnownValue("github_team_members.test", tfjsonpath.New("members"), SetAbsent([]knownvalue.Check{ + knownvalue.MapPartial(map[string]knownvalue.Check{ + "username": knownvalue.StringExact(testAccConf.testOrgUser2), + }), + })), + statecheck.ExpectKnownValue("github_team_members.test", tfjsonpath.New("members"), knownvalue.SetExact([]knownvalue.Check{ + knownvalue.MapPartial(map[string]knownvalue.Check{ + "username": knownvalue.StringExact(testAccConf.testOrgUser1), + }), + })), + }, + }, + }, + }) + }) + + t.Run("updates_team_members_changes", func(t *testing.T) { + t.Parallel() + + skipUnlessHasOrgUser2(t) + skipUnlessHasOrgUser3(t) + + team := mustCreateTestTeam(t, nil) + flippedCaseUsername2 := flipUsernameCase(testAccConf.testOrgUser2) + flippedCaseUsername3 := flipUsernameCase(testAccConf.testOrgUser3) + + memberConfig := ` + members { + username = "%s" + role = "%s" + } +` + + baseConfig := ` +resource "github_team_members" "test" { + team_slug = "%s" + + %s + %s + %s +}` + initialConfig := fmt.Sprintf(baseConfig, team.GetSlug(), fmt.Sprintf(memberConfig, testAccConf.testOrgUser1, "maintainer"), "", "") + + resource.Test(t, resource.TestCase{ + ProviderFactories: providerFactories, + Steps: []resource.TestStep{ + { + Config: initialConfig, + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue("github_team_members.test", tfjsonpath.New("members"), knownvalue.SetSizeExact(1)), + }, + }, + { + Config: fmt.Sprintf(baseConfig, team.GetSlug(), fmt.Sprintf(memberConfig, testAccConf.testOrgUser1, "maintainer"), fmt.Sprintf(memberConfig, flippedCaseUsername2, "member"), fmt.Sprintf(memberConfig, flippedCaseUsername3, "member")), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("github_team_members.test", plancheck.ResourceActionUpdate), + }, + }, + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue("github_team_members.test", tfjsonpath.New("members"), knownvalue.SetSizeExact(3)), + }, + }, + { + Config: fmt.Sprintf(baseConfig, team.GetSlug(), fmt.Sprintf(memberConfig, testAccConf.testOrgUser1, "maintainer"), fmt.Sprintf(memberConfig, flippedCaseUsername2, "member"), ""), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("github_team_members.test", plancheck.ResourceActionUpdate), + }, + }, + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue("github_team_members.test", tfjsonpath.New("members"), knownvalue.SetSizeExact(2)), }, }, }, @@ -430,4 +548,72 @@ resource "github_team_members" "test" { }, }) }) + + t.Run("is_case_insensitive", func(t *testing.T) { + t.Parallel() + + team := mustCreateTestTeam(t, nil) + flippedCaseUsername := flipUsernameCase(testAccConf.testOrgUser1) + + config := fmt.Sprintf(` +resource "github_team_members" "test" { + team_slug = "%s" + + members { + username = "%%s" + role = "maintainer" + } +} +`, team.GetSlug()) + + duplicateConfig := fmt.Sprintf(` +resource "github_team_members" "test" { + team_slug = "%s" + + members { + username = "%%s" + role = "maintainer" + } + + members { + username = "%%s" + role = "member" + } +} +`, team.GetSlug()) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessHasOrgs(t) }, + ProviderFactories: providerFactories, + Steps: []resource.TestStep{ + { + Config: fmt.Sprintf(duplicateConfig, testAccConf.testOrgUser1, flippedCaseUsername), + PlanOnly: true, + ExpectError: regexp.MustCompile("duplicate user '.*?' found in 'members' collection"), + }, + { + Config: fmt.Sprintf(config, flippedCaseUsername), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("github_team_members.test", plancheck.ResourceActionCreate), + }, + }, + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue("github_team_members.test", tfjsonpath.New("members"), knownvalue.SetSizeExact(1)), + }, + }, + { + Config: fmt.Sprintf(config, testAccConf.testOrgUser1), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("github_team_members.test", plancheck.ResourceActionNoop), + }, + }, + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue("github_team_members.test", tfjsonpath.New("members"), knownvalue.SetSizeExact(1)), + }, + }, + }, + }) + }) } diff --git a/github/util_diff.go b/github/util_diff.go index fedf54898a..8dd06281af 100644 --- a/github/util_diff.go +++ b/github/util_diff.go @@ -11,6 +11,7 @@ import ( "strings" "github.com/google/go-github/v89/github" + "github.com/hashicorp/go-cty/cty" "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) @@ -332,3 +333,33 @@ func suppressUnorderedListDiff(fieldKey string, f func(a, b any) int) schema.Sch return reflect.DeepEqual(oldList, newList) } } + +func diffDuplicateUsernameCheck(ctx context.Context, d *schema.ResourceDiff, fieldKey string) error { + tflog.Debug(ctx, "diffing nested username check", map[string]any{"field": fieldKey}) + if d.HasChange(fieldKey) && d.NewValueKnown(fieldKey) { + tflog.Trace(ctx, "field is changed and it's new value is known", map[string]any{"field": fieldKey, "new_value": d.Get(fieldKey)}) + v, diags := d.GetRawConfigAt(cty.GetAttrPath(fieldKey)) + if diags.HasError() { + return fmt.Errorf("error reading '%s' config: %v", fieldKey, diags) + } + + if !v.IsNull() && v.IsKnown() { + seen := make(map[string]struct{}) + it := v.ElementIterator() + for it.Next() { + _, elem := it.Element() + val := elem.GetAttr("username") + if val.IsNull() || !val.IsKnown() { + continue + } + + username := strings.ToLower(val.AsString()) + if _, ok := seen[username]; ok { + return fmt.Errorf("duplicate user '%s' found in '%s' collection", username, fieldKey) + } + seen[username] = struct{}{} + } + } + } + return nil +}