Use Pager for GitHub API calls (#22)

* Use pager for api call

* Fix bug with teams

* Did right refactoring

* Did right refactoring

* Fix some tests

* Added right support of paginators

* Address comments in PR

* Fix etcd tests

* Made tests to check pagination api calls

* Added check for team members > 1

.github-authorized-keys-demo.default.yml

@@ -1,3 +1,4 @@
+
 github_api_token: {token api}
 github_organization: {organization}
 github_team: {team}

.travis.yml

@@ -11,7 +11,6 @@ env:
  - TEST_GITHUB_TEAM=${GITHUB_TEAM}
  - TEST_GITHUB_TEAM_ID=${GITHUB_TEAM_ID}
  - TEST_GITHUB_USER=${GITHUB_USER}
- - TEST_ETCD_ENDPOINT=http://${ETCD_IP}:2379
 services:
  - docker
 
@@ -24,6 +23,8 @@ script:
  - |-
  export ETCD_IP=$(ifconfig eth0 | grep 'inet addr' | cut -d: -f2 | awk '{print $1}')
 
+ - export TEST_ETCD_ENDPOINT=http://${ETCD_IP}:2379
+
  - docker-compose -f docker-compose-test.yaml up -d
 
  - |-

api/github.go

@@ -23,6 +23,8 @@ import (
 
  "github.com/google/go-github/github"
  "golang.org/x/oauth2"
+ log "github.com/Sirupsen/logrus"
+ "github.com/spf13/viper"
 )
 
 var (
@@ -31,8 +33,16 @@ var (
 
  // ErrorGitHubAccessDenied - returned when there was access denied to github.com resource
  ErrorGitHubAccessDenied = errors.New("Access denied")
+
+ // ErrorGitHubNotFound - returned when github.com resource not found
+ ErrorGitHubNotFound = errors.New("Not found")
+
 )
 
+func init() {
+ viper.SetDefault("github_api_max_page_size", 100)
+}
+
 // Naive oauth setup
 func newAccessToken(token string) oauth2.TokenSource {
  return oauth2.StaticTokenSource(
@@ -58,22 +68,34 @@ func (c *GithubClient) GetTeam(name string, id int) (team *github.Team, err erro
  team = nil
  err = nil
 
- teams, response, _ := c.client.Organizations.ListTeams(c.owner, nil)
+ var opt = &github.ListOptions{
+ PerPage: viper.GetInt("github_api_max_page_size"),
+ }
 
- if response.StatusCode != 200 {
- err = ErrorGitHubAccessDenied
-
- } else {
- for _, localTeam := range teams {
- if *localTeam.ID == id || *localTeam.Slug == name {
- team = localTeam
- // team found
- return
+ for {
+ teams, response, _ := c.client.Organizations.ListTeams(c.owner, opt)
+
+ if response.StatusCode != 200 {
+ err = ErrorGitHubAccessDenied
+ return
+ } else {
+ for _, localTeam := range teams {
+ if *localTeam.ID == id || *localTeam.Slug == name {
+ team = localTeam
+ // team found
+ return
+ }
  }
  }
- err = errors.New("Team with such name or id not found")
+
+ if response.LastPage == 0 {
+ break
+ }
+ opt.Page = response.NextPage
  }
  // Exit with error
+
+ err = errors.New("No such team name or id could be found")
  return
 }
 
@@ -94,20 +116,84 @@ func (c *GithubClient) IsTeamMember(user string, team *github.Team) (bool, error
 }
 
 // GetKeys - return array of user's {userName} public keys
-func (c *GithubClient) GetKeys(userName string) ([]*github.Key, *github.Response, error) {
- return c.client.Users.ListKeys(userName, nil)
+func (c *GithubClient) GetKeys(userName string) (keys []*github.Key, err error) {
+ defer func() {
+ if r := recover(); r != nil {
+ keys = make([]*github.Key, 0)
+ err = ErrorGitHubConnectionFailed
+ }
+ }()
+
+ logger := log.WithFields(log.Fields{"class": "GithubClient", "method": "Get"})
+
+ var opt = &github.ListOptions{
+ PerPage: viper.GetInt("github_api_max_page_size"),
+ }
+
+ for {
+ items, response, local_err := c.client.Users.ListKeys(userName, opt)
+
+ logger.Debugf("Response: %v", response)
+ logger.Debugf("Response.StatusCode: %v", response.StatusCode)
+
+ switch response.StatusCode {
+ case 200:
+ keys = append(keys, items...)
+ case 404:
+ err = ErrorGitHubNotFound
+ return
+ default:
+ err = ErrorGitHubAccessDenied
+ return
+ }
+
+ if local_err != nil {
+ err = local_err
+ return
+ }
+
+ if response.LastPage == 0 {
+ break
+ }
+ opt.Page = response.NextPage
+ }
+
+ return
 }
 
 // GetTeamMembers - return array of user's that are {team} members
 func (c *GithubClient) GetTeamMembers(team *github.Team) (users []*github.User, err error) {
  defer func() {
  if r := recover(); r != nil {
-  users = make([]*github.User, 0)
+ users = make([]*github.User, 0)
  err = ErrorGitHubConnectionFailed
  }
  }()
 
- users, _, err = c.client.Organizations.ListTeamMembers(*team.ID, nil)
+ var opt = &github.OrganizationListTeamMembersOptions{
+ ListOptions: github.ListOptions{
+ PerPage: viper.GetInt("github_api_max_page_size"),
+ },
+ }
+
+ for {
+ members, resp, local_err := c.client.Organizations.ListTeamMembers(*team.ID, opt)
+ if resp.StatusCode != 200 {
+ return nil, ErrorGitHubAccessDenied
+ }
+ if local_err != nil {
+ err = local_err
+ return
+ }
+
+ users = append(users, members...)
+
+ if resp.LastPage == 0 {
+ break
+ }
+ opt.Page = resp.NextPage
+ }
+
  return
 }
 

api/github_test.go

@@ -39,6 +39,8 @@ var _ = Describe("GithubClient", func() {
  validTeamName = viper.GetString("github_team")
  validTeamID = viper.GetInt("github_team_id")
  validUser = viper.GetString("github_user")
+ // Set max page size to 1 for test pagination code
+ viper.Set("github_api_max_page_size", 1)
  })
 
  Describe("getTeam()", func() {
@@ -74,7 +76,7 @@ var _ = Describe("GithubClient", func() {
  team, err := c.GetTeam("dasdasd", 0)
 
  Expect(err).NotTo(BeNil())
- Expect(err.Error()).To(Equal("Team with such name or id not found"))
+ Expect(err.Error()).To(Equal("No such team name or id could be found"))
 
  Expect(team).To(BeNil())
  })
@@ -163,7 +165,7 @@ var _ = Describe("GithubClient", func() {
  It("should return nil error and no empty list of keys", func() {
  c := NewGithubClient(validToken, validOrg)
  user, _ := c.getUser(validUser)
- keys, _, err := c.GetKeys(*user.Login)
+ keys, err := c.GetKeys(*user.Login)
 
  Expect(err).To(BeNil())
 
@@ -183,6 +185,7 @@ var _ = Describe("GithubClient", func() {
  Expect(err).To(BeNil())
 
  Expect(len(members) > 0).To(BeTrue())
+ Expect(len(members) > 1).To(BeTrue())
  })
  })
  })

api/linux_test.go

@@ -24,6 +24,7 @@ import (
  . "github.com/onsi/gomega"
  "os/user"
  "strconv"
+ model "github.com/cloudposse/github-authorized-keys/model/linux"
 )
 
 var _ = Describe("Linux", func() {
@@ -83,12 +84,12 @@ var _ = Describe("Linux", func() {
  Describe("userCreate()", func() {
  Context("call without GID", func() {
  var (
- userName LinuxUser
+ userName model.User
  linux Linux
  )
 
  BeforeEach(func() {
- userName = LinuxUser{Gid: "", Name: "test", Shell: "/bin/bash", Groups: []string{"wheel", "root"}}
+ userName = model.NewUser("test", "", []string{"wheel", "root"}, "/bin/bash")
  linux = NewLinux("/")
  })
 
@@ -101,35 +102,35 @@ var _ = Describe("Linux", func() {
 
  Expect(err).To(BeNil())
 
- osUser, _ := user.Lookup(userName.Name)
+ osUser, _ := user.Lookup(userName.Name())
 
- Expect(osUser.Username).To(Equal(userName.Name))
+ Expect(osUser.Username).To(Equal(userName.Name()))
 
  value, _ := strconv.ParseInt(osUser.Gid, 10, 64)
  Expect(value > 0).To(BeTrue())
 
  gids, _ := osUser.GroupIds()
 
- for _, group := range userName.Groups {
+ for _, group := range userName.Groups() {
  linuxGroup, err := user.LookupGroup(group)
  Expect(err).To(BeNil())
  Expect(gids).To(ContainElement(string(linuxGroup.Gid)))
  }
 
- shell := linux.userShell(userName.Name)
+ shell := linux.userShell(userName.Name())
 
- Expect(shell).To(Equal(userName.Shell))
+ Expect(shell).To(Equal(userName.Shell()))
  })
  })
 
  Context("call with GID", func() {
  var (
- userName LinuxUser
+ userName model.User
  linux Linux
  )
 
  BeforeEach(func() {
- userName = LinuxUser{Gid: "42", Name: "test", Shell: "/bin/bash", Groups: []string{"root"}}
+ userName = model.NewUser("test", "42", []string{"root"}, "/bin/bash")
  linux = NewLinux("/")
  })
 
@@ -142,23 +143,23 @@ var _ = Describe("Linux", func() {
 
  Expect(err).To(BeNil())
 
- osUser, _ := user.Lookup(userName.Name)
+ osUser, _ := user.Lookup(userName.Name())
 
- Expect(osUser.Username).To(Equal(userName.Name))
+ Expect(osUser.Username).To(Equal(userName.Name()))
 
- Expect(string(osUser.Gid)).To(Equal(userName.Gid))
+ Expect(string(osUser.Gid)).To(Equal(userName.Gid()))
 
  gids, _ := osUser.GroupIds()
 
- for _, group := range userName.Groups {
+ for _, group := range userName.Groups() {
  linuxGroup, err := user.LookupGroup(group)
  Expect(err).To(BeNil())
  Expect(gids).To(ContainElement(string(linuxGroup.Gid)))
  }
 
- shell := linux.userShell(userName.Name)
+ shell := linux.userShell(userName.Name())
 
- Expect(shell).To(Equal(userName.Shell))
+ Expect(shell).To(Equal(userName.Shell()))
  })
  })
  })

key_storages/github_keys.go

@@ -2,7 +2,6 @@ package keyStorages
 
 import (
  "errors"
- log "github.com/Sirupsen/logrus"
  "strings"
 
  "github.com/cloudposse/github-authorized-keys/api"
@@ -26,8 +25,6 @@ func (s *GithubKeys) Get(user string) (value string, err error) {
 
  value = ""
 
- logger := log.WithFields(log.Fields{"class": "GithubClient", "method": "Get"})
-
  // Load team
  team, err := s.client.GetTeam(s.team, s.teamID)
  if err != nil {
@@ -55,30 +52,23 @@ func (s *GithubKeys) Get(user string) (value string, err error) {
  return
  }
 
- keys, response, err := s.client.GetKeys(user)
+ keys, err := s.client.GetKeys(user)
 
- logger.Debugf("Response: %v", response)
- logger.Debugf("Response.StatusCode: %v", response.StatusCode)
+ if err == nil {
 
- switch response.StatusCode {
- case 200:
  result := []string{}
  for _, value := range keys {
  result = append(result, *value.Key)
  }
  value = strings.Join(result, "\n")
- return
 
- case 404:
- value = ""
+ } else if err == api.ErrorGitHubNotFound {
  err = ErrStorageKeyNotFound
- return
-
- default:
- value = ""
+ } else {
  err = errors.New("Access denied")
- return
  }
+
+ return
 }
 
 // NewGithubKeys - constructor for github key storage