Skip to content
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

Merged
merged 17 commits into from
Jun 26, 2019
7 changes: 4 additions & 3 deletions github/resource_github_membership.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ func resourceGithubMembership() *schema.Resource {

Schema: map[string]*schema.Schema{
"username": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
Type: schema.TypeString,
Required: true,
ForceNew: true,
DiffSuppressFunc: caseInsensitive(),
},
"role": {
Type: schema.TypeString,
Expand Down
37 changes: 37 additions & 0 deletions github/resource_github_membership_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"testing"
"unicode"

"github.com/google/go-github/v25/github"
"github.com/hashicorp/terraform/helper/resource"
Expand All @@ -29,6 +30,27 @@ func TestAccGithubMembership_basic(t *testing.T) {
})
}

func TestAccGithubMembership_caseInsensitive(t *testing.T) {
if len(testCollaborator) == 0 {
megan07 marked this conversation as resolved.
Show resolved Hide resolved
t.Skip("Skipping because length of `GITHUB_TEST_COLLABORATOR` is 0")
}
var membership github.Membership

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckGithubMembershipDestroy,
Steps: []resource.TestStep{
{
Config: testAccGithubMembershipConfig_caseInsensitive(),
Check: resource.ComposeTestCheckFunc(
testAccCheckGithubMembershipExists("github_membership.test_org_membership", &membership),
),
},
},
})
}

