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
43 changes: 39 additions & 4 deletions github/resource_github_membership_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,52 @@ package github

import (
"context"
"errors"
"fmt"
"testing"
"unicode"

"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")
Copy link
Contributor

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

}

var membership github.Membership
var otherMembership github.Membership

oc := []rune(testCollaborator)
if unicode.IsUpper(oc[0]) {
Copy link
Contributor

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?

oc[0] = unicode.ToLower(oc[0])
} else {
oc[0] = unicode.ToUpper(oc[0])
}

otherCase := string(oc)

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckGithubMembershipDestroy,
Steps: []resource.TestStep{
{
Config: testAccGithubMembershipConfig,
Config: testAccGithubMembershipConfig(testCollaborator),
Check: resource.ComposeTestCheckFunc(
testAccCheckGithubMembershipExists("github_membership.test_org_membership", &membership),
testAccCheckGithubMembershipRoleState("github_membership.test_org_membership", &membership),
),
},
{
Config: testAccGithubMembershipConfig(otherCase),
Check: resource.ComposeTestCheckFunc(
testAccCheckGithubMembershipExists("github_membership.test_org_membership", &otherMembership),
testAccGithubMembershipTheSame(&membership, &otherMembership),
),
},
},
})
}
Expand All @@ -36,7 +59,7 @@ func TestAccGithubMembership_importBasic(t *testing.T) {
CheckDestroy: testAccCheckGithubMembershipDestroy,
Steps: []resource.TestStep{
{
Config: testAccGithubMembershipConfig,
Config: testAccGithubMembershipConfig(testCollaborator),
},
{
ResourceName: "github_membership.test_org_membership",
Expand Down Expand Up @@ -134,9 +157,21 @@ func testAccCheckGithubMembershipRoleState(n string, membership *github.Membersh
}
}

var testAccGithubMembershipConfig string = fmt.Sprintf(`
func testAccGithubMembershipConfig(username string) string {
return fmt.Sprintf(`
resource "github_membership" "test_org_membership" {
username = "%s"
role = "member"
}
`, testCollaborator)
`, username)
}

func testAccGithubMembershipTheSame(orig, other *github.Membership) resource.TestCheckFunc {
return func(s *terraform.State) error {
if *orig.URL != *other.URL {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

return errors.New("users are different")
}

return nil
}
}
16 changes: 10 additions & 6 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 @@ -87,6 +89,7 @@ func resourceGithubRepositoryCollaboratorRead(d *schema.ResourceData, meta inter
if err != nil {
return err
} else if invitation != nil {
username = *invitation.Invitee.Login
permissionName, err := getInvitationPermission(invitation)
if err != nil {
return err
Expand All @@ -112,14 +115,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)
d.Set("permission", permissionName)
return nil
}
Expand Down Expand Up @@ -153,6 +156,7 @@ func resourceGithubRepositoryCollaboratorDelete(d *schema.ResourceData, meta int
if err != nil {
return err
} else if invitation != nil {
username = *invitation.Invitee.Login
_, err = client.Repositories.DeleteInvitation(ctx, orgName, repoName, *invitation.ID)
return err
}
Expand All @@ -172,7 +176,7 @@ func findRepoInvitation(client *github.Client, ctx context.Context, owner, repo,
}

for _, i := range invitations {
if *i.Invitee.Login == collaborator {
if strings.ToLower(*i.Invitee.Login) == strings.ToLower(collaborator) {
return i, nil
}
}
Expand Down
80 changes: 76 additions & 4 deletions github/resource_github_repository_collaborator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,14 @@ package github

import (
"context"
"errors"
"fmt"
"regexp"
"strings"
"testing"
"unicode"

"github.com/google/go-github/v25/github"
"github.com/hashicorp/terraform/helper/acctest"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/terraform"
Expand All @@ -14,23 +18,47 @@ import (
const expectedPermission string = "admin"

func TestAccGithubRepositoryCollaborator_basic(t *testing.T) {
if testCollaborator == "" {
t.Skip("Skipping because length of `GITHUB_TEST_COLLABORATOR` is not set")
}

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

var origInvitation github.RepositoryInvitation
var otherInvitation github.RepositoryInvitation

oc := []rune(testCollaborator)
if unicode.IsUpper(oc[0]) {
oc[0] = unicode.ToLower(oc[0])
} else {
oc[0] = unicode.ToUpper(oc[0])
}
otherCase := string(oc)

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckGithubRepositoryCollaboratorDestroy,
Steps: []resource.TestStep{
{
Config: testAccGithubRepositoryCollaboratorConfig(repoName),
Config: testAccGithubRepositoryCollaboratorConfig(repoName, testCollaborator),
Check: resource.ComposeTestCheckFunc(
testAccCheckGithubRepositoryCollaboratorExists(resourceName),
testAccCheckGithubRepositoryCollaboratorInvited(repoName, testCollaborator, &origInvitation),
testAccCheckGithubRepositoryCollaboratorPermission(resourceName),
resource.TestCheckResourceAttr(resourceName, "permission", expectedPermission),
resource.TestMatchResourceAttr(resourceName, "invitation_id", regexp.MustCompile(`^[0-9]+$`)),
),
},
{
Config: testAccGithubRepositoryCollaboratorConfig(repoName, otherCase),
Check: resource.ComposeTestCheckFunc(
testAccCheckGithubRepositoryCollaboratorInvited(repoName, otherCase, &otherInvitation),
resource.TestCheckResourceAttr(resourceName, "username", testCollaborator),
testAccGithubRepositoryCollaboratorTheSame(&origInvitation, &otherInvitation),
),
},
},
})
}
Expand All @@ -44,7 +72,7 @@ func TestAccGithubRepositoryCollaborator_importBasic(t *testing.T) {
CheckDestroy: testAccCheckGithubRepositoryCollaboratorDestroy,
Steps: []resource.TestStep{
{
Config: testAccGithubRepositoryCollaboratorConfig(repoName),
Config: testAccGithubRepositoryCollaboratorConfig(repoName, testCollaborator),
},
{
ResourceName: "github_repository_collaborator.test_repo_collaborator",
Expand Down Expand Up @@ -169,7 +197,7 @@ func testAccCheckGithubRepositoryCollaboratorPermission(n string) resource.TestC
}
}

func testAccGithubRepositoryCollaboratorConfig(repoName string) string {
func testAccGithubRepositoryCollaboratorConfig(repoName, username string) string {
return fmt.Sprintf(`
resource "github_repository" "test" {
name = "%s"
Expand All @@ -180,5 +208,49 @@ resource "github_repository_collaborator" "test_repo_collaborator" {
username = "%s"
permission = "%s"
}
`, repoName, testCollaborator, expectedPermission)
`, repoName, username, expectedPermission)
}

func testAccCheckGithubRepositoryCollaboratorInvited(repoName, username string, invitation *github.RepositoryInvitation) resource.TestCheckFunc {
return func(s *terraform.State) error {
opt := &github.ListOptions{PerPage: maxPerPage}

client := testAccProvider.Meta().(*Organization).client
org := testAccProvider.Meta().(*Organization).name

for {
invitations, resp, err := client.Repositories.ListInvitations(context.TODO(), org, repoName, opt)
if err != nil {
return errors.New(err.Error())
}

if len(invitations) > 1 {
return errors.New(fmt.Sprintf("multiple invitations have been sent for repository %s", repoName))
}

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

if resp.NextPage == 0 {
break
}
opt.Page = resp.NextPage
}

return errors.New(fmt.Sprintf("no invitation found for %s", username))
}
}

func testAccGithubRepositoryCollaboratorTheSame(orig, other *github.RepositoryInvitation) resource.TestCheckFunc {
return func(s *terraform.State) error {
if orig.ID != other.ID {
return errors.New("collaborators are different")
}

return nil
}
}
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
34 changes: 34 additions & 0 deletions github/resource_github_team_membership_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ package github

import (
"context"
"errors"
"fmt"
"strconv"
"testing"
"unicode"

"github.com/google/go-github/v25/github"
"github.com/hashicorp/terraform/helper/acctest"
Expand All @@ -13,9 +15,24 @@ import (
)

func TestAccGithubTeamMembership_basic(t *testing.T) {
if testCollaborator == "" {
t.Skip("Skipping because length of `GITHUB_TEST_COLLABORATOR` is not set")
}

var membership github.Membership
var otherMembership github.Membership

randString := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)

oc := []rune(testCollaborator)
if unicode.IsUpper(oc[0]) {
oc[0] = unicode.ToLower(oc[0])
} else {
oc[0] = unicode.ToUpper(oc[0])
}

otherCase := string(oc)

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Expand All @@ -35,6 +52,13 @@ func TestAccGithubTeamMembership_basic(t *testing.T) {
testAccCheckGithubTeamMembershipRoleState("github_team_membership.test_team_membership", "maintainer", &membership),
),
},
{
Config: testAccGithubTeamMembershipConfig(randString, otherCase, "maintainer"),
Check: resource.ComposeTestCheckFunc(
testAccCheckGithubTeamMembershipExists("github_team_membership.test_team_membership", &otherMembership),
testAccGithubTeamMembershipTheSame(&membership, &otherMembership),
),
},
},
})
}
Expand Down Expand Up @@ -184,3 +208,13 @@ resource "github_team_membership" "test_team_membership" {
}
`, username, randString, username, role)
}

func testAccGithubTeamMembershipTheSame(orig, other *github.Membership) resource.TestCheckFunc {
return func(s *terraform.State) error {
if *orig.URL != *other.URL {
return errors.New("users are different")
}

return nil
}
}
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