retry change detection #598
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #598 +/- ##
===================================================
- Coverage 44.22765% 44.21531% -0.01234%
===================================================
Files 469 469
Lines 39412 39423 +11
Branches 183 183
===================================================
Hits 17431 17431
- Misses 19874 19885 +11
Partials 2107 2107
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -81,7 +95,7 @@ func getChangedFilesFromAPI(ctx context.Context, ghContext *actionscore.Context, | |||
| } | |||
| } | |||
|
|
|||
| if page == res.LastPage { | |||
| if res.NextPage == 0 { | |||
There was a problem hiding this comment.
I am not sure this will work? Keeping it res.LastPage worked for me in the messaging branch that had multiple pages.
There was a problem hiding this comment.
I ran it w/ a fmt.Println and you can see the last page detection not working here https://github.com/synapsecns/sanguine/actions/runs/4309209034/jobs/7516328981
It'd get to 12 and then reset to 1. res.LastPage can be 0 when the cursor is lost
|
|
||
| files, res, err = client.PullRequests.ListFiles(reqCtx, repoOwner, repoName, prNumber, &github.ListOptions{ | ||
| Page: page, | ||
| PerPage: 100, |
There was a problem hiding this comment.
In master, I now changed this to PerPage: 20 and it worked after not working for a long time. Not sure if I just ended up getting lucky with that, but maybe just use 20 since that worked for me?
There was a problem hiding this comment.
so max is 100, 20 will cause rate limit hits during high commit times
There was a problem hiding this comment.
Yeah, just slacked you before seeing this.
My thing failed getting page 2. Definitely leave at 100.
| @@ -14,6 +14,7 @@ require ( | |||
| github.com/coinbase/rosetta-sdk-go v0.8.1 | |||
| github.com/davecgh/go-spew v1.1.1 | |||
| github.com/ethereum/go-ethereum v1.10.26 | |||
| github.com/hedzr/log v1.6.3 | |||
There was a problem hiding this comment.
I spent so much time on this :-)
| if err != nil { | ||
| return nil, fmt.Errorf("could not get files for repoOwner %s, repoName %s, prNumber %d, page number %d with page size %d: %w", | ||
| repoOwner, repoName, prNumber, page, perPage, err) | ||
| return nil, fmt.Errorf("could not get files after %d retries: %w", retryCount, err) |
There was a problem hiding this comment.
Maybe include the information I had in my log about the page number and repoName and prNumber and repoOwner?
There was a problem hiding this comment.
done and wrapped above
trajan0x
requested review from
nautsimon,
CryptoMaxPlanck and
ChiTimesChi
as code owners
Description
Add retry change detection