-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
retry change detection #598
Changes from all commits
da43426
e0a99a1
a08121e
76a1dc4
8e96cae
11c9ffc
7977abd
d40a5b6
2d822fb
22b1493
3eb6a3c
8441fdd
2e005c2
e142c16
9ded633
049db2d
b2d370b
3441cd7
e8007e5
7889402
161e368
ab1286a
b3f1daf
edb9eb8
0f9123b
3b382da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
name: CI | ||
|
||
on: | ||
pull_request: | ||
types: [ labeled ] | ||
|
||
jobs: | ||
build: | ||
if: ${{ contains( github.event.label.name, 'needs-go-generate') }} | ||
runs-on: ubuntu-latest | ||
|
||
steps: | ||
- uses: actions/checkout@v2 | ||
- name: Fail on needs-go-generate | ||
run: echo Failed, needs go generate. && exit 1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
make/repo.Makefile |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,14 +4,15 @@ import ( | |
"context" | ||
"encoding/json" | ||
"fmt" | ||
"os" | ||
"strings" | ||
|
||
"github.com/avast/retry-go" | ||
"github.com/google/go-github/v41/github" | ||
"github.com/synapsecns/sanguine/contrib/git-changes-action/detector/actionscore" | ||
"github.com/synapsecns/sanguine/contrib/git-changes-action/detector/tree" | ||
"golang.org/x/exp/slices" | ||
"golang.org/x/oauth2" | ||
"os" | ||
"strings" | ||
"time" | ||
) | ||
|
||
// GetChangeTree returns the ref for the given event name. | ||
|
@@ -64,15 +65,27 @@ func getChangedFilesFromAPI(ctx context.Context, ghContext *actionscore.Context, | |
ct = tree.NewTree() | ||
|
||
page := 1 | ||
perPage := 20 | ||
const retryCount = 10 | ||
const perPage = 100 | ||
for { | ||
files, res, err := client.PullRequests.ListFiles(ctx, repoOwner, repoName, prNumber, &github.ListOptions{ | ||
Page: page, | ||
PerPage: perPage, | ||
}) | ||
var files []*github.CommitFile | ||
var res *github.Response | ||
err = retry.Do(func() error { | ||
reqCtx, cancel := context.WithTimeout(ctx, 15*time.Second) | ||
defer cancel() | ||
|
||
files, res, err = client.PullRequests.ListFiles(reqCtx, repoOwner, repoName, prNumber, &github.ListOptions{ | ||
Page: page, | ||
PerPage: perPage, | ||
}) | ||
if err != nil { | ||
return 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 | ||
}, retry.Context(ctx), retry.Attempts(retryCount)) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done and wrapped above |
||
} | ||
|
||
for _, file := range files { | ||
|
@@ -84,7 +97,7 @@ func getChangedFilesFromAPI(ctx context.Context, ghContext *actionscore.Context, | |
} | ||
} | ||
|
||
if page == res.LastPage { | ||
if res.NextPage == 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
break | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent so much time on this :-)