[BUG] Ensure `github_emu_group_mapping` behaves correctly if mapping changes upstream by deiga · Pull Request #3118 · integrations/terraform-provider-github

deiga · 2026-01-25T19:37:23Z

Before the change?

Read would not check if the Team and External Group we're still connected to each other
Changing group_id would not force a new resource, potentially leaving dangling connections
Resource ID format did not follow current conventions
Create and Update shared same functionality, leading to potentially wasted API calls

After the change?

Update ID format to <team_id>:<team_slug>:<group_id>
Adds Schema migration to ensure IDs are migrated to new format
Read properly checks that Team ID of team_slug and any Team mapped to group_id match
Changing group_id forces new resource creation
Split Create and Update to allow fine-grained control in Update
Adds check on diff for team_slug change to vlaidate if it's a team rename or switch

Pull request checklist

Schema migrations have been created if needed (example)
Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

Yes
No

github-actions · 2026-01-25T19:37:34Z

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

stevehipwell

I've added some review comments, but please could you remove the new deps. I don't think we should be introducing new dependencies with functional changes and I'm not sold on the value of doing something like this especially where we get a whole old copy of go-github in the vendor dir.

deiga · 2026-01-27T21:29:53Z

I've added some review comments, but please could you remove the new deps. I don't think we should be introducing new dependencies with functional changes and I'm not sold on the value of doing something like this especially where we get a whole old copy of go-github in the vendor dir.

Regarding migueleliasweb/go-github-mock:

I agree that having a second vendored go-github version isn't great. I'm hoping to resolve that as I opened a PR with the package to upgrade it.
I do think it makes little sense for us to maintain mocking code for the github API if it's readily available as a package for us to use, with direct struct support.

Regarding go-test/deep:
It was incredibly cumbersome to figure out why the migration tests we're failing because the structs in https://github.com/integrations/terraform-provider-github/pull/3118/changes#diff-6e6f4a2962b97dd1b6c8f7283eb71cb6cc2cae860c17d87041a32f0dfeb4cd0cR106 were visually the same, the integer types were just not matching. And I couldn't find a reasonable way to get that information out of reflect or fmt.Sprintf.
If you know of a better way, I'm all ears.

I would propose we keep both package additions as neither should be posing a risk for the functionality of the provider, while trying to make maintenance simpler

stevehipwell · 2026-01-28T16:25:09Z

RE migueleliasweb/go-github-mock could you use google/go-cmp instead? This is the idiomatic library for Go test comparisons.

RE migueleliasweb/go-github-mock if you want to use this please open an issue and or seperate PR to discuss.

deiga · 2026-01-29T21:57:50Z

RE migueleliasweb/go-github-mock could you use google/go-cmp instead? This is the idiomatic library for Go test comparisons.

RE migueleliasweb/go-github-mock if you want to use this please open an issue and or seperate PR to discuss.

@stevehipwell Thanks for the go-cmp pointer, implemented that now.

stevehipwell

Sorry, but I'm not following the point of matchTeamID()? Given that the team ID can't change without recreating the resource I'd only expect there to be a call to client.Teams.GetTeamBySlug() in create, to set it into the schema, and also in the diff if the slug changes. Other than that there shouldn't be any need to call that API during the general lifecycle.

FYI I like the addition of team_id to the schema, I'll add that into my team changes.

deiga · 2026-01-30T21:38:27Z

Sorry, but I'm not following the point of matchTeamID()? Given that the team ID can't change without recreating the resource I'd only expect there to be a call to client.Teams.GetTeamBySlug() in create, to set it into the schema, and also in the diff if the slug changes. Other than that there shouldn't be any need to call that API during the general lifecycle.

FYI I like the addition of team_id to the schema, I'll add that into my team changes.

@stevehipwell My idea with matchTeamId was to check if the Group and Team had been disconnected upstream.

But I just realize that in Create we just created the connection, so matchTeamID is silly.
And for Update we've just checked in Read that group IDs from state and upstream match.

You're right. it's unnecessary!

stevehipwell

This is coming along well, I've added some questions/suggestions but the main one is to remove the unnecessary API call to get the team ID in read.

stevehipwell

I should have clarified the table test comment, I'm expecting the table tests to be self contained. Otherwise there isn't much point as it's non-trivial to add new cases.

stevehipwell · 2026-02-03T20:15:09Z

@deiga this PR has conflicts.

deiga · 2026-02-04T10:42:04Z

@stevehipwell Conflicts resolved

stevehipwell

@deiga could you remove the migration test from this PR and add it to a new PR? I think that pattern needs more discussion and may benefit from being looked at from a simpler perspective, such as actions secret. If this is done I think we're ready to merge.

deiga · 2026-02-04T19:24:23Z

@stevehipwell Done

stevehipwell

LGTM

Signed-off-by: Timo Sand <[email protected]>

