Skip to content

[BUG] Ensure github_emu_group_mapping behaves correctly if mapping changes upstream#3118

Merged
stevehipwell merged 25 commits intointegrations:mainfrom
F-Secure-web:fix-emu-group-mapping-read-issue
Feb 4, 2026
Merged

[BUG] Ensure github_emu_group_mapping behaves correctly if mapping changes upstream#3118
stevehipwell merged 25 commits intointegrations:mainfrom
F-Secure-web:fix-emu-group-mapping-read-issue

Conversation

@deiga
Copy link
Copy Markdown
Collaborator

@deiga deiga commented Jan 25, 2026

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

@deiga deiga requested a review from stevehipwell January 25, 2026 19:37
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 25, 2026

👋 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! 🚀

@github-actions github-actions bot added the Type: Bug Something isn't working as documented label Jan 25, 2026
@deiga deiga added this to the v6.11.0 Release milestone Jan 25, 2026
stevehipwell
stevehipwell requested changes Jan 27, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Collaborator Author

deiga commented Jan 27, 2026

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

@deiga deiga requested a review from stevehipwell January 27, 2026 21:29
@stevehipwell
Copy link
Copy Markdown
Collaborator

stevehipwell commented Jan 28, 2026

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 deiga force-pushed the fix-emu-group-mapping-read-issue branch from 8af4f04 to 705b3b0 Compare January 29, 2026 20:50
@deiga
Copy link
Copy Markdown
Collaborator Author

deiga commented Jan 29, 2026

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
stevehipwell requested changes Jan 30, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Collaborator Author

deiga commented Jan 30, 2026

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!

@deiga deiga requested a review from stevehipwell January 30, 2026 22:26
stevehipwell
stevehipwell requested changes Feb 2, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@deiga deiga requested a review from stevehipwell February 3, 2026 16:08
@deiga deiga force-pushed the fix-emu-group-mapping-read-issue branch from 0d4e974 to 0da08b1 Compare February 3, 2026 16:08
stevehipwell
stevehipwell requested changes Feb 3, 2026
View reviewed changes
@deiga deiga force-pushed the fix-emu-group-mapping-read-issue branch from 0da08b1 to c12065a Compare February 3, 2026 16:43
@deiga deiga requested a review from stevehipwell February 3, 2026 16:45
stevehipwell
stevehipwell requested changes Feb 3, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@deiga deiga force-pushed the fix-emu-group-mapping-read-issue branch from c12065a to 2649046 Compare February 3, 2026 18:58
@deiga deiga requested a review from stevehipwell February 3, 2026 18:58
@stevehipwell
Copy link
Copy Markdown
Collaborator

stevehipwell commented Feb 3, 2026

@deiga this PR has conflicts.

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

deiga commented Feb 4, 2026

@stevehipwell Conflicts resolved

stevehipwell
stevehipwell requested changes Feb 4, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 deiga force-pushed the fix-emu-group-mapping-read-issue branch from fa53c43 to 4d3de7a Compare February 4, 2026 19:24
@deiga
Copy link
Copy Markdown
Collaborator Author

deiga commented Feb 4, 2026

@stevehipwell Done

@deiga deiga requested a review from stevehipwell February 4, 2026 19:24
stevehipwell
stevehipwell approved these changes Feb 4, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stevehipwell
Copy link
Copy Markdown
Collaborator

stevehipwell commented Feb 4, 2026

@deiga could you please rebase this PR?

deiga added 25 commits February 4, 2026 21:38
@deiga
Move toInt and toInt64 functions to util.go
5cc6da0 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
resource_github_emu_group_mapping) Change Read to use `ListExtern…
4654ca5 
…alGroupsForTeamBySlug` to ensure we match state and reality

Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
resource_github_emu_group_mapping: Refactor Create to have less u…
1787fb3 
…nnecessary code

Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
emu_group_mapping: Add group_name computed attribute
a4a8eaf 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Add matching of team ID into Read
8693502
@deiga
Split Create and Update to separate functions
19dfa51
@deiga
Update Importer to use new ID pattern
9973292
@deiga
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 <timo.sand@f-secure.com>
@deiga
Changes group_id to ForceNew
cd9220b 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Add skipped test while waiting for terraform-plugin-testing
1f80b9b 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Add CustomizeDiff function to determine if team_slug change needs…
54cf8dc 
… `ForceNew`

Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Update docs
98276f5 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Use lookupTeamID instead of getTeamID
eb28a9f 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Move Create before Read
22d8455 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Replace deep package with go-cmp
ff8de82 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
ListExternalGroupsForTeamBySlug does not return a nested Teams sl…
e632207 
…ice. We need to directly lookup TeamID

Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Replace go-github-mock with githubApiMock
7649872 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Remove unnecessry matchTeamID function
e1fe75b 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Address review comments
6195269 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Rename state migartion functions
0e9d579 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Inline unnecessary function
998aa15 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Use new reusable diffing pattern
f8e0147 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Address review comments
1b4f74e 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Refactor mockResponse builder to accept inputs
343bc7d 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Delete test
9c3367d 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga deiga force-pushed the fix-emu-group-mapping-read-issue branch from 4d3de7a to 9c3367d Compare February 4, 2026 19:38
@deiga
Copy link
Copy Markdown
Collaborator Author

deiga commented Feb 4, 2026

@stevehipwell Done

@stevehipwell stevehipwell merged commit 4c95ca9 into integrations:main Feb 4, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickfloyd nickfloyd nickfloyd approved these changes

@stevehipwell stevehipwell stevehipwell approved these changes

Assignees

No one assigned

Labels

Type: Bug Something isn't working as documented

Projects

None yet

Milestone

v6.11.0 Release

Development

Successfully merging this pull request may close these issues.

3 participants

@deiga @stevehipwell @nickfloyd