Skip to content

[BUG]: fix lowercase permadiff team members - #3539

Open
deiga wants to merge 15 commits into
mainfrom
fix-lowercase-permadiff-team-members
Open

[BUG]: fix lowercase permadiff team members#3539
deiga wants to merge 15 commits into
mainfrom
fix-lowercase-permadiff-team-members

Conversation

@deiga

@deiga deiga commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

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

@deiga
deiga requested a review from Copilot July 14, 2026 14:26
@github-actions

Copy link
Copy Markdown

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

@github-actions github-actions Bot added the Type: Bug Something isn't working as documented label Jul 14, 2026
@deiga
deiga requested a review from stevehipwell July 14, 2026 14:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread github/resource_github_team_members.go Outdated

@stevehipwell stevehipwell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

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

@deiga
deiga force-pushed the fix-lowercase-permadiff-team-members branch from b17fe7e to a2d5533 Compare July 14, 2026 18:18
@deiga
deiga requested a review from stevehipwell July 14, 2026 18:18
Comment thread github/resource_github_team_members.go Outdated
@deiga
deiga requested a review from stevehipwell July 15, 2026 19:50
@eyalgal eyalgal added this to the v6.13.1 milestone Jul 22, 2026

@stevehipwell stevehipwell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

deiga commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

@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

@deiga
deiga force-pushed the fix-lowercase-permadiff-team-members branch from a2d5533 to 81fc515 Compare July 24, 2026 06:15
@deiga
deiga requested review from Copilot and stevehipwell and removed request for Copilot July 24, 2026 06:18
Comment thread github/resource_github_team_members.go Outdated
@deiga
deiga force-pushed the fix-lowercase-permadiff-team-members branch from 81fc515 to 9a6ba79 Compare July 28, 2026 16:39
@deiga
deiga requested a review from stevehipwell July 28, 2026 16:39
@deiga
deiga force-pushed the fix-lowercase-permadiff-team-members branch from 9a6ba79 to 8957ffa Compare August 10, 2026 16:48
Comment thread github/resource_github_team_members.go Outdated
Comment thread github/resource_github_team_members.go Outdated
Comment thread github/resource_github_team_members.go Outdated
Comment thread github/util_diff.go Outdated
@deiga
deiga requested a review from stevehipwell August 22, 2026 11:11
@deiga
deiga force-pushed the fix-lowercase-permadiff-team-members branch from 8957ffa to 8ab4832 Compare August 22, 2026 11:12

@stevehipwell stevehipwell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@team-review-router
team-review-router Bot requested a review from a team September 7, 2026 13:50
deiga added 15 commits September 7, 2026 22:04
…anges

Signed-off-by: Timo Sand <timo.sand@f-secure.com>
…rcase

Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
…rcased username and role

Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
@deiga
deiga force-pushed the fix-lowercase-permadiff-team-members branch from 8ab4832 to 517eff0 Compare September 7, 2026 19:28
@stevehipwell stevehipwell modified the milestones: v6.13.1, v6.14.0 Sep 8, 2026
for _, member := range currentMembers {
if _, ok := want[member.login]; !ok {
tflog.Debug(ctx, "Removing team member.", map[string]any{"team_slug": slug, "username": member.login})
login := strings.ToLower(member.login)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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")
			}
		})
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-github-review Request a review from GitHub r/team_members Type: Bug Something isn't working as documented

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: v6.13.0 lower cases all team members

5 participants