fix: comment list pagination #21
Conversation
pkg/notifier/gitlab/comment.go
Outdated
| if sentinel > 100 { | ||
| return nil, fmt.Errorf("gitlab.comment.list: too many pages, something went wrong") | ||
| } |
There was a problem hiding this comment.
pagination from example https://github.com/xanzy/go-gitlab/blob/main/examples/pagination.go
plus sentinel. I think 100 * 100 = 10,000 comments are insane.
There was a problem hiding this comment.
I can change to break with waring message instead err, if it's your preference design
.github/workflows/test.yaml
Outdated
| with: | ||
| aqua_version: v2.10.1 | ||
| aqua_version: v2.27.0 |
There was a problem hiding this comment.
I got a error and resolved by update aqua: https://github.com/hirosassa/tfcmt-gitlab/actions/runs/8629098634/job/23652549544
|
I could confirm that a MR which has 60+ comments are patched properly with this fix. Without this fix, it had grown to 360+ comments by running CI multiple times 😅 |
|
@hirosassa @kitagry resolved conflict |
pkg/notifier/gitlab/comment_test.go
Outdated
| @@ -125,6 +125,16 @@ func TestCommentPost(t *testing.T) { | |||
| func TestCommentList(t *testing.T) { | |||
| t.Parallel() | |||
| comments := []*gitlab.Note{ | |||
There was a problem hiding this comment.
Could you this variables as inline in testcase?
pkg/notifier/gitlab/comment.go
Outdated
| opt.Page = resp.NextPage | ||
|
|
||
| if sentinel > maxPages { | ||
| return nil, fmt.Errorf("gitlab.comment.list: too many pages, something went wrong") |
There was a problem hiding this comment.
I think it is not necessary to return error. It is better to print log.
|
|
||
| client.API = createMockGitLabAPI(mockCtrl) | ||
|
|
||
| _, err = client.Comment.List(1) // no assert res, only assert `.MaxTimes(100)` |
currently
tfcmt-gitlabfetches only 20 comments. if there are more comments, those won't be patched but posted as new comment. (and increase comment and ....)there can be same issues for other listings, but I think only comment list has problem because
ListCommits: It seems that we use only last commit, so first page is enoughListMergeRequestsByCommit: no pagination function? and single page may enoughrelated document: https://docs.gitlab.com/ee/api/rest/index.html#pagination