…alGroupsForTeamBySlug` to ensure we match state and reality Signed-off-by: Timo Sand <[email protected]>

…nnecessary code Signed-off-by: Timo Sand <[email protected]>

Signed-off-by: Timo Sand <[email protected]>

- 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]>

Signed-off-by: Timo Sand <[email protected]>

… `ForceNew` Signed-off-by: Timo Sand <[email protected]>

Signed-off-by: Timo Sand <[email protected]>

…ice. We need to directly lookup TeamID Signed-off-by: Timo Sand <[email protected]>

Signed-off-by: Timo Sand <[email protected]>

deiga · 2026-02-04T19:38:42Z

@stevehipwell Done

…changes upstream (integrations#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]>

deiga requested a review from stevehipwell January 25, 2026 19:37

github-actions Bot added the Type: Bug Something isn't working as documented label Jan 25, 2026

deiga added this to the v6.11.0 Release milestone Jan 25, 2026

stevehipwell requested changes Jan 27, 2026

View reviewed changes

Comment thread github/resource_github_emu_group_mapping.go Outdated

Comment thread github/resource_github_emu_group_mapping.go Outdated

Comment thread github/resource_github_emu_group_mapping.go Outdated

Comment thread go.mod

deiga requested a review from stevehipwell January 27, 2026 21:29

deiga force-pushed the fix-emu-group-mapping-read-issue branch from 8af4f04 to 705b3b0 Compare January 29, 2026 20:50

stevehipwell requested changes Jan 30, 2026

View reviewed changes

deiga requested a review from stevehipwell January 30, 2026 22:26

stevehipwell requested changes Feb 2, 2026

View reviewed changes

deiga requested a review from stevehipwell February 3, 2026 16:08

deiga force-pushed the fix-emu-group-mapping-read-issue branch from 0d4e974 to 0da08b1 Compare February 3, 2026 16:08

stevehipwell requested changes Feb 3, 2026

View reviewed changes

deiga force-pushed the fix-emu-group-mapping-read-issue branch from 0da08b1 to c12065a Compare February 3, 2026 16:43

deiga requested a review from stevehipwell February 3, 2026 16:45

stevehipwell requested changes Feb 3, 2026

View reviewed changes

deiga force-pushed the fix-emu-group-mapping-read-issue branch from c12065a to 2649046 Compare February 3, 2026 18:58

deiga requested a review from stevehipwell February 3, 2026 18:58

deiga force-pushed the fix-emu-group-mapping-read-issue branch from 2649046 to 7f04918 Compare February 4, 2026 09:59

stevehipwell requested changes Feb 4, 2026

View reviewed changes

deiga force-pushed the fix-emu-group-mapping-read-issue branch from fa53c43 to 4d3de7a Compare February 4, 2026 19:24

deiga requested a review from stevehipwell February 4, 2026 19:24

stevehipwell approved these changes Feb 4, 2026

View reviewed changes

deiga added 25 commits February 4, 2026 21:38

Move toInt and toInt64 functions to util.go

5cc6da0

Signed-off-by: Timo Sand <[email protected]>

resource_github_emu_group_mapping) Change Read to use `ListExtern…

4654ca5

…alGroupsForTeamBySlug` to ensure we match state and reality Signed-off-by: Timo Sand <[email protected]>

resource_github_emu_group_mapping: Refactor Create to have less u…

1787fb3

…nnecessary code Signed-off-by: Timo Sand <[email protected]>

emu_group_mapping: Add group_name computed attribute

a4a8eaf

Signed-off-by: Timo Sand <[email protected]>

Add matching of team ID into Read

8693502

Split Create and Update to separate functions

19dfa51

Update Importer to use new ID pattern

9973292

Add Schema migration for new ID format

7858f1a

- 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

cd9220b

Signed-off-by: Timo Sand <[email protected]>

Add skipped test while waiting for terraform-plugin-testing

1f80b9b

Signed-off-by: Timo Sand <[email protected]>

Add CustomizeDiff function to determine if team_slug change needs…

54cf8dc

… `ForceNew` Signed-off-by: Timo Sand <[email protected]>

Update docs

98276f5

Signed-off-by: Timo Sand <[email protected]>

Use lookupTeamID instead of getTeamID

eb28a9f

Signed-off-by: Timo Sand <[email protected]>

Move Create before Read

22d8455

Signed-off-by: Timo Sand <[email protected]>

Replace deep package with go-cmp

ff8de82

Signed-off-by: Timo Sand <[email protected]>

ListExternalGroupsForTeamBySlug does not return a nested Teams sl…

e632207

…ice. We need to directly lookup TeamID Signed-off-by: Timo Sand <[email protected]>

Replace go-github-mock with githubApiMock

7649872

Signed-off-by: Timo Sand <[email protected]>

Remove unnecessry matchTeamID function

e1fe75b

Signed-off-by: Timo Sand <[email protected]>

Address review comments

6195269

Signed-off-by: Timo Sand <[email protected]>

Rename state migartion functions

0e9d579

Signed-off-by: Timo Sand <[email protected]>

Inline unnecessary function

998aa15

Signed-off-by: Timo Sand <[email protected]>

Use new reusable diffing pattern

f8e0147

Signed-off-by: Timo Sand <[email protected]>

Address review comments

1b4f74e

Signed-off-by: Timo Sand <[email protected]>

Refactor mockResponse builder to accept inputs

343bc7d

Signed-off-by: Timo Sand <[email protected]>

Delete test

9c3367d

Signed-off-by: Timo Sand <[email protected]>

deiga force-pushed the fix-emu-group-mapping-read-issue branch from 4d3de7a to 9c3367d Compare February 4, 2026 19:38

nickfloyd approved these changes Feb 4, 2026

View reviewed changes

stevehipwell merged commit 4c95ca9 into integrations:main Feb 4, 2026
7 checks passed

Conversation

deiga commented Jan 25, 2026

Before the change?

After the change?

Pull request checklist

Does this introduce a breaking change?

Uh oh!

github-actions Bot commented Jan 25, 2026

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deiga commented Jan 27, 2026

Uh oh!

stevehipwell commented Jan 28, 2026

Uh oh!

deiga commented Jan 29, 2026

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

deiga commented Jan 30, 2026

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevehipwell commented Feb 3, 2026

Uh oh!

deiga commented Feb 4, 2026

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

deiga commented Feb 4, 2026

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

deiga commented Feb 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants