-
Notifications
You must be signed in to change notification settings - Fork 765
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.
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
Bug Fix: GitHub Usernames are Case Insensitive #241
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@megan07 this looks good so far. I took a quick pass and left a couple of comments. I'll circle back later today.
@@ -140,3 +162,18 @@ var testAccGithubMembershipConfig string = fmt.Sprintf(` | |||
role = "member" | |||
} | |||
`, testCollaborator) | |||
|
|||
func testAccGithubMembershipConfig_caseInsensitive() string { | |||
otherCase := []rune(testCollaborator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I get the intent right, you are modifying the collaborator username, by toggling the case of the first rune, to test case insensitivity functionality. Is that correct? If so what about changing the function signature to take in an input, and just pass the collaborator name with the expected case.
func testAccGithubMembershipConfig_caseInsensitive(memberName string) {
return fmt.Sprintf(`
resource "github_membership" "test_org_membership" {
username = "%s"
role = "member"
}
`, memberName)
}
This way in the acceptance test you can run two TestSteps:
Steps: []resource.TestStep{
{
Config: testAccGithubMembershipConfig_caseInsensitive(strings.ToUpper(testCollaborator)),
Check: resource.ComposeTestCheckFunc(
testAccCheckGithubMembershipExists("github_membership.test_org_membership", &membership),
),
},
{
Config: testAccGithubMembershipConfig_caseInsensitive(strings.ToLower(testCollaborator)),
Check: resource.ComposeTestCheckFunc(
testAccCheckGithubMembershipExists("github_membership.test_org_membership", &membership),
),
},
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@megan07 I left a few comments for you, but this is otherwise good.
--- PASS: TestAccGithubMembership_basic (3.30s)
--- PASS: TestAccGithubMembership_caseInsensitive (2.84s)
--- PASS: TestAccGithubMembership_importBasic (2.79s)
--- PASS: TestAccGithubRepositoryCollaborator_basic (9.65s)
--- PASS: TestAccGithubRepositoryCollaborator_importBasic (9.17s)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments, but otherwise LGTM
|
||
"github.com/google/go-github/v25/github" | ||
"github.com/hashicorp/terraform/helper/resource" | ||
"github.com/hashicorp/terraform/terraform" | ||
) | ||
|
||
func TestAccGithubMembership_basic(t *testing.T) { | ||
if testCollaborator == "" { | ||
t.Skip("Skipping because length of `GITHUB_TEST_COLLABORATOR` is not set") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think this is a copy/paste typo for the message
|
||
func testAccGithubMembershipTheSame(orig, other *github.Membership) resource.TestCheckFunc { | ||
return func(s *terraform.State) error { | ||
if *orig.URL != *other.URL { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this also test to make sure at least one isn't null/empty? or should two empties be equal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, for the intent of the test, two empties would be equal. I can't think of a time when this would be empty here, but if they were to both be empty, I think it means the case change didn't have an effect.
var otherMembership github.Membership | ||
|
||
oc := []rune(testCollaborator) | ||
if unicode.IsUpper(oc[0]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if its possible, but could 0
ever be a number or symbol in a username?
I'm still seeing this:
|
Addressing #196 where GitHub usernames were being treated as case-sensitive in the diff.
Before fix:
After fix:
I also tested that if the user DOES update the casing in the GitHub UI, a terraform refresh will pick that up.