Skip to content

Commit 4c95ca9

Browse files
deigadeiga
authored
[BUG] Ensure github_emu_group_mapping behaves correctly if mapping changes upstream (#3118)
* Move `toInt` and `toInt64` functions to `util.go` Signed-off-by: Timo Sand <[email protected]> * `resource_github_emu_group_mapping`) Change `Read` to use `ListExternalGroupsForTeamBySlug` to ensure we match state and reality Signed-off-by: Timo Sand <[email protected]> * `resource_github_emu_group_mapping`: Refactor `Create` to have less unnecessary code Signed-off-by: Timo Sand <[email protected]> * `emu_group_mapping`: Add `group_name` computed attribute Signed-off-by: Timo Sand <[email protected]> * Add matching of team ID into Read * Split Create and Update to separate functions * Update Importer to use new ID pattern * Add Schema migration for new ID format - Adds `github.com/migueleliasweb/go-github-mock` package for mocking go-github endpoints - Adds `github.com/go-test/deep` package for better error messages when types of fields differ Signed-off-by: Timo Sand <[email protected]> * Changes `group_id` to `ForceNew` Signed-off-by: Timo Sand <[email protected]> * Add skipped test while waiting for `terraform-plugin-testing` Signed-off-by: Timo Sand <[email protected]> * Add `CustomizeDiff` function to determine if `team_slug` change needs `ForceNew` Signed-off-by: Timo Sand <[email protected]> * Update docs Signed-off-by: Timo Sand <[email protected]> * Use `lookupTeamID` instead of `getTeamID` Signed-off-by: Timo Sand <[email protected]> * Move `Create` before `Read` Signed-off-by: Timo Sand <[email protected]> * Replace `deep` package with `go-cmp` Signed-off-by: Timo Sand <[email protected]> * `ListExternalGroupsForTeamBySlug` does not return a nested `Teams` slice. We need to directly lookup TeamID Signed-off-by: Timo Sand <[email protected]> * Replace `go-github-mock` with `githubApiMock` Signed-off-by: Timo Sand <[email protected]> * Remove unnecessry `matchTeamID` function Signed-off-by: Timo Sand <[email protected]> * Address review comments Signed-off-by: Timo Sand <[email protected]> * Rename state migartion functions Signed-off-by: Timo Sand <[email protected]> * Inline unnecessary function Signed-off-by: Timo Sand <[email protected]> * Use new reusable diffing pattern Signed-off-by: Timo Sand <[email protected]> * Address review comments Signed-off-by: Timo Sand <[email protected]> * Refactor mockResponse builder to accept inputs Signed-off-by: Timo Sand <[email protected]> * Delete test Signed-off-by: Timo Sand <[email protected]> --------- Signed-off-by: Timo Sand <[email protected]>
1 parent 2182312 commit 4c95ca9

File tree

8 files changed

+393
-141
lines changed

8 files changed

+393
-141
lines changed

‎github/resource_github_emu_group_mapping.go‎

Lines changed: 185 additions & 107 deletions
Large diffs are not rendered by default.

‎github/resource_github_emu_group_mapping_migration.go‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
package github
2+
3+
import (
4+
"context"
5+
"net/http"
6+
"strconv"
7+
8+
"github.com/hashicorp/terraform-plugin-log/tflog"
9+
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
10+
)
11+
12+
func resourceGithubEMUGroupMappingV0() *schema.Resource {
13+
return &schema.Resource{
14+
Schema: map[string]*schema.Schema{
15+
"team_slug": {
16+
Type: schema.TypeString,
17+
Required: true,
18+
Description: "Slug of the GitHub team.",
19+
},
20+
"group_id": {
21+
Type: schema.TypeInt,
22+
Required: true,
23+
Description: "Integer corresponding to the external group ID to be linked.",
24+
},
25+
"etag": {
26+
Type: schema.TypeString,
27+
Computed: true,
28+
},
29+
},
30+
}
31+
}
32+
33+
func resourceGithubEMUGroupMappingStateUpgradeV0(ctx context.Context, rawState map[string]any, meta any) (map[string]any, error) {
34+
orgName := meta.(*Owner).name
35+
tflog.Trace(ctx, "GitHub EMU Group Mapping State before migration", map[string]any{"state": rawState, "owner": orgName})
36+
37+
client := meta.(*Owner).v3client
38+
39+
teamSlug := rawState["team_slug"].(string)
40+
// We need to bypass the etag because we need to get the latest group
41+
ctx = context.WithValue(ctx, ctxEtag, nil)
42+
groupsList, resp, err := client.Teams.ListExternalGroupsForTeamBySlug(ctx, orgName, teamSlug)
43+
if err != nil {
44+
if resp != nil && (resp.StatusCode == http.StatusNotFound) {
45+
// If the Group is not found, remove it from state
46+
tflog.Info(ctx, "Removing EMU group mapping from state because team no longer exists in GitHub", map[string]any{
47+
"resource_id": rawState["id"],
48+
})
49+
return nil, err
50+
}
51+
return nil, err
52+
}
53+
54+
group := groupsList.Groups[0]
55+
teamID, err := lookupTeamID(ctx, meta.(*Owner), teamSlug)
56+
if err != nil {
57+
return nil, err
58+
}
59+
rawState["team_id"] = teamID
60+
resourceID, err := buildID(strconv.FormatInt(teamID, 10), teamSlug, strconv.FormatInt(group.GetGroupID(), 10))
61+
if err != nil {
62+
return nil, err
63+
}
64+
rawState["id"] = resourceID
65+
66+
tflog.Trace(ctx, "GitHub EMU Group Mapping State after migration", map[string]any{"state": rawState})
67+
return rawState, nil
68+
}