func TestAccGithubMembership_importBasic(t *testing.T) {
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Expand Down Expand Up @@ -140,3 +162,18 @@ var testAccGithubMembershipConfig string = fmt.Sprintf(`
role = "member"
}
`, testCollaborator)

func testAccGithubMembershipConfig_caseInsensitive() string {
otherCase := []rune(testCollaborator)

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

if unicode.IsUpper(otherCase[0]) {
otherCase[0] = unicode.ToLower(otherCase[0])
} else {
otherCase[0] = unicode.ToUpper(otherCase[0])
}
return fmt.Sprintf(`
resource "github_membership" "test_org_membership" {
username = "%s"
role = "member"
}
`, string(otherCase))
}
26 changes: 14 additions & 12 deletions github/resource_github_repository_collaborator.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"log"
"strings"

"github.com/google/go-github/v25/github"
"github.com/hashicorp/terraform/helper/schema"
Expand All @@ -21,9 +22,10 @@ func resourceGithubRepositoryCollaborator() *schema.Resource {
// editing repository collaborators are not supported by github api so forcing new on any changes
Schema: map[string]*schema.Schema{
"username": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
Type: schema.TypeString,
Required: true,
ForceNew: true,
DiffSuppressFunc: caseInsensitive(),
},
"repository": {
Type: schema.TypeString,
Expand Down Expand Up @@ -83,7 +85,7 @@ func resourceGithubRepositoryCollaboratorRead(d *schema.ResourceData, meta inter
ctx := context.WithValue(context.Background(), ctxId, d.Id())

// First, check if the user has been invited but has not yet accepted
invitation, err := findRepoInvitation(client, ctx, orgName, repoName, username)
invitation, username, err := findRepoInvitation(client, ctx, orgName, repoName, username)
megan07 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
} else if invitation != nil {
Expand Down Expand Up @@ -112,14 +114,14 @@ func resourceGithubRepositoryCollaboratorRead(d *schema.ResourceData, meta inter
}

for _, c := range collaborators {
if *c.Login == username {
if strings.ToLower(*c.Login) == strings.ToLower(username) {
permissionName, err := getRepoPermission(c.Permissions)
if err != nil {
return err
}

d.Set("repository", repoName)
d.Set("username", username)
d.Set("username", *c.Login)
megan07 marked this conversation as resolved.
Show resolved Hide resolved
d.Set("permission", permissionName)
return nil
}
Expand Down Expand Up @@ -149,7 +151,7 @@ func resourceGithubRepositoryCollaboratorDelete(d *schema.ResourceData, meta int
ctx := context.WithValue(context.Background(), ctxId, d.Id())

// Delete any pending invitations
invitation, err := findRepoInvitation(client, ctx, orgName, repoName, username)
invitation, username, err := findRepoInvitation(client, ctx, orgName, repoName, username)
megan07 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
} else if invitation != nil {
Expand All @@ -163,17 +165,17 @@ func resourceGithubRepositoryCollaboratorDelete(d *schema.ResourceData, meta int
return err
}

func findRepoInvitation(client *github.Client, ctx context.Context, owner, repo, collaborator string) (*github.RepositoryInvitation, error) {
func findRepoInvitation(client *github.Client, ctx context.Context, owner, repo, collaborator string) (*github.RepositoryInvitation, string, error) {
opt := &github.ListOptions{PerPage: maxPerPage}
for {
invitations, resp, err := client.Repositories.ListInvitations(ctx, owner, repo, opt)
if err != nil {
return nil, err
return nil, collaborator, err
}

for _, i := range invitations {
if *i.Invitee.Login == collaborator {
return i, nil
if strings.ToLower(*i.Invitee.Login) == strings.ToLower(collaborator) {
return i, *i.Invitee.Login, nil
}
}

Expand All @@ -182,5 +184,5 @@ func findRepoInvitation(client *github.Client, ctx context.Context, owner, repo,
}
opt.Page = resp.NextPage
}
return nil, nil
return nil, collaborator, nil
}
63 changes: 63 additions & 0 deletions github/resource_github_repository_collaborator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ package github
import (
"context"
"fmt"
"github.com/hashicorp/terraform/helper/schema"
"os"
"regexp"
"testing"
"unicode"

"github.com/hashicorp/terraform/helper/acctest"
"github.com/hashicorp/terraform/helper/resource"
Expand Down Expand Up @@ -35,6 +38,36 @@ func TestAccGithubRepositoryCollaborator_basic(t *testing.T) {
})
}

func TestAccGithubRepositoryCollaborator_caseInsensitive(t *testing.T) {
if len(testCollaborator) == 0 {
t.Skip("Skipping because length of `GITHUB_TEST_COLLABORATOR` is 0")
}

inviteeToken := os.Getenv("GITHUB_TEST_COLLABORATOR_TOKEN")
if inviteeToken == "" {
t.Skip("GITHUB_TEST_COLLABORATOR_TOKEN was not provided, skipping test")
}

resourceName := "github_repository_collaborator.test"
repoName := fmt.Sprintf("tf-acc-test-collab-%s", acctest.RandString(5))

var providers []*schema.Provider

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
ProviderFactories: testAccProviderFactories(&providers),
CheckDestroy: func(s *terraform.State) error { return nil },
Steps: []resource.TestStep{
{
Config: testAccGithubRepositoryCollaboratorConfig_caseInsensitive(inviteeToken, repoName),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "username", testCollaborator),
),
},
},
})
}

func TestAccGithubRepositoryCollaborator_importBasic(t *testing.T) {
repoName := fmt.Sprintf("tf-acc-test-collab-%s", acctest.RandString(5))

Expand Down Expand Up @@ -182,3 +215,33 @@ resource "github_repository" "test" {
}
`, repoName, testCollaborator, expectedPermission)
}

func testAccGithubRepositoryCollaboratorConfig_caseInsensitive(inviteeToken, repoName string) string {
otherCase := []rune(testCollaborator)
if unicode.IsUpper(otherCase[0]) {
otherCase[0] = unicode.ToLower(otherCase[0])
} else {
otherCase[0] = unicode.ToUpper(otherCase[0])
}
return fmt.Sprintf(`
provider "github" {
alias = "invitee"
token = "%s"
}

resource "github_repository" "test" {
name = "%s"
}

resource "github_repository_collaborator" "test" {
repository = "${github_repository.test.name}"
username = "%s"
permission = "push"
}

resource "github_user_invitation_accepter" "test" {
provider = "github.invitee"
invitation_id = "${github_repository_collaborator.test.invitation_id}"
}
`, inviteeToken, repoName, string(otherCase))
}
7 changes: 4 additions & 3 deletions github/resource_github_team_membership.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,10 @@ func resourceGithubTeamMembership() *schema.Resource {
ForceNew: true,
},
"username": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
Type: schema.TypeString,
Required: true,
ForceNew: true,
DiffSuppressFunc: caseInsensitive(),
},
"role": {
Type: schema.TypeString,
Expand Down
49 changes: 49 additions & 0 deletions github/resource_github_team_membership_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"strconv"
"testing"
"unicode"

"github.com/google/go-github/v25/github"
"github.com/hashicorp/terraform/helper/acctest"
Expand Down Expand Up @@ -39,6 +40,28 @@ func TestAccGithubTeamMembership_basic(t *testing.T) {
})
}

func TestAccGithubTeamMembership_caseInsensitive(t *testing.T) {
if len(testCollaborator) == 0 {
t.Skip("Skipping because length of `GITHUB_TEST_COLLABORATOR` is 0")
}
var membership github.Membership
randString := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckGithubTeamMembershipDestroy,
Steps: []resource.TestStep{
{
Config: testAccGithubTeamMembershipConfig_caseInsensitive(randString, testCollaborator, "member"),
Check: resource.ComposeTestCheckFunc(
testAccCheckGithubTeamMembershipExists("github_team_membership.test_team_membership", &membership),
),
},
},
})
}

func TestAccGithubTeamMembership_importBasic(t *testing.T) {
randString := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)

Expand Down Expand Up @@ -184,3 +207,29 @@ resource "github_team_membership" "test_team_membership" {
}
`, username, randString, username, role)
}

func testAccGithubTeamMembershipConfig_caseInsensitive(randString, username, role string) string {
otherCase := []rune(testCollaborator)
if unicode.IsUpper(otherCase[0]) {
otherCase[0] = unicode.ToLower(otherCase[0])
} else {
otherCase[0] = unicode.ToUpper(otherCase[0])
}
return fmt.Sprintf(`
resource "github_membership" "test_org_membership" {
username = "%s"
role = "member"
}

resource "github_team" "test_team" {
name = "tf-acc-test-team-membership-%s"
description = "Terraform acc test group"
}

resource "github_team_membership" "test_team_membership" {
team_id = "${github_team.test_team.id}"
username = "%s"
role = "%s"
}
`, string(otherCase), randString, string(otherCase), role)
}
6 changes: 6 additions & 0 deletions github/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ const (
maxPerPage = 100
)

func caseInsensitive() schema.SchemaDiffSuppressFunc {
return func(k, old, new string, d *schema.ResourceData) bool {
return strings.ToLower(old) == strings.ToLower(new)
}
}

func validateValueFunc(values []string) schema.SchemaValidateFunc {
return func(v interface{}, k string) (we []string, errors []error) {
value := v.(string)
Expand Down