wip: fix(prow/plugins/trigger): use git client instead of github PushEvent while checking files changed

[FedeDP]

So, i opened this one just to start a discussion.
In falcosecurity/test-infra we have been hit by a Github API limit: basically, we have a weekly cron job that opens a pull request on our test-infra repo which adds lots of new files.
As stated by github docs, Github limits the number of files changed in the pull request and push events to 3000:

Note: Responses include a maximum of 3000 files. The paginated response returns 30 files per page by default.

This means that some post-submit jobs are not being triggered for us.

I saw that prow trigger plugin directly uses the GithubEvent list of files for both pull_request and push, being then hit by the aforementioned Github API limit.
This PR updates push code to directly use a git client diff instead (fallbacking at the github event when the git client fails to be retrieved).

There are a couple of issues:

• i don't know how to properly update tests, since they directly use the GithubEvent and i'd need to mock a git repo instead
• pull_request still use files from github event

Tracking issue on falcosecurity/test-infra: falcosecurity/test-infra#1158

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: FedeDP / name: Federico Di Pierro (bba9ea4)

[k8s-ci-robot]

Welcome @FedeDP!

It looks like this is your first PR to kubernetes/test-infra 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/test-infra has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @FedeDP. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: FedeDP
Once this PR has been reviewed and has the lgtm label, please assign cjwagner for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• prow/plugins/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[FedeDP]

Any hint?

[jmguzik]

This PR updates push code to directly use a git client diff instead (fall-backing at the github event when the git client fails to be retrieved).

To keep compatibility, shouldn't it be the other way? If your code fails, just add git client as a fallback method.

There are a couple of issues:
i don't know how to properly update tests, since they directly use the GithubEvent and i'd need to mock a git repo >instead pull_request still use files from github event

I would do https://docs.prow.k8s.io/docs/github/#interfacing-a-subset-of-client but for the git client.

[jmguzik]

I would use this as a fallback method

[FedeDP]

But how can we know when do we need to fallback? I mean, we cannot know in advance whether the Github Event contains truncated list of changed files, right?
Unless we state that if len(changedFiles) == 3000 then we must fallback to the git client method. Of course that would work for a post submit that changed exactly 3000 files too :D

[jmguzik]

The thing is Idk what are the consequences of this decision and what would be the difference, but I know trigger is quite important plugin and I do not want to break it's compatibility. So either you properly test the code and prove your solution is a good a valid replacement or you just execute github version, you fail because of the limit you mentioned and then renew using git client. The github version was working up until now for most of cases.

[FedeDP]

but I know trigger is quite important plugin and I do not want to break it's compatibility

💯

The github version was working up until now for most of cases.

Agree, of course!

So either you properly test the code and prove your solution is a good a valid replacement or you just execute github version, you fail because of the limit you mentioned and then renew using git client.

If only there was a way to know whether Github truncated its event, it would be fairly easy.
I will dig into that, thank you very much!

[jmguzik]

/ok-to-test

[jmguzik]

Any hint?

You were probably left without any feedback because of WIP label. I am here just because of random clicks :)

[FedeDP]

You were probably left without any feedback because of WIP label. I am here just because of random clicks :)

No problem! I will look into implementing tests, then eventually implement the fix for presubmit too (ie: PRs), if we all agree about the correct way to handle this.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[FedeDP]

/remove-lifecycle rotten

[k8s-ci-robot]

PR needs rebase.

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/test-infra repository.

[jihoon-seo]

Hello @FedeDP, the Prow source code has been moved to https://github.com/kubernetes-sigs/prow on April 9, 2024.
Please consider migrate this PR (by opening a new one in there).
Thanks!

[FedeDP]

Closing this one! Will arrange to add the new patch to the new repo!
Thanks for the hint :)
/close

[k8s-ci-robot]

@FedeDP: Closed this PR.

In response to this:

Closing this one! Will arrange to add the new patch to the new repo!
Thanks for the hint :)
/close

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/test-infra repository.