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

fix: retrieve all changes via api #50

Merged
merged 2 commits into from
Nov 13, 2020
Merged

fix: retrieve all changes via api #50

merged 2 commits into from
Nov 13, 2020

Conversation

simllll
Copy link
Contributor

@simllll simllll commented Nov 13, 2020

the number returned by pullRequest.changed_files doesn't reflect the correct number of real changed files. Therefore lot of filters didn't work in my scenario, as it said nothing has changed. This especially happens if there are more than 100 files changed (first time I experienced it where over 1000 files have changed, and now when there were about 300 files that have changed).

I changed the detection by querying the api as long as it returns new results. This ensures all pages have been retrieved. It's tested and used in production on our end, but please check again if I didn't miss anything.

Thanks a lot for this awesome github action :)

the number returned by pullRequest.changed_files doesn't reflect the correct number of real changed files. Therefore lot of filters didn't work in my scenario, as it said nothing has changed. This especially happens if there are more than 100 files changed (first time I experienced it where over 1000 files have changed, and now when there were about 300 files that have changed). 

I changed the detection by querying the api as long as it returns new results. This ensures all pages have been retrieved. It's tested and used in production on our end, but please check again if I didn't miss anything.

Thanks a lot for this awesome github action :)
@dorny
Copy link
Owner

dorny commented Nov 13, 2020

Thank You for contribution 👍

It's quite weird that changed_files has wrong values but listing the files works.
I quickly checked the docs and tried to google something but found nothing.

Merging it now. Just after that I will release it as a new version.

@dorny dorny merged commit dec8b80 into dorny:master Nov 13, 2020
dorny added a commit that referenced this pull request Nov 13, 2020
@dorny
Copy link
Owner

dorny commented Nov 13, 2020

I just did one additional test from my test repo and it somehow lists changed files twice.
It doesn't break the filter true/false output but it breaks the listing functionality.
I'm just looking into it... Probably the last "page" from listFiles() is processed twice.
I will wait a bit with releasing this until I fix it.

@dorny
Copy link
Owner

dorny commented Nov 13, 2020

@simllll
Looks like the root cause of issue was not that pullRequest.changed_files had wrong value. I just used 0 as a start page instead of 1. I just tested it in my private test repo where I added 1000+ files and it worked.

Fix is released as v2.5.3. Could you please verify if it works for you?
If the pullRequest.changed_files really reports invalid value for you then we can change it back to your implementation with while loop.

@simllll
Copy link
Contributor Author

simllll commented Nov 13, 2020

Cool to hear, thanks for looking into it! I will test it tomorrow, but it sounds pretty reasonable ;) thanks again!

@simllll
Copy link
Contributor Author

simllll commented Nov 14, 2020

Thanks again, looks good so far!! Seems it's solved now

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

2 participants