From 46519166567b84153e0babbc745bbb2245ac8588 Mon Sep 17 00:00:00 2001 From: Federico Di Pierro Date: Wed, 8 May 2024 14:00:09 +0200 Subject: [PATCH] fix(pkg): fallback at using git client when github API files limits is reached. Github API has an hard limit of 3000 files returned for push/pull request events. Workaround this when checking pre/post submit jobs to be triggered, by manually diffing using the git client. Signed-off-by: Federico Di Pierro --- cmd/status-reconciler/main.go | 7 +++++- pkg/config/jobs.go | 25 ++++++++++++++++--- pkg/github/types.go | 4 ++++ pkg/plugins/skip/skip.go | 2 +- pkg/plugins/trigger/generic-comment.go | 16 +++++++------ pkg/plugins/trigger/pull-request.go | 4 ++-- pkg/plugins/trigger/push.go | 33 +++++++++++++++++++------- pkg/statusreconciler/controller.go | 9 ++++--- 8 files changed, 75 insertions(+), 25 deletions(-) diff --git a/cmd/status-reconciler/main.go b/cmd/status-reconciler/main.go index 23832b8ccf..bfffba8936 100644 --- a/cmd/status-reconciler/main.go +++ b/cmd/status-reconciler/main.go @@ -127,6 +127,11 @@ func main() { logrus.WithError(err).Fatal("Error getting GitHub client.") } + gitClient, err := o.github.GitClientFactory("", &o.config.InRepoConfigCacheDirBase, o.dryRun, false) + if err != nil { + logrus.WithError(err).Fatal("Error getting Git client.") + } + prowJobClient, err := o.kubernetes.ProwJobClient(configAgent.Config().ProwJobNamespace, o.dryRun) if err != nil { logrus.WithError(err).Fatal("Error getting kube client.") @@ -139,7 +144,7 @@ func main() { logrus.WithError(err).Fatal("Cannot create opener") } - c := statusreconciler.NewController(o.continueOnError, o.getDenyList(), o.getDenyListAll(), opener, o.config, o.statusURI, prowJobClient, githubClient, pluginAgent) + c := statusreconciler.NewController(o.continueOnError, o.getDenyList(), o.getDenyListAll(), opener, o.config, o.statusURI, prowJobClient, gitClient, githubClient, pluginAgent) interrupts.Run(func(ctx context.Context) { c.Run(ctx) }) diff --git a/pkg/config/jobs.go b/pkg/config/jobs.go index 06c017ea1d..0bf2bb6bd4 100644 --- a/pkg/config/jobs.go +++ b/pkg/config/jobs.go @@ -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 { + for _, change := range changes { + changedFiles = append(changedFiles, change.Filename) + } } } return changedFiles, nil diff --git a/pkg/github/types.go b/pkg/github/types.go index 250c1093ce..76255e329f 100644 --- a/pkg/github/types.go +++ b/pkg/github/types.go @@ -51,6 +51,10 @@ const ( // DefaultGraphQLEndpoint is the default GitHub GraphQL API endpoint. DefaultGraphQLEndpoint = "https://api.github.com/graphql" + + // ChangesFilesLimit is the limit to the files changed pushed by the github rest API. + // See https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests-files. + ChangesFilesLimit = 3000 ) var ( diff --git a/pkg/plugins/skip/skip.go b/pkg/plugins/skip/skip.go index 922d700757..d25f41a658 100644 --- a/pkg/plugins/skip/skip.go +++ b/pkg/plugins/skip/skip.go @@ -114,7 +114,7 @@ func handle(gc githubClient, log *logrus.Entry, e *github.GenericCommentEvent, c } statuses := combinedStatus.Statuses - filteredPresubmits, err := trigger.FilterPresubmits(honorOkToTest, gc, e.Body, pr, presubmits, log) + filteredPresubmits, err := trigger.FilterPresubmits(honorOkToTest, gitClient, gc, e.Body, pr, presubmits, log) if err != nil { resp := fmt.Sprintf("Cannot get combined status for PR #%d in %s/%s: %v", number, org, repo, err) log.Warn(resp) diff --git a/pkg/plugins/trigger/generic-comment.go b/pkg/plugins/trigger/generic-comment.go index da8c1e1269..5a29dedfb6 100644 --- a/pkg/plugins/trigger/generic-comment.go +++ b/pkg/plugins/trigger/generic-comment.go @@ -18,6 +18,7 @@ package trigger import ( "fmt" + "sigs.k8s.io/prow/pkg/git/v2" "github.com/sirupsen/logrus" "sigs.k8s.io/prow/pkg/kube" @@ -126,12 +127,12 @@ func handleGenericComment(c Client, trigger plugins.Trigger, gc github.GenericCo return err } - toTest, err := FilterPresubmits(HonorOkToTest(trigger), c.GitHubClient, gc.Body, pr, presubmits, c.Logger) + toTest, err := FilterPresubmits(HonorOkToTest(trigger), c.GitClient, c.GitHubClient, gc.Body, pr, presubmits, c.Logger) if err != nil { return err } if needsHelp, note := pjutil.ShouldRespondWithHelp(gc.Body, len(toTest)); needsHelp { - return addHelpComment(c.GitHubClient, gc.Body, org, repo, pr.Base.Ref, pr.Number, presubmits, gc.HTMLURL, commentAuthor, note, c.Logger) + return addHelpComment(c.GitClient, c.GitHubClient, gc.Body, org, repo, pr, presubmits, gc.HTMLURL, commentAuthor, note, c.Logger) } // we want to be able to track re-tests separately from the general body of tests additionalLabels := map[string]string{} @@ -195,7 +196,7 @@ type GitHubClient interface { // If a comment that we get matches more than one of the above patterns, we // consider the set of matching presubmits the union of the results from the // matching cases. -func FilterPresubmits(honorOkToTest bool, gitHubClient GitHubClient, body string, pr *github.PullRequest, presubmits []config.Presubmit, logger *logrus.Entry) ([]config.Presubmit, error) { +func FilterPresubmits(honorOkToTest bool, gc git.ClientFactory, gitHubClient GitHubClient, body string, pr *github.PullRequest, presubmits []config.Presubmit, logger *logrus.Entry) ([]config.Presubmit, error) { org, repo, sha := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name, pr.Head.SHA contextGetter := func() (sets.Set[string], sets.Set[string], error) { @@ -212,8 +213,8 @@ func FilterPresubmits(honorOkToTest bool, gitHubClient GitHubClient, body string return nil, err } - number, branch := pr.Number, pr.Base.Ref - changes := config.NewGitHubDeferredChangedFilesProvider(gitHubClient, org, repo, number) + number, branch, baseSHA, headSHA := pr.Number, pr.Base.Ref, pr.Base.SHA, pr.Head.SHA + changes := config.NewGitHubDeferredChangedFilesProvider(gc, gitHubClient, org, repo, number, baseSHA, headSHA) return pjutil.FilterPresubmits(filter, changes, branch, presubmits, logger) } @@ -231,8 +232,9 @@ func getContexts(combinedStatus *github.CombinedStatus) (sets.Set[string], sets. return failedContexts, allContexts } -func addHelpComment(githubClient githubClient, body, org, repo, branch string, number int, presubmits []config.Presubmit, HTMLURL, user, note string, logger *logrus.Entry) error { - changes := config.NewGitHubDeferredChangedFilesProvider(githubClient, org, repo, number) +func addHelpComment(gc git.ClientFactory, githubClient githubClient, body, org, repo string, pr *github.PullRequest, presubmits []config.Presubmit, HTMLURL, user, note string, logger *logrus.Entry) error { + number, branch, baseSHA, headSHA := pr.Number, pr.Base.Ref, pr.Base.SHA, pr.Head.SHA + changes := config.NewGitHubDeferredChangedFilesProvider(gc, githubClient, org, repo, number, baseSHA, headSHA) testAllNames, optionalJobsCommands, requiredJobsCommands, err := pjutil.AvailablePresubmits(changes, branch, presubmits, logger) if err != nil { return err diff --git a/pkg/plugins/trigger/pull-request.go b/pkg/plugins/trigger/pull-request.go index e815eb384a..9cfef32988 100644 --- a/pkg/plugins/trigger/pull-request.go +++ b/pkg/plugins/trigger/pull-request.go @@ -354,8 +354,8 @@ func buildAllButDrafts(c Client, pr *github.PullRequest, eventGUID string, baseS // buildAll ensures that all builds that should run and will be required are built func buildAll(c Client, pr *github.PullRequest, eventGUID string, baseSHA string, presubmits []config.Presubmit) error { - org, repo, number, branch := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name, pr.Number, pr.Base.Ref - changes := config.NewGitHubDeferredChangedFilesProvider(c.GitHubClient, org, repo, number) + org, repo, number, branch, headSHA := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name, pr.Number, pr.Base.Ref, pr.Head.SHA + changes := config.NewGitHubDeferredChangedFilesProvider(c.GitClient, c.GitHubClient, org, repo, number, baseSHA, headSHA) toTest, err := pjutil.FilterPresubmits(pjutil.NewTestAllFilter(), changes, branch, presubmits, c.Logger) if err != nil { return err diff --git a/pkg/plugins/trigger/push.go b/pkg/plugins/trigger/push.go index 68c59c6c30..01182c1ad6 100644 --- a/pkg/plugins/trigger/push.go +++ b/pkg/plugins/trigger/push.go @@ -21,27 +21,44 @@ 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 { return func() ([]string, error) { - changed := make(map[string]bool) + + // Fallback to use PushEvent + changes := make(map[string]bool) for _, commit := range pe.Commits { for _, added := range commit.Added { - changed[added] = true + changes[added] = true } for _, removed := range commit.Removed { - changed[removed] = true + changes[removed] = true } for _, modified := range commit.Modified { - changed[modified] = true + changes[modified] = true } } var changedFiles []string - for file := range changed { - changedFiles = append(changedFiles, file) + 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 + } + } + } + + if len(changedFiles) == 0 { + for file := range changes { + changedFiles = append(changedFiles, file) + } } return changedFiles, nil } @@ -74,7 +91,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 diff --git a/pkg/statusreconciler/controller.go b/pkg/statusreconciler/controller.go index 221d56c51e..d7545ca766 100644 --- a/pkg/statusreconciler/controller.go +++ b/pkg/statusreconciler/controller.go @@ -19,6 +19,7 @@ package statusreconciler import ( "context" "fmt" + "sigs.k8s.io/prow/pkg/git/v2" "strings" "time" @@ -38,7 +39,7 @@ import ( ) // NewController constructs a new controller to reconcile stauses on config change -func NewController(continueOnError bool, addedPresubmitDenylist, addedPresubmitDenylistAll sets.Set[string], opener io.Opener, configOpts configflagutil.ConfigOptions, statusURI string, prowJobClient prowv1.ProwJobInterface, githubClient github.Client, pluginAgent *plugins.ConfigAgent) *Controller { +func NewController(continueOnError bool, addedPresubmitDenylist, addedPresubmitDenylistAll sets.Set[string], opener io.Opener, configOpts configflagutil.ConfigOptions, statusURI string, prowJobClient prowv1.ProwJobInterface, gc git.ClientFactory, githubClient github.Client, pluginAgent *plugins.ConfigAgent) *Controller { sc := &statusController{ logger: logrus.WithField("client", "statusController"), opener: opener, @@ -57,6 +58,7 @@ func NewController(continueOnError bool, addedPresubmitDenylist, addedPresubmitD pluginAgent: pluginAgent, }, githubClient: githubClient, + gitClient: gc, statusMigrator: &gitHubMigrator{ githubClient: githubClient, continueOnError: continueOnError, @@ -151,6 +153,7 @@ type Controller struct { addedPresubmitDenylistAll sets.Set[string] prowJobTriggerer prowJobTriggerer githubClient githubClient + gitClient git.ClientFactory statusMigrator statusMigrator trustedChecker trustedChecker statusClient statusClient @@ -247,8 +250,8 @@ func (c *Controller) triggerNewPresubmits(addedPresubmits map[string][]config.Pr filter := pjutil.NewArbitraryFilter(func(p config.Presubmit) (shouldRun bool, forcedToRun bool, defaultBehavior bool) { return true, false, true }, "inline-filter") - org, repo, number, branch := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name, pr.Number, pr.Base.Ref - changes := config.NewGitHubDeferredChangedFilesProvider(c.githubClient, org, repo, number) + org, repo, number, branch, baseSHA, headSHA := pr.Base.Repo.Owner.Login, pr.Base.Repo.Name, pr.Number, pr.Base.Ref, pr.Base.SHA, pr.Head.SHA + changes := config.NewGitHubDeferredChangedFilesProvider(c.gitClient, c.githubClient, org, repo, number, baseSHA, headSHA) logger := log.WithFields(logrus.Fields{"org": org, "repo": repo, "number": number, "branch": branch}) toTrigger, err := pjutil.FilterPresubmits(filter, changes, branch, presubmits, logger) if err != nil {