-
Notifications
You must be signed in to change notification settings - Fork 99
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
fix(pkg): fallback at using git client when github API files limits is reached #146
base: main
Are you sure you want to change the base?
Changes from 1 commit
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 |
---|---|---|
|
@@ -21,6 +21,7 @@ import ( | |
"fmt" | ||
"net/url" | ||
"regexp" | ||
"sigs.k8s.io/prow/pkg/git/v2" | ||
"strings" | ||
"time" | ||
|
||
|
@@ -525,7 +526,8 @@ type githubClient interface { | |
// NewGitHubDeferredChangedFilesProvider uses a closure to lazily retrieve the file changes only if they are needed. | ||
// We only have to fetch the changes if there is at least one RunIfChanged/SkipIfOnlyChanged job that is not being | ||
// force run (due to a `/retest` after a failure or because it is explicitly triggered with `/test foo`). | ||
func NewGitHubDeferredChangedFilesProvider(client githubClient, org, repo string, num int) ChangedFilesProvider { | ||
func NewGitHubDeferredChangedFilesProvider(gc git.ClientFactory, client githubClient, org, repo string, num int, | ||
baseSHA, headSHA string) ChangedFilesProvider { | ||
var changedFiles []string | ||
return func() ([]string, error) { | ||
// Fetch the changed files from github at most once. | ||
|
@@ -534,8 +536,25 @@ func NewGitHubDeferredChangedFilesProvider(client githubClient, org, repo string | |
if err != nil { | ||
return nil, fmt.Errorf("error getting pull request changes: %w", err) | ||
} | ||
for _, change := range changes { | ||
changedFiles = append(changedFiles, change.Filename) | ||
|
||
// Fallback to use gitClient since github API truncated the response | ||
if len(changes) == github.ChangesFilesLimit && gc != nil { | ||
repoClient, err := gc.ClientFor(org, repo) | ||
if err == nil { | ||
// Use git client since Github PushEvent is limited to 3000 keys: | ||
// https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests-files | ||
files, err := repoClient.Diff(headSHA, baseSHA) | ||
if err == nil { | ||
changedFiles = files | ||
} | ||
} | ||
} | ||
|
||
// in all failure cases, return truncated files | ||
if len(changedFiles) == 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. If the gitclient approach fails, return the truncated list of files. |
||
for _, change := range changes { | ||
changedFiles = append(changedFiles, change.Filename) | ||
} | ||
} | ||
} | ||
return changedFiles, nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,28 +21,46 @@ import ( | |
|
||
prowapi "sigs.k8s.io/prow/pkg/apis/prowjobs/v1" | ||
"sigs.k8s.io/prow/pkg/config" | ||
"sigs.k8s.io/prow/pkg/git/v2" | ||
"sigs.k8s.io/prow/pkg/github" | ||
"sigs.k8s.io/prow/pkg/pjutil" | ||
) | ||
|
||
func listPushEventChanges(pe github.PushEvent) config.ChangedFilesProvider { | ||
func listPushEventChanges(gc git.ClientFactory, pe github.PushEvent) config.ChangedFilesProvider { | ||
var changedFiles []string | ||
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. This should be the only actual "breaking change", ie: cache |
||
return func() ([]string, error) { | ||
changed := make(map[string]bool) | ||
for _, commit := range pe.Commits { | ||
for _, added := range commit.Added { | ||
changed[added] = true | ||
if changedFiles == nil { | ||
// Fallback to use PushEvent | ||
changes := make(map[string]bool) | ||
for _, commit := range pe.Commits { | ||
for _, added := range commit.Added { | ||
changes[added] = true | ||
} | ||
for _, removed := range commit.Removed { | ||
changes[removed] = true | ||
} | ||
for _, modified := range commit.Modified { | ||
changes[modified] = true | ||
} | ||
} | ||
for _, removed := range commit.Removed { | ||
changed[removed] = true | ||
if len(changes) == github.ChangesFilesLimit && gc != nil { | ||
repoClient, err := gc.ClientFor(pe.Repo.Owner.Name, pe.Repo.Name) | ||
if err == nil { | ||
// Use git client since Github PushEvent is limited to 3000 keys: | ||
// https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests-files | ||
files, err := repoClient.Diff(pe.After, pe.Before) | ||
if err == nil { | ||
changedFiles = files | ||
} | ||
} | ||
} | ||
for _, modified := range commit.Modified { | ||
changed[modified] = true | ||
|
||
if len(changedFiles) == 0 { | ||
for file := range changes { | ||
changedFiles = append(changedFiles, file) | ||
} | ||
} | ||
} | ||
var changedFiles []string | ||
for file := range changed { | ||
changedFiles = append(changedFiles, file) | ||
} | ||
return changedFiles, nil | ||
} | ||
} | ||
|
@@ -74,7 +92,7 @@ func handlePE(c Client, pe github.PushEvent) error { | |
postsubmits := getPostsubmits(c.Logger, c.GitClient, c.Config, org+"/"+repo, shaGetter) | ||
|
||
for _, j := range postsubmits { | ||
if shouldRun, err := j.ShouldRun(pe.Branch(), listPushEventChanges(pe)); err != nil { | ||
if shouldRun, err := j.ShouldRun(pe.Branch(), listPushEventChanges(c.GitClient, pe)); err != nil { | ||
return err | ||
} else if !shouldRun { | ||
continue | ||
|
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.
As suggested here: kubernetes/test-infra#30615 (comment) this is now a fallback, that is only executed when:
Of course, this means that if unluckily number of changes is exactly 3000, we would run the git client flow for nothing.