[BUG]: fix lowercase permadiff team members - #3539
Conversation
|
👋 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. |
There was a problem hiding this comment.
Pull request overview
These provider review instructions are being used.
Findings
- 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 (
Sethashing +StateFunc) to prevent case-only diffs; without a targeted test, this bug can regress silently (similar to how case handling already has coverage forgithub_team_membership). - Suggested fix: Add an acceptance test for
github_team_membersthat applies with one username case, then flips case and asserts a no-op plan (and/or verifies stored state uses the canonical lowercase form).
- File reference:
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.usernameto lowercase viaStateFunc. - Make
membersset identity case-insensitive by hashing the lowercase username in a customSetfunction.
stevehipwell
left a comment
There was a problem hiding this comment.
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.
|
@deiga FYI you should be able to add acceptance tests with alternative cases using the existing users. |
b17fe7e to
a2d5533
Compare
stevehipwell
left a comment
There was a problem hiding this comment.
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.
|
@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 |
a2d5533 to
81fc515
Compare
81fc515 to
9a6ba79
Compare
9a6ba79 to
8957ffa
Compare
8957ffa to
8ab4832
Compare
…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>
8ab4832 to
517eff0
Compare
| 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) |
There was a problem hiding this comment.
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")
}
})
}
}
Resolves #3533
Before the change?
After the change?
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!