Skip to content
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

Compare base and ref when token is empty #133

Merged
merged 3 commits into from Feb 15, 2024
Merged

Compare base and ref when token is empty #133

merged 3 commits into from Feb 15, 2024

Conversation

frouioui
Copy link
Contributor

Description

After we started using paths-filter on all our +80 workflows, we quickly saw API rate limit errors. The exact issue is detailed in both #108 and #73. In our case, this issue would arise pretty quickly considering that each workflow uses paths-filter and thus executes at least one API call. The 1000 requests/hour limit can quickly be met after a dozen of commits.

To alleviate this issue, we considered not using the GitHub API at all. In fact, a simple git diff should be sufficient. The documentation says we can set token to an empty string to force the use of the git command. Doing so resulted in paths-filter executing the git command below. That command would return an excessively large number of modified files (even though they were not modified by PR/commit). The screenshot below exhibits this large number of modified files.

git log --format= --no-renames --name-status -z -n 1 

image

This PR fixes this issue by using getChanges instead of getChangesInLastCommit. Turns out that getChanges is more accurate since we can provide a range that starts at the base and ends at the head/current commit.

We started using this fix in vitessio/vitess#10106. The fix seems to have alleviated all of the issues we were previously seeing.

Related Issues

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
mitchell-codecov added a commit to mitchell-codecov/paths-filter that referenced this pull request Jun 15, 2022
@taschetto
Copy link

Would love to help moving this forward. Any ideas how @dorny? @frouioui?

@astorrs
Copy link

astorrs commented Feb 14, 2024

@dorny Can this be merged in? Without it we're effectively blocked from using the action on PRs.

@frouioui
Copy link
Contributor Author

I am happy to fix the conflict @dorny if we're good to merge.

@dorny
Copy link
Owner

dorny commented Feb 14, 2024

@astorrs @frouioui Thanks guys, the PR seems reasonable. I'm sorry that I've overlooked it for so long. I will resolve the conflicts and merge the PR tomorrow.

@dorny dorny merged commit 45f16f1 into dorny:master Feb 15, 2024
7 checks passed
@dorny
Copy link
Owner

dorny commented Feb 15, 2024

Released as v3.0.1

@frouioui
Copy link
Contributor Author

Thanks @dorny ! 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants