Skip to content

Commit 7d12ec2

Browse files
lunnytechknowlogick
lunny
authored and
techknowlogick
committed
improve github downloader on migrations (#7049)
* improve github downloader on migrations * fix tests * fix uppercase function parameters
1 parent 43cf2f3 commit 7d12ec2

File tree

5 files changed

+134
-144
lines changed

5 files changed

+134
-144
lines changed

‎modules/migrations/base/downloader.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,9 @@ type Downloader interface {
1111
GetMilestones() ([]*Milestone, error)
1212
GetReleases() ([]*Release, error)
1313
GetLabels() ([]*Label, error)
14-
GetIssues(start, limit int) ([]*Issue, error)
14+
GetIssues(page, perPage int) ([]*Issue, bool, error)
1515
GetComments(issueNumber int64) ([]*Comment, error)
16-
GetPullRequests(start, limit int) ([]*PullRequest, error)
16+
GetPullRequests(page, perPage int) ([]*PullRequest, error)
1717
}
1818

1919
// DownloaderFactory defines an interface to match a downloader implementation and create a downloader

‎modules/migrations/git.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,17 +53,17 @@ func (g *PlainGitDownloader) GetReleases() ([]*base.Release, error) {
5353
return nil, ErrNotSupported
5454
}
5555

56-
// GetIssues returns issues according start and limit
57-
func (g *PlainGitDownloader) GetIssues(start, limit int) ([]*base.Issue, error) {
58-
return nil, ErrNotSupported
56+
// GetIssues returns issues according page and perPage
57+
func (g *PlainGitDownloader) GetIssues(page, perPage int) ([]*base.Issue, bool, error) {
58+
return nil, false, ErrNotSupported
5959
}
6060

6161
// GetComments returns comments according issueNumber
6262
func (g *PlainGitDownloader) GetComments(issueNumber int64) ([]*base.Comment, error) {
6363
return nil, ErrNotSupported
6464
}
6565

66-
// GetPullRequests returns pull requests according start and limit
66+
// GetPullRequests returns pull requests according page and perPage
6767
func (g *PlainGitDownloader) GetPullRequests(start, limit int) ([]*base.PullRequest, error) {
6868
return nil, ErrNotSupported
6969
}

‎modules/migrations/github.go‎

Lines changed: 119 additions & 131 deletions
Original file line numberDiff line numberDiff line change
@@ -272,71 +272,65 @@ func convertGithubReactions(reactions *github.Reactions) *base.Reactions {
272272
}
273273

274274
// GetIssues returns issues according start and limit
275-
func (g *GithubDownloaderV3) GetIssues(start, limit int) ([]*base.Issue, error) {
276-
var perPage = 100
275+
func (g *GithubDownloaderV3) GetIssues(page, perPage int) ([]*base.Issue, bool, error) {
277276
opt := &github.IssueListByRepoOptions{
278277
Sort: "created",
279278
Direction: "asc",
280279
State: "all",
281280
ListOptions: github.ListOptions{
282281
PerPage: perPage,
282+
Page: page,
283283
},
284284
}
285-
var allIssues = make([]*base.Issue, 0, limit)
286-
for {
287-
issues, resp, err := g.client.Issues.ListByRepo(g.ctx, g.repoOwner, g.repoName, opt)
288-
if err != nil {
289-
return nil, fmt.Errorf("error while listing repos: %v", err)
290-
}
291-
for _, issue := range issues {
292-
if issue.IsPullRequest() {
293-
continue
294-
}
295-
var body string
296-
if issue.Body != nil {
297-
body = *issue.Body
298-
}
299-
var milestone string
300-
if issue.Milestone != nil {
301-
milestone = *issue.Milestone.Title
302-
}
303-
var labels = make([]*base.Label, 0, len(issue.Labels))
304-
for _, l := range issue.Labels {
305-
labels = append(labels, convertGithubLabel(&l))
306-
}
307-
var reactions *base.Reactions
308-
if issue.Reactions != nil {
309-
reactions = convertGithubReactions(issue.Reactions)
310-
}
311285

312-
var email string
313-
if issue.User.Email != nil {
314-
email = *issue.User.Email
315-
}
316-
allIssues = append(allIssues, &base.Issue{
317-
Title: *issue.Title,
318-
Number: int64(*issue.Number),
319-
PosterName: *issue.User.Login,
320-
PosterEmail: email,
321-
Content: body,
322-
Milestone: milestone,
323-
State: *issue.State,
324-
Created: *issue.CreatedAt,
325-
Labels: labels,
326-
Reactions: reactions,
327-
Closed: issue.ClosedAt,
328-
IsLocked: *issue.Locked,
329-
})
330-
if len(allIssues) >= limit {
331-
return allIssues, nil
332-
}
286+
var allIssues = make([]*base.Issue, 0, perPage)
287+
288+
issues, _, err := g.client.Issues.ListByRepo(g.ctx, g.repoOwner, g.repoName, opt)
289+
if err != nil {
290+
return nil, false, fmt.Errorf("error while listing repos: %v", err)
291+
}
292+
for _, issue := range issues {
293+
if issue.IsPullRequest() {
294+
continue
333295
}
334-
if resp.NextPage == 0 {
335-
break
296+
var body string
297+
if issue.Body != nil {
298+
body = *issue.Body
336299
}
337-
opt.Page = resp.NextPage
300+
var milestone string
301+
if issue.Milestone != nil {
302+
milestone = *issue.Milestone.Title
303+
}
304+
var labels = make([]*base.Label, 0, len(issue.Labels))
305+
for _, l := range issue.Labels {
306+
labels = append(labels, convertGithubLabel(&l))
307+
}
308+
var reactions *base.Reactions
309+
if issue.Reactions != nil {
310+
reactions = convertGithubReactions(issue.Reactions)
311+
}
312+
313+
var email string
314+
if issue.User.Email != nil {
315+
email = *issue.User.Email
316+
}
317+
allIssues = append(allIssues, &base.Issue{
318+
Title: *issue.Title,
319+
Number: int64(*issue.Number),
320+
PosterName: *issue.User.Login,
321+
PosterEmail: email,
322+
Content: body,
323+
Milestone: milestone,
324+
State: *issue.State,
325+
Created: *issue.CreatedAt,
326+
Labels: labels,
327+
Reactions: reactions,
328+
Closed: issue.ClosedAt,
329+
IsLocked: *issue.Locked,
330+
})
338331
}
339-
return allIssues, nil
332+
333+
return allIssues, len(issues) < perPage, nil
340334
}
341335

342336
// GetComments returns comments according issueNumber
@@ -379,97 +373,91 @@ func (g *GithubDownloaderV3) GetComments(issueNumber int64) ([]*base.Comment, er
379373
return allComments, nil
380374
}
381375

382-
// GetPullRequests returns pull requests according start and limit
383-
func (g *GithubDownloaderV3) GetPullRequests(start, limit int) ([]*base.PullRequest, error) {
376+
// GetPullRequests returns pull requests according page and perPage
377+
func (g *GithubDownloaderV3) GetPullRequests(page, perPage int) ([]*base.PullRequest, error) {
384378
opt := &github.PullRequestListOptions{
385379
Sort: "created",
386380
Direction: "asc",
387381
State: "all",
388382
ListOptions: github.ListOptions{
389-
PerPage: 100,
383+
PerPage: perPage,
384+
Page: page,
390385
},
391386
}
392-
var allPRs = make([]*base.PullRequest, 0, 100)
393-
for {
394-
prs, resp, err := g.client.PullRequests.List(g.ctx, g.repoOwner, g.repoName, opt)
395-
if err != nil {
396-
return nil, fmt.Errorf("error while listing repos: %v", err)
397-
}
398-
for _, pr := range prs {
399-
var body string
400-
if pr.Body != nil {
401-
body = *pr.Body
402-
}
403-
var milestone string
404-
if pr.Milestone != nil {
405-
milestone = *pr.Milestone.Title
406-
}
407-
var labels = make([]*base.Label, 0, len(pr.Labels))
408-
for _, l := range pr.Labels {
409-
labels = append(labels, convertGithubLabel(l))
410-
}
387+
var allPRs = make([]*base.PullRequest, 0, perPage)
411388

412-
// FIXME: This API missing reactions, we may need another extra request to get reactions
389+
prs, _, err := g.client.PullRequests.List(g.ctx, g.repoOwner, g.repoName, opt)
390+
if err != nil {
391+
return nil, fmt.Errorf("error while listing repos: %v", err)
392+
}
393+
for _, pr := range prs {
394+
var body string
395+
if pr.Body != nil {
396+
body = *pr.Body
397+
}
398+
var milestone string
399+
if pr.Milestone != nil {
400+
milestone = *pr.Milestone.Title
401+
}
402+
var labels = make([]*base.Label, 0, len(pr.Labels))
403+
for _, l := range pr.Labels {
404+
labels = append(labels, convertGithubLabel(l))
405+
}
413406

414-
var email string
415-
if pr.User.Email != nil {
416-
email = *pr.User.Email
417-
}
418-
var merged bool
419-
// pr.Merged is not valid, so use MergedAt to test if it's merged
420-
if pr.MergedAt != nil {
421-
merged = true
422-
}
407+
// FIXME: This API missing reactions, we may need another extra request to get reactions
423408

424-
var headRepoName string
425-
var cloneURL string
426-
if pr.Head.Repo != nil {
427-
headRepoName = *pr.Head.Repo.Name
428-
cloneURL = *pr.Head.Repo.CloneURL
429-
}
430-
var mergeCommitSHA string
431-
if pr.MergeCommitSHA != nil {
432-
mergeCommitSHA = *pr.MergeCommitSHA
433-
}
409+
var email string
410+
if pr.User.Email != nil {
411+
email = *pr.User.Email
412+
}
413+
var merged bool
414+
// pr.Merged is not valid, so use MergedAt to test if it's merged
415+
if pr.MergedAt != nil {
416+
merged = true
417+
}
434418

435-
allPRs = append(allPRs, &base.PullRequest{
436-
Title: *pr.Title,
437-
Number: int64(*pr.Number),
438-
PosterName: *pr.User.Login,
439-
PosterEmail: email,
440-
Content: body,
441-
Milestone: milestone,
442-
State: *pr.State,
443-
Created: *pr.CreatedAt,
444-
Closed: pr.ClosedAt,
445-
Labels: labels,
446-
Merged: merged,
447-
MergeCommitSHA: mergeCommitSHA,
448-
MergedTime: pr.MergedAt,
449-
IsLocked: pr.ActiveLockReason != nil,
450-
Head: base.PullRequestBranch{
451-
Ref: *pr.Head.Ref,
452-
SHA: *pr.Head.SHA,
453-
RepoName: headRepoName,
454-
OwnerName: *pr.Head.User.Login,
455-
CloneURL: cloneURL,
456-
},
457-
Base: base.PullRequestBranch{
458-
Ref: *pr.Base.Ref,
459-
SHA: *pr.Base.SHA,
460-
RepoName: *pr.Base.Repo.Name,
461-
OwnerName: *pr.Base.User.Login,
462-
},
463-
PatchURL: *pr.PatchURL,
464-
})
465-
if len(allPRs) >= limit {
466-
return allPRs, nil
467-
}
419+
var headRepoName string
420+
var cloneURL string
421+
if pr.Head.Repo != nil {
422+
headRepoName = *pr.Head.Repo.Name
423+
cloneURL = *pr.Head.Repo.CloneURL
468424
}
469-
if resp.NextPage == 0 {
470-
break
425+
var mergeCommitSHA string
426+
if pr.MergeCommitSHA != nil {
427+
mergeCommitSHA = *pr.MergeCommitSHA
471428
}
472-
opt.Page = resp.NextPage
429+
430+
allPRs = append(allPRs, &base.PullRequest{
431+
Title: *pr.Title,
432+
Number: int64(*pr.Number),
433+
PosterName: *pr.User.Login,
434+
PosterEmail: email,
435+
Content: body,
436+
Milestone: milestone,
437+
State: *pr.State,
438+
Created: *pr.CreatedAt,
439+
Closed: pr.ClosedAt,
440+
Labels: labels,
441+
Merged: merged,
442+
MergeCommitSHA: mergeCommitSHA,
443+
MergedTime: pr.MergedAt,
444+
IsLocked: pr.ActiveLockReason != nil,
445+
Head: base.PullRequestBranch{
446+
Ref: *pr.Head.Ref,
447+
SHA: *pr.Head.SHA,
448+
RepoName: headRepoName,
449+
OwnerName: *pr.Head.User.Login,
450+
CloneURL: cloneURL,
451+
},
452+
Base: base.PullRequestBranch{
453+
Ref: *pr.Base.Ref,
454+
SHA: *pr.Base.SHA,
455+
RepoName: *pr.Base.Repo.Name,
456+
OwnerName: *pr.Base.User.Login,
457+
},
458+
PatchURL: *pr.PatchURL,
459+
})
473460
}
461+
474462
return allPRs, nil
475463
}

‎modules/migrations/github_test.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,9 +166,11 @@ func TestGitHubDownloadRepo(t *testing.T) {
166166
}, releases[len(releases)-1:])
167167

168168
// downloader.GetIssues()
169-
issues, err := downloader.GetIssues(0, 3)
169+
issues, isEnd, err := downloader.GetIssues(1, 8)
170170
assert.NoError(t, err)
171171
assert.EqualValues(t, 3, len(issues))
172+
assert.False(t, isEnd)
173+
172174
var (
173175
closed1 = time.Date(2018, 10, 23, 02, 57, 43, 0, time.UTC)
174176
)
@@ -319,7 +321,7 @@ something like in the latest 15days could be enough don't you think ?
319321
}, comments[:3])
320322

321323
// downloader.GetPullRequests()
322-
prs, err := downloader.GetPullRequests(0, 3)
324+
prs, err := downloader.GetPullRequests(1, 3)
323325
assert.NoError(t, err)
324326
assert.EqualValues(t, 3, len(prs))
325327

‎modules/migrations/migrate.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -128,8 +128,8 @@ func migrateRepository(downloader base.Downloader, uploader base.Uploader, opts
128128

129129
if opts.Issues {
130130
log.Trace("migrating issues and comments")
131-
for {
132-
issues, err := downloader.GetIssues(0, 100)
131+
for i := 1; ; i++ {
132+
issues, isEnd, err := downloader.GetIssues(i, 100)
133133
if err != nil {
134134
return err
135135
}
@@ -160,16 +160,16 @@ func migrateRepository(downloader base.Downloader, uploader base.Uploader, opts
160160
}
161161
}
162162

163-
if len(issues) < 100 {
163+
if isEnd {
164164
break
165165
}
166166
}
167167
}
168168

169169
if opts.PullRequests {
170170
log.Trace("migrating pull requests and comments")
171-
for {
172-
prs, err := downloader.GetPullRequests(0, 100)
171+
for i := 1; ; i++ {
172+
prs, err := downloader.GetPullRequests(i, 100)
173173
if err != nil {
174174
return err
175175
}

0 commit comments

Comments
 (0)