improve github downloader on migrations (#7049)

* improve github downloader on migrations

* fix tests

* fix  uppercase function parameters

modules/migrations/base/downloader.go

@@ -11,9 +11,9 @@ type Downloader interface {
 	GetMilestones() ([]*Milestone, error)
 	GetReleases() ([]*Release, error)
 	GetLabels() ([]*Label, error)
-	GetIssues(start, limit int) ([]*Issue, error)
+	GetIssues(page, perPage int) ([]*Issue, bool, error)
 	GetComments(issueNumber int64) ([]*Comment, error)
-	GetPullRequests(start, limit int) ([]*PullRequest, error)
+	GetPullRequests(page, perPage int) ([]*PullRequest, error)
 }
 
 // DownloaderFactory defines an interface to match a downloader implementation and create a downloader

modules/migrations/git.go

@@ -53,9 +53,9 @@ func (g *PlainGitDownloader) GetReleases() ([]*base.Release, error) {
 	return nil, ErrNotSupported
 }
 
-// GetIssues returns issues according start and limit
-func (g *PlainGitDownloader) GetIssues(start, limit int) ([]*base.Issue, error) {
-	return nil, ErrNotSupported
+// GetIssues returns issues according page and perPage
+func (g *PlainGitDownloader) GetIssues(page, perPage int) ([]*base.Issue, bool, error) {
+	return nil, false, ErrNotSupported
 }
 
 // GetComments returns comments according issueNumber
@@ -63,7 +63,7 @@ func (g *PlainGitDownloader) GetComments(issueNumber int64) ([]*base.Comment, er
 	return nil, ErrNotSupported
 }
 
-// GetPullRequests returns pull requests according start and limit
+// GetPullRequests returns pull requests according page and perPage
 func (g *PlainGitDownloader) GetPullRequests(start, limit int) ([]*base.PullRequest, error) {
 	return nil, ErrNotSupported
 }

modules/migrations/github.go

@@ -272,71 +272,65 @@ func convertGithubReactions(reactions *github.Reactions) *base.Reactions {
 }
 
 // GetIssues returns issues according start and limit
-func (g *GithubDownloaderV3) GetIssues(start, limit int) ([]*base.Issue, error) {
-	var perPage = 100
+func (g *GithubDownloaderV3) GetIssues(page, perPage int) ([]*base.Issue, bool, error) {
 	opt := &github.IssueListByRepoOptions{
 		Sort:      "created",
 		Direction: "asc",
 		State:     "all",
 		ListOptions: github.ListOptions{
 			PerPage: perPage,
+			Page:    page,
 		},
 	}
-	var allIssues = make([]*base.Issue, 0, limit)
-	for {
-		issues, resp, err := g.client.Issues.ListByRepo(g.ctx, g.repoOwner, g.repoName, opt)
-		if err != nil {
-			return nil, fmt.Errorf("error while listing repos: %v", err)
-		}
-		for _, issue := range issues {
-			if issue.IsPullRequest() {
-				continue
-			}
-			var body string
-			if issue.Body != nil {
-				body = *issue.Body
-			}
-			var milestone string
-			if issue.Milestone != nil {
-				milestone = *issue.Milestone.Title
-			}
-			var labels = make([]*base.Label, 0, len(issue.Labels))
-			for _, l := range issue.Labels {
-				labels = append(labels, convertGithubLabel(&l))
-			}
-			var reactions *base.Reactions
-			if issue.Reactions != nil {
-				reactions = convertGithubReactions(issue.Reactions)
-			}
 
-			var email string
-			if issue.User.Email != nil {
-				email = *issue.User.Email
-			}
-			allIssues = append(allIssues, &base.Issue{
-				Title:       *issue.Title,
-				Number:      int64(*issue.Number),
-				PosterName:  *issue.User.Login,
-				PosterEmail: email,
-				Content:     body,
-				Milestone:   milestone,
-				State:       *issue.State,
-				Created:     *issue.CreatedAt,
-				Labels:      labels,
-				Reactions:   reactions,
-				Closed:      issue.ClosedAt,
-				IsLocked:    *issue.Locked,
-			})
-			if len(allIssues) >= limit {
-				return allIssues, nil
-			}
+	var allIssues = make([]*base.Issue, 0, perPage)
+
+	issues, _, err := g.client.Issues.ListByRepo(g.ctx, g.repoOwner, g.repoName, opt)
+	if err != nil {
+		return nil, false, fmt.Errorf("error while listing repos: %v", err)
+	}
+	for _, issue := range issues {
+		if issue.IsPullRequest() {
+			continue
 		}
-		if resp.NextPage == 0 {
-			break
+		var body string
+		if issue.Body != nil {
+			body = *issue.Body
 		}
-		opt.Page = resp.NextPage
+		var milestone string
+		if issue.Milestone != nil {
+			milestone = *issue.Milestone.Title
+		}
+		var labels = make([]*base.Label, 0, len(issue.Labels))
+		for _, l := range issue.Labels {
+			labels = append(labels, convertGithubLabel(&l))
+		}
+		var reactions *base.Reactions
+		if issue.Reactions != nil {
+			reactions = convertGithubReactions(issue.Reactions)
+		}
+
+		var email string
+		if issue.User.Email != nil {
+			email = *issue.User.Email
+		}
+		allIssues = append(allIssues, &base.Issue{
+			Title:       *issue.Title,
+			Number:      int64(*issue.Number),
+			PosterName:  *issue.User.Login,
+			PosterEmail: email,
+			Content:     body,
+			Milestone:   milestone,
+			State:       *issue.State,
+			Created:     *issue.CreatedAt,
+			Labels:      labels,
+			Reactions:   reactions,
+			Closed:      issue.ClosedAt,
+			IsLocked:    *issue.Locked,
+		})
 	}
-	return allIssues, nil
+
+	return allIssues, len(issues) < perPage, nil
 }
 
 // GetComments returns comments according issueNumber
@@ -379,97 +373,91 @@ func (g *GithubDownloaderV3) GetComments(issueNumber int64) ([]*base.Comment, er
 	return allComments, nil
 }
 
-// GetPullRequests returns pull requests according start and limit
-func (g *GithubDownloaderV3) GetPullRequests(start, limit int) ([]*base.PullRequest, error) {
+// GetPullRequests returns pull requests according page and perPage
+func (g *GithubDownloaderV3) GetPullRequests(page, perPage int) ([]*base.PullRequest, error) {
 	opt := &github.PullRequestListOptions{
 		Sort:      "created",
 		Direction: "asc",
 		State:     "all",
 		ListOptions: github.ListOptions{
-			PerPage: 100,
+			PerPage: perPage,
+			Page:    page,
 		},
 	}
-	var allPRs = make([]*base.PullRequest, 0, 100)
-	for {
-		prs, resp, err := g.client.PullRequests.List(g.ctx, g.repoOwner, g.repoName, opt)
-		if err != nil {
-			return nil, fmt.Errorf("error while listing repos: %v", err)
-		}
-		for _, pr := range prs {
-			var body string
-			if pr.Body != nil {
-				body = *pr.Body
-			}
-			var milestone string
-			if pr.Milestone != nil {
-				milestone = *pr.Milestone.Title
-			}
-			var labels = make([]*base.Label, 0, len(pr.Labels))
-			for _, l := range pr.Labels {
-				labels = append(labels, convertGithubLabel(l))
-			}
+	var allPRs = make([]*base.PullRequest, 0, perPage)
 
-			// FIXME: This API missing reactions, we may need another extra request to get reactions
+	prs, _, err := g.client.PullRequests.List(g.ctx, g.repoOwner, g.repoName, opt)
+	if err != nil {
+		return nil, fmt.Errorf("error while listing repos: %v", err)
+	}
+	for _, pr := range prs {
+		var body string
+		if pr.Body != nil {
+			body = *pr.Body
+		}
+		var milestone string
+		if pr.Milestone != nil {
+			milestone = *pr.Milestone.Title
+		}
+		var labels = make([]*base.Label, 0, len(pr.Labels))
+		for _, l := range pr.Labels {
+			labels = append(labels, convertGithubLabel(l))
+		}
 
-			var email string
-			if pr.User.Email != nil {
-				email = *pr.User.Email
-			}
-			var merged bool
-			// pr.Merged is not valid, so use MergedAt to test if it's merged
-			if pr.MergedAt != nil {
-				merged = true
-			}
+		// FIXME: This API missing reactions, we may need another extra request to get reactions
 
-			var headRepoName string
-			var cloneURL string
-			if pr.Head.Repo != nil {
-				headRepoName = *pr.Head.Repo.Name
-				cloneURL = *pr.Head.Repo.CloneURL
-			}
-			var mergeCommitSHA string
-			if pr.MergeCommitSHA != nil {
-				mergeCommitSHA = *pr.MergeCommitSHA
-			}
+		var email string
+		if pr.User.Email != nil {
+			email = *pr.User.Email
+		}
+		var merged bool
+		// pr.Merged is not valid, so use MergedAt to test if it's merged
+		if pr.MergedAt != nil {
+			merged = true
+		}
 
-			allPRs = append(allPRs, &base.PullRequest{
-				Title:          *pr.Title,
-				Number:         int64(*pr.Number),
-				PosterName:     *pr.User.Login,
-				PosterEmail:    email,
-				Content:        body,
-				Milestone:      milestone,
-				State:          *pr.State,
-				Created:        *pr.CreatedAt,
-				Closed:         pr.ClosedAt,
-				Labels:         labels,
-				Merged:         merged,
-				MergeCommitSHA: mergeCommitSHA,
-				MergedTime:     pr.MergedAt,
-				IsLocked:       pr.ActiveLockReason != nil,
-				Head: base.PullRequestBranch{
-					Ref:       *pr.Head.Ref,
-					SHA:       *pr.Head.SHA,
-					RepoName:  headRepoName,
-					OwnerName: *pr.Head.User.Login,
-					CloneURL:  cloneURL,
-				},
-				Base: base.PullRequestBranch{
-					Ref:       *pr.Base.Ref,
-					SHA:       *pr.Base.SHA,
-					RepoName:  *pr.Base.Repo.Name,
-					OwnerName: *pr.Base.User.Login,
-				},
-				PatchURL: *pr.PatchURL,
-			})
-			if len(allPRs) >= limit {
-				return allPRs, nil
-			}
+		var headRepoName string
+		var cloneURL string
+		if pr.Head.Repo != nil {
+			headRepoName = *pr.Head.Repo.Name
+			cloneURL = *pr.Head.Repo.CloneURL
 		}
-		if resp.NextPage == 0 {
-			break
+		var mergeCommitSHA string
+		if pr.MergeCommitSHA != nil {
+			mergeCommitSHA = *pr.MergeCommitSHA
 		}
-		opt.Page = resp.NextPage
+
+		allPRs = append(allPRs, &base.PullRequest{
+			Title:          *pr.Title,
+			Number:         int64(*pr.Number),
+			PosterName:     *pr.User.Login,
+			PosterEmail:    email,
+			Content:        body,
+			Milestone:      milestone,
+			State:          *pr.State,
+			Created:        *pr.CreatedAt,
+			Closed:         pr.ClosedAt,
+			Labels:         labels,
+			Merged:         merged,
+			MergeCommitSHA: mergeCommitSHA,
+			MergedTime:     pr.MergedAt,
+			IsLocked:       pr.ActiveLockReason != nil,
+			Head: base.PullRequestBranch{
+				Ref:       *pr.Head.Ref,
+				SHA:       *pr.Head.SHA,
+				RepoName:  headRepoName,
+				OwnerName: *pr.Head.User.Login,
+				CloneURL:  cloneURL,
+			},
+			Base: base.PullRequestBranch{
+				Ref:       *pr.Base.Ref,
+				SHA:       *pr.Base.SHA,
+				RepoName:  *pr.Base.Repo.Name,
+				OwnerName: *pr.Base.User.Login,
+			},
+			PatchURL: *pr.PatchURL,
+		})
 	}
+
 	return allPRs, nil
 }

modules/migrations/github_test.go

@@ -166,9 +166,11 @@ func TestGitHubDownloadRepo(t *testing.T) {
 	}, releases[len(releases)-1:])
 
 	// downloader.GetIssues()
-	issues, err := downloader.GetIssues(0, 3)
+	issues, isEnd, err := downloader.GetIssues(1, 8)
 	assert.NoError(t, err)
 	assert.EqualValues(t, 3, len(issues))
+	assert.False(t, isEnd)
+
 	var (
 		closed1 = time.Date(2018, 10, 23, 02, 57, 43, 0, time.UTC)
 	)
@@ -319,7 +321,7 @@ something like in the latest 15days could be enough don't you think ?
 	}, comments[:3])
 
 	// downloader.GetPullRequests()
-	prs, err := downloader.GetPullRequests(0, 3)
+	prs, err := downloader.GetPullRequests(1, 3)
 	assert.NoError(t, err)
 	assert.EqualValues(t, 3, len(prs))
 

modules/migrations/migrate.go

@@ -128,8 +128,8 @@ func migrateRepository(downloader base.Downloader, uploader base.Uploader, opts
 
 	if opts.Issues {
 		log.Trace("migrating issues and comments")
-		for {
-			issues, err := downloader.GetIssues(0, 100)
+		for i := 1; ; i++ {
+			issues, isEnd, err := downloader.GetIssues(i, 100)
 			if err != nil {
 				return err
 			}
@@ -160,7 +160,7 @@ func migrateRepository(downloader base.Downloader, uploader base.Uploader, opts
 				}
 			}
 
-			if len(issues) < 100 {
+			if isEnd {
 				break
 			}
 		}
@@ -168,8 +168,8 @@ func migrateRepository(downloader base.Downloader, uploader base.Uploader, opts
 
 	if opts.PullRequests {
 		log.Trace("migrating pull requests and comments")
-		for {
-			prs, err := downloader.GetPullRequests(0, 100)
+		for i := 1; ; i++ {
+			prs, err := downloader.GetPullRequests(i, 100)
 			if err != nil {
 				return err
 			}