-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Use git command instead of GitHub API to list modified files #10106
Use git command instead of GitHub API to list modified files #10106
Conversation
c379d64
to
6d3f81a
Compare
57ed844
to
5dd263c
Compare
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
5dd263c
to
5be6559
Compare
6103d3a
to
5be6559
Compare
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
88bdea4
to
12823e2
Compare
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
12823e2
to
125b391
Compare
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.
Nice work! Can we open an upstream PR if we haven't yet? The less we have to maintain the better :-)
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.
This is great work!
+1 to @mattlord's suggestion of opening an upstream PR.
Not merging this right now, because I believe @frouioui intends to rename the |
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Pull Request to upstream is available dorny/paths-filter#133. |
…filter Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Description
This Pull Request aims to fix the API Rate error we were observing with the new
dorny/paths-filter
integration. We recently implemented a feature that skips workflows based on the list of modified files, usingdorny/paths-filter
, however, when several commits were pushed at the same time we observed the following error:We think this can be alleviated by simply not using the GitHub API. In fact, even if
dorny/paths-filter
was making only API call per workflow, we would easily reach the 1000 requests/hour limit with over 80 workflows. Simply push 13 commits in the Vitess organization within the same hour and the limit would be reached.In
dorny/paths-filter
's README, the usage of the tool is documented as follows:Setting
token
to an empty string should therefore force the use of thegit
command instead of the GitHub API. After trying to set thetoken
to an empty string, there was a weird behavior where running the followinggit
command would return a large list of modified files (even though they are not modified by the PR).In this workflow, we can see that
paths-filter
is attempting to calculate the diff using thegit
command. However, as we can see in the screenshot below, they find over 4000 modified files, which does not correspond to the changes made by this commit or the other commits on the PR's branch.The
git log --format ...
command is used by default indorny/paths-filter
whentoken
is set to an empty string. To fix this issue, I forkeddorny/paths-filter
(here) and pushed a commit that fixes the issue. I changed all the workflows to use this new fork.The fix consists of doing a
diff
using the following command:That way we only capture the difference that happened between the PR's base and the current commit.
Checklist