Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
deiga wants to merge 18 commits into
base: main
Choose a base branch
from
Open

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

deiga wants to merge 18 commits into from

Conversation

@deiga
Copy link
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

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

go.mod
module github.com/integrations/terraform-provider-github/v6

go 1.24.0
go 1.24.4
Copy link
Collaborator

@stevehipwell stevehipwell Jan 27, 2026

Choose a reason for hiding this comment

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

Suggested change
go 1.24.4
go 1.24

@deiga
Copy link
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
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 added 15 commits January 29, 2026 22:47
@deiga
Move toInt and toInt64 functions to util.go
6af7b61 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
resource_github_emu_group_mapping) Change Read to use `ListExtern…
927ebf7 
…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…
5b8b43a 
…nnecessary code

Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
emu_group_mapping: Add group_name computed attribute
ab63985 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Add matching of team ID into Read
a30be2f
@deiga
Split Create and Update to separate functions
1258865
@deiga
Update Importer to use new ID pattern
09ef993
@deiga
Add Schema migration for new ID format
61103ee 
- 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
2a27e67 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Add skipped test while waiting for terraform-plugin-testing
46fe6ca 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Add CustomizeDiff function to determine if team_slug change needs…
6ba2803 
… `ForceNew`

Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Update docs
b87713b 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Use lookupTeamID instead of getTeamID
98c2b9d 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Move Create before Read
e4ea104 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Replace deep package with go-cmp
705b3b0 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga deiga force-pushed the fix-emu-group-mapping-read-issue branch from 8af4f04 to 705b3b0 Compare January 29, 2026 20:50
deiga added 2 commits January 29, 2026 23:54
@deiga
ListExternalGroupsForTeamBySlug does not return a nested Teams sl…
4a72832 
…ice. We need to directly lookup TeamID

Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Replace go-github-mock with githubApiMock
87ed6ed 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
Copy link
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
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
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
Remove unnecessry matchTeamID function
0d4e974 
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga deiga requested a review from stevehipwell January 30, 2026 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevehipwell stevehipwell Awaiting requested review from stevehipwell

Requested changes must be addressed to merge this pull request.

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.

2 participants

@deiga @stevehipwell