‎github/resource_github_emu_group_mapping_test.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,51 @@ func TestAccGithubEMUGroupMapping(t *testing.T) {
126126
},
127127
})
128128
})
129+
130+
t.Run("forces new when switching to different team", func(t *testing.T) {
131+
t.Skip("Skipping this test because we don't have terraform-plugin-testing available yet.")
132+
randomID := acctest.RandString(5)
133+
teamName1 := fmt.Sprintf("%semu1-%s", testResourcePrefix, randomID)
134+
teamName2 := fmt.Sprintf("%semu2-%s", testResourcePrefix, randomID)
135+
136+
config := `
137+
resource "github_team" "test1" {
138+
name = "%s"
139+
description = "EMU group mapping test team 1"
140+
}
141+
resource "github_team" "test2" {
142+
name = "%s"
143+
description = "EMU group mapping test team 2"
144+
}
145+
resource "github_emu_group_mapping" "test" {
146+
team_slug = github_team.%s.slug
147+
group_id = %d
148+
}
149+
`
150+
151+
resource.Test(t, resource.TestCase{
152+
PreCheck: func() { skipUnlessEnterprise(t) },
153+
ProviderFactories: providerFactories,
154+
CheckDestroy: testAccCheckGithubEMUGroupMappingDestroy,
155+
Steps: []resource.TestStep{
156+
{
157+
Config: fmt.Sprintf(config, teamName1, teamName2, "test1", groupID),
158+
Check: resource.ComposeTestCheckFunc(
159+
resource.TestCheckResourceAttr("github_emu_group_mapping.test", "team_slug", teamName1),
160+
),
161+
},
162+
{
163+
Config: fmt.Sprintf(config, teamName1, teamName2, "test2", groupID),
164+
// ConfigPlanChecks: resource.ConfigPlanChecks{
165+
// PreApply: []plancheck.PlanCheck{
166+
// plancheckExpectKnownValues("github_emu_group_mapping.test", "team_slug", teamName2),
167+
// plancheck.ExpectResourceAction("github_emu_group_mapping.test", plancheck.ResourceActionDestroyBeforeCreate), // Verify that ForceNew is triggered
168+
// },
169+
// },
170+
},
171+
},
172+
})
173+
})
129174
}
130175

131176
func testAccCheckGithubEMUGroupMappingDestroy(s *terraform.State) error {

‎github/util.go‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,3 +342,43 @@ func deleteResourceOn404AndSwallow304OtherwiseReturnError(err error, d *schema.R
342342
}
343343
return err
344344
}
345+
346+
// Helper function to safely convert interface{} to int, handling both int and float64.
347+
func toInt(v any) int {
348+
switch val := v.(type) {
349+
case int:
350+
return val
351+
case float64:
352+
return int(val)
353+
case int64:
354+
return int(val)
355+
default:
356+
return 0
357+
}
358+
}
359+
360+
// Helper function to safely convert interface{} to int64, handling both int and float64.
361+
func toInt64(v any) int64 {
362+
switch val := v.(type) {
363+
case int:
364+
return int64(val)
365+
case int64:
366+
return val
367+
case float64:
368+
return int64(val)
369+
default:
370+
return 0
371+
}
372+
}
373+
374+
// lookupTeamID looks up the ID of a team by its slug.
375+
func lookupTeamID(ctx context.Context, meta *Owner, slug string) (int64, error) {
376+
client := meta.v3client
377+
owner := meta.name
378+
379+
team, _, err := client.Teams.GetTeamBySlug(ctx, owner, slug)
380+
if err != nil {
381+
return 0, err
382+
}
383+
return team.GetID(), nil
384+
}

‎github/util_diff.go‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"net/http"
99

1010
"github.com/google/go-github/v82/github"
11+
"github.com/hashicorp/terraform-plugin-log/tflog"
1112
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
1213
)
1314

