fix(pkg): fallback at using git client when github API files limits is reached #146
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: FedeDP The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @FedeDP! |
|
Hi @FedeDP. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
needs-ok-to-test
✅ Deploy Preview for k8s-prow ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
the
size/L
| changedFiles = append(changedFiles, change.Filename) | ||
|
|
||
| // Fallback to use gitClient since github API truncated the response | ||
| if len(changes) == github.ChangesFilesLimit && gc != nil { |
There was a problem hiding this comment.
As suggested here: kubernetes/test-infra#30615 (comment) this is now a fallback, that is only executed when:
- gitclient is not nil
- length of changes is exactly 3000
Of course, this means that if unluckily number of changes is exactly 3000, we would run the git client flow for nothing.
| } | ||
|
|
||
| // in all failure cases, return truncated files | ||
| if len(changedFiles) == 0 { |
There was a problem hiding this comment.
If the gitclient approach fails, return the truncated list of files.
pkg/plugins/trigger/push.go
Outdated
| } | ||
| } | ||
| var changedFiles []string | ||
| for file := range changed { | ||
| changedFiles = append(changedFiles, file) | ||
| if len(changes) == github.ChangesFilesLimit && gc != nil { |
There was a problem hiding this comment.
As above for pull requests, only load the new code if exactly 3000 files changes are found.
pkg/plugins/trigger/push.go
Outdated
| } | ||
| } | ||
|
|
||
| if len(changedFiles) == 0 { |
There was a problem hiding this comment.
If the gitclient approach fails, return the truncated list of files just like before.
|
Note: i am still trying to understand how to test these changes; i am currently building all images and pushing them under my own dockerhub account; then i might switch our https://github.com/falcosecurity/test-infra repo to use my own prow images and see if triggering a job that failed before, works fine after. |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
…s 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 <[email protected]>
FedeDP
force-pushed
the
fix/trigger_github_api_files_limits
branch
from
4651916
to
ad16d86
Compare
| "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.
This should be the only actual "breaking change", ie: cache changedFiles just like we do in pull-request.go, to avoid a git client diff call for each configured post submit when we are in the fallback case.
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Signed-off-by: Federico Di Pierro <[email protected]>
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
/remove-lifecycle stale Sorry i still hadn't a chance to test this. |
k8s-ci-robot
removed
the
lifecycle/stale
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.
See https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests-files.
Porting of kubernetes/test-infra#30615 to the new repo.