[BUG]: fix lowercase permadiff team members

[deiga]

Resolves #3533

---

Before the change?

• username lower casing is causing permadiff

After the change?

• fixes permadiff by storing username as lowercase in state

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]

👋 Hi, and thank you for this contribution!

This repo is maintained by GitHub and community members on a best-effort basis. We'll get to this as soon as we can.

You can help us prioritize by joining the discussion on open issues and PRs, sharing details on the changes you need, and reviewing other contributions.

---

_{🤖 This is an automated message.}

[Copilot]

Pull request overview

These provider review instructions are being used.

Findings

1. MEDIUM — Missing regression test for the permadiff fix
   • File reference: github/resource_github_team_members.go:66-79
   • Why this is a problem: The fix relies on new schema normalization behavior (Set hashing + StateFunc) to prevent case-only diffs; without a targeted test, this bug can regress silently (similar to how case handling already has coverage for github_team_membership).
   • Suggested fix: Add an acceptance test for github_team_members that applies with one username case, then flips case and asserts a no-op plan (and/or verifies stored state uses the canonical lowercase form).

This PR addresses the github_team_members perpetual diff caused by username case normalization, by canonicalizing usernames to lowercase for state and set element identity.

Changes:

• Canonicalize members.username to lowercase via StateFunc.
• Make members set identity case-insensitive by hashing the lowercase username in a custom Set function.

[stevehipwell]

I think changing Set would technically be a breaking change, but I think we ought to do it anyway as any breaking issues would be catching incorrect usage.

RE the username casing, I think that we should keep the diff suppression pattern as that way we don't lose any of the input data.

[stevehipwell]

@deiga FYI you should be able to add acceptance tests with alternative cases using the existing users.

[stevehipwell]

I don't think we need this?

[deiga]

Without this, importing a mixed-case username will create a diff.

[stevehipwell]

Yeah, because the code is missing a check to reconcile the config members with the actual members as lower case. So we don't need this, but we do need a fix.

I need to revert the GraphQL change as go-github now supports filtering the members so I can take a look at this using your regression test (I think I've handled this in the repository collaborators resource).

[deiga]

After going through multiple different iterations, it seems to me that StateFunc is the only proper solution here.
Yes, it's deprecated in Framework, but with suggestions to move the logic to Create.

I tried multiple different ways, but I couldn't get Create to store the username values in a way that would have allowed the case insensitive test to pass.

[stevehipwell]

I don't think we should use flippedCaseUsername anywhere other than the targeted test. This will fix the imports test as it checks the state directly and does not use Terraform logic for equality.

[deiga]

@stevehipwell that's definitely an option. I decided to add it to multiple tests as it highlighted problems with cases in multiple use-cases. But I can try to make the case sensitivity test cover all the same problems instead

[stevehipwell]

This comment isn't correct, it would just block the same user plus role combo with different case. Given that it doesn't act as a block for duplicates and you need to use a diff function to actually block I don't think this code is necessary. I ended up at the same decision after copying your code form this PR in a different one.

[deiga]

After removing the Set function the case insensitive test starts failing as the default Set function considers different cased usernames as separate entries. This is IMO the wrong behaviour.

=== CONT  TestAccGithubTeamMembers/is_case_insensitive
    resource_github_team_members_test.go:585: Step 2/3 error: After applying this test step, the refresh plan was not empty.
        stdout
        
        
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # github_team_members.test will be updated in-place
          ~ resource "github_team_members" "test" {
                id        = "19113114"
                # (2 unchanged attributes hidden)
        
              - members {
                  - role     = "maintainer" -> null
                  - username = "gh-terraform-testing-test1" -> null
                }
              + members {
                  + role     = "maintainer"
                  + username = "Gh-terraform-testing-test1"
                }
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.

[stevehipwell]

This should just call resourceGithubTeamMembersDiff and it can call the other functions.

[deiga]

That seems like an unnecessary change as it increases verbosity without improving readability IMO, but I'll do it