@@ -109,3 +110,53 @@ func diffSecretVariableVisibility(ctx context.Context, d *schema.ResourceDiff, _
109110

110111
return nil
111112
}
113+
114+
// diffTeam compares the team_id and team_slug fields to determine if the team has changed.
115+
func diffTeam(ctx context.Context, diff *schema.ResourceDiff, m any) error {
116+
// Skip for new resources - no existing team_id to compare against
117+
if len(diff.Id()) == 0 {
118+
return nil
119+
}
120+
121+
if diff.HasChange("team_slug") {
122+
if isNewTeamID(ctx, diff, m) {
123+
return diff.ForceNew("team_slug")
124+
}
125+
}
126+
127+
return nil
128+
}
129+
130+
// helper function to determine if the team has changed or was renamed.
131+
func isNewTeamID(ctx context.Context, diff *schema.ResourceDiff, m any) bool {
132+
// Get old team_id from state
133+
oldTeamID := toInt64(diff.Get("team_id"))
134+
if oldTeamID == 0 {
135+
return false
136+
}
137+
meta := m.(*Owner)
138+
139+
// Resolve new team_slug to team ID via API
140+
oldTeamSlug, newTeamSlug := diff.GetChange("team_slug")
141+
newTeamID, err := lookupTeamID(ctx, meta, newTeamSlug.(string))
142+
if err != nil {
143+
// If team doesn't exist or API fails, skip ForceNew check and let Read handle it
144+
tflog.Debug(ctx, "Unable to resolve new team_slug to team ID, skipping ForceNew check", map[string]any{
145+
"new_team_slug": newTeamSlug,
146+
"error": err.Error(),
147+
})
148+
return false
149+
}
150+
151+
if newTeamID != oldTeamID {
152+
tflog.Debug(ctx, "Team ID changed, forcing new resource", map[string]any{
153+
"old_team_id": oldTeamID,
154+
"new_team_id": newTeamID,
155+
"new_team_slug": newTeamSlug,
156+
"old_team_slug": oldTeamSlug,
157+
})
158+
return true
159+
}
160+
161+
return false
162+
}

‎github/util_rules.go‎

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -10,34 +10,6 @@ import (
1010
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
1111
)
1212

13-
// Helper function to safely convert interface{} to int, handling both int and float64.
14-
func toInt(v any) int {
15-
switch val := v.(type) {
16-
case int:
17-
return val
18-
case float64:
19-
return int(val)
20-
case int64:
21-
return int(val)
22-
default:
23-
return 0
24-
}
25-
}
26-
27-
// Helper function to safely convert interface{} to int64, handling both int and float64.
28-
func toInt64(v any) int64 {
29-
switch val := v.(type) {
30-
case int:
31-
return int64(val)
32-
case int64:
33-
return val
34-
case float64:
35-
return int64(val)
36-
default:
37-
return 0
38-
}
39-
}
40-
4113
func toPullRequestMergeMethods(input any) []github.PullRequestMergeMethod {
4214
value, ok := input.([]any)
4315
if !ok || len(value) == 0 {

‎go.mod‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
module github.com/integrations/terraform-provider-github/v6
22

3-
go 1.24.0
3+
go 1.24.4
44

55
require (
66
github.com/go-jose/go-jose/v3 v3.0.4
7+
github.com/google/go-cmp v0.7.0
78
github.com/google/go-github/v82 v82.0.0
89
github.com/google/uuid v1.6.0
910
github.com/hashicorp/go-cty v1.5.0
@@ -21,7 +22,6 @@ require (
2122
github.com/cloudflare/circl v1.6.1 // indirect
2223
github.com/fatih/color v1.18.0 // indirect
2324
github.com/golang/protobuf v1.5.4 // indirect
24-
github.com/google/go-cmp v0.7.0 // indirect
2525
github.com/google/go-querystring v1.2.0 // indirect
2626
github.com/hashicorp/errwrap v1.0.0 // indirect
2727
github.com/hashicorp/go-checkpoint v0.5.0 // indirect

‎website/docs/r/emu_group_mapping.html.markdown‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@ description: |-
99

1010
This resource manages mappings between external groups for enterprise managed users and GitHub teams. It wraps the [Teams#ExternalGroups API](https://docs.github.com/en/rest/reference/teams#external-groups). Note that this is a distinct resource from `github_team_sync_group_mapping`. `github_emu_group_mapping` is special to the Enterprise Managed User (EMU) external group feature, whereas `github_team_sync_group_mapping` is specific to Identity Provider Groups.
1111

12-
!> **Warning:**: This resources `Read` function has a fundamental bug. It doesn't verify that the group is actually linked to the team. Someone could modify the linked group outside of Terraform and the resource would not detect it.
13-
1412
## Example Usage
1513

1614
```hcl
@@ -29,8 +27,8 @@ The following arguments are supported:
2927

3028
## Import
3129

32-
GitHub EMU External Group Mappings can be imported using the external `group_id` and `team_slug` separated by a colon, e.g.
30+
GitHub EMU External Group Mappings can be imported using the `team_slug` and external `group_id` separated by a colon, e.g.
3331

3432
```sh
35-
$ terraform import github_emu_group_mapping.example_emu_group_mapping 28836:emu-test-team
33+
$ terraform import github_emu_group_mapping.example_emu_group_mapping emu-test-team:28836
3634
```

0 commit comments

Comments
 (0)