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

LGTM

[robert-crandall]

I’m still seeing the perpetual diff for mixed-case usernames; can you add a test to verify this is case insensitive?

Here's an example test that's failing in this PR:

package github

import (
	"testing"

	"github.com/hashicorp/go-cty/cty"
	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
)

func TestGithubTeamMembersCaseOnlyDiff(t *testing.T) {
	for _, username := range []string{"mixedcaseuser", "MixedCaseUser"} {
		t.Run(username, func(t *testing.T) {
			r := resourceGithubTeamMembers()

			// Match the lowercase state produced by the read function.
			d := schema.TestResourceDataRaw(t, r.Schema, map[string]any{
				"team_slug": "example",
				"team_id":   "123",
				"members": []any{
					map[string]any{
						"username": "mixedcaseuser",
						"role":     "member",
					},
				},
			})
			d.SetId("123")

			config := cty.ObjectVal(map[string]cty.Value{
				"team_slug": cty.StringVal("example"),
				"team_id":   cty.NullVal(cty.String),
				"members": cty.SetVal([]cty.Value{
					cty.ObjectVal(map[string]cty.Value{
						"username": cty.StringVal(username),
						"role":     cty.StringVal("member"),
					}),
				}),
			})

			// CustomizeDiff reads RawConfig from the instance state.
			state := d.State()
			state.RawConfig = config

			diff, err := r.Diff(
				t.Context(),
				state,
				terraform.NewResourceConfigShimmed(config, r.CoreConfigSchema()),
				&Owner{},
			)
			if err != nil {
				t.Fatal(err)
			}

			if diff != nil && !diff.Empty() {
				for key, attr := range diff.Attributes {
					t.Logf("%s: %q -> %q", key, attr.Old, attr.New)
				}
				t.Fatal("username casing alone must not produce a diff")
			}
		})
	}
}

[deiga]

@robert-crandall thanks for the repro. The is_case_insensitive acceptance test already covers this: without the Set func its post-apply refresh plan shows exactly that remove/add, and with it restored the test passes.

@stevehipwell this means bringing back the custom Set func on members. It hashes the lowercased username plus the role. Here's why I don't think it can be dropped:

• SDKv2 matches set elements between state and config by hash before comparing any fields. DiffSuppressFunc only runs inside elements whose hashes already match.
• With the default hash, mixedcaseuser and MixedCaseUser never match. The plan becomes remove one, add the other, so caseInsensitive() only ever sees "" vs "MixedCaseUser". That's the failure Robert's test shows, and the same one in my earlier is_case_insensitive output.
• CustomizeDiff can't fix this on its own. diffDuplicateUsernameCheck can only return an error, and rewriting the plan with SetNew/Clear only works on computed attributes. members is Required, so the call fails with only operates on computed keys. Making members computed would break removing members, which this resource is supposed to manage authoritatively.

You were right that the hash doesn't prevent duplicates: on its own it would quietly merge User and user. That job stays with diffDuplicateUsernameCheck, which reads the raw config before anything is merged. It also catches the same user listed twice with different roles. The code comment now says this, instead of the earlier claim that the hash blocks duplicates.

The other option I looked at was keeping the case from state in Read. It fixes the perpetual diff after create, but an import still causes a diff, and a case-only config change plans as an update rather than a no-op. That fails step 3 of the test steps you suggested.

[Copilot]

🟢 Approval recommended

The implementation addresses the permanent diff while preserving role changes and adding adequate regression coverage.

0 open findings

1 resolved since last review

• The new case-normalization behavior for members (custom Set hash + username StateFunc…

🧠 Review effort: Balanced