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.

Sign up for GitHub

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
Merged

Bug Fix: GitHub Usernames are Case Insensitive #241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
e4a5d4e
update usernames to case insensitive
Jun 14, 2019
a9ba1cd
flatten stuff
Jun 14, 2019
0b784bb
Undo branch protection work
Jun 14, 2019
08b732c
fix imports
Jun 14, 2019
a5b71f6
remove changes to org block
Jun 14, 2019
e4375c3
update tests
Jun 14, 2019
b56a29c
Fixing panic when testCollaborator isn't set
mbfrahry Jun 17, 2019
ff7d46d
Remove unneccessary test check
mbfrahry Jun 17, 2019
19b5f88
fix repository collaborator test
Jun 20, 2019
a68fc4c
updates after review comments
Jun 24, 2019
793befc
updates after review comments
Jun 24, 2019
b76fe4f
Merge branch 'master' into f-username-case
megan07 Jun 24, 2019
7e8fa40
indent
Jun 24, 2019
2b1b424
merge with master
Jun 24, 2019
37fed31
adding test collaborator check to test
Jun 24, 2019
34e594c
removing 'length of' from skip message
Jun 24, 2019
d995f54
update tests for cases for first char is not a letter
Jun 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions github/resource_github_membership.go
View file
Open in desktop
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
61 changes: 57 additions & 4 deletions github/resource_github_membership_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package github

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

Expand All @@ -11,6 +12,10 @@ import (
)

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

var membership github.Membership

resource.Test(t, resource.TestCase{
Expand All @@ -19,7 +24,7 @@ func TestAccGithubMembership_basic(t *testing.T) {
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),
Expand All @@ -29,14 +34,50 @@ func TestAccGithubMembership_basic(t *testing.T) {
})
}

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

var membership github.Membership
var otherMembership github.Membership

otherCase := flipUsernameCase(testCollaborator)

if testCollaborator == otherCase {
t.Skip("Skipping because `GITHUB_TEST_COLLABORATOR` has no letters to flip case")
}

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

func TestAccGithubMembership_importBasic(t *testing.T) {
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckGithubMembershipDestroy,
Steps: []resource.TestStep{
{
Config: testAccGithubMembershipConfig,
Config: testAccGithubMembershipConfig(testCollaborator),
},
{
ResourceName: "github_membership.test_org_membership",
Expand Down Expand Up @@ -134,9 +175,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
View file
Open in desktop
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
99 changes: 95 additions & 4 deletions github/resource_github_repository_collaborator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@ package github

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

"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,6 +17,10 @@ import (
const expectedPermission string = "admin"

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

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

Expand All @@ -23,7 +30,7 @@ func TestAccGithubRepositoryCollaborator_basic(t *testing.T) {
CheckDestroy: testAccCheckGithubRepositoryCollaboratorDestroy,
Steps: []resource.TestStep{
{
Config: testAccGithubRepositoryCollaboratorConfig(repoName),
Config: testAccGithubRepositoryCollaboratorConfig(repoName, testCollaborator),
Check: resource.ComposeTestCheckFunc(
testAccCheckGithubRepositoryCollaboratorExists(resourceName),
testAccCheckGithubRepositoryCollaboratorPermission(resourceName),
Expand All @@ -35,6 +42,46 @@ func TestAccGithubRepositoryCollaborator_basic(t *testing.T) {
})
}

func TestAccGithubRepositoryCollaborator_caseInsensitive(t *testing.T) {
if testCollaborator == "" {
t.Skip("Skipping because `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

otherCase := flipUsernameCase(testCollaborator)

if testCollaborator == otherCase {
t.Skip("Skipping because `GITHUB_TEST_COLLABORATOR` has no letters to flip case")
}

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckGithubRepositoryCollaboratorDestroy,
Steps: []resource.TestStep{
{
Config: testAccGithubRepositoryCollaboratorConfig(repoName, testCollaborator),
Check: resource.ComposeTestCheckFunc(
testAccCheckGithubRepositoryCollaboratorInvited(repoName, testCollaborator, &origInvitation),
),
},
{
Config: testAccGithubRepositoryCollaboratorConfig(repoName, otherCase),
Check: resource.ComposeTestCheckFunc(
testAccCheckGithubRepositoryCollaboratorInvited(repoName, otherCase, &otherInvitation),
resource.TestCheckResourceAttr(resourceName, "username", testCollaborator),
testAccGithubRepositoryCollaboratorTheSame(&origInvitation, &otherInvitation),
),
},
},
})
}

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

Expand All @@ -44,7 +91,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 +216,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 +227,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
View file
Open in desktop
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
Loading