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

Adds pagination to Github request. #4

Merged
merged 1 commit into from
Jan 6, 2020

Conversation

potiuk
Copy link
Contributor

@potiuk potiuk commented Jan 4, 2020

Fixes #3.

Unfortunately most of the Github APIs returning collections returns maximum 30
responses. This is documented here https://developer.github.com/v3/#pagination
but unfortunately it is not mentioned in the particular APIs that are affected.
So when reading https://developer.github.com/v3/pulls/#list-pull-requests it's
not at all obvious that pagination is in operation and that you should add
another loop through pages.

Periodic labeler does not have pagination implemented, therefore the result is
that in big repositories with > 30 PRs only the OLDEST prs will be checked
(that's what is the sorting order). If all those 30 PRs are properly labelled,
then the plugin will stop working and will not pick any new PRs. There is an
interesting behaviour of that - if there are unlabelled old PRs, they will get
updated, so - in fact - they will disappear from the "oldest" list and possibly
some new PRs that will be checked in the new pass. However once you have 30
oldest, properly labelled PRs, the plugin stops working. That's really annoying
and difficult to detect problem - especially if your repo does not have a lot
of PRs.

Pagination is easy. It is described here:
https://github.com/google/go-github/#pagination

@kaxil
Copy link

kaxil commented Jan 4, 2020

@paulfantom Would love to get this merged. We want to use it for https://github.com/apache/airflow project

main.go Outdated Show resolved Hide resolved
for _, s := range slice {
set[s] = struct{}{}
}
set := make(map[string]struct{}, len(slice))
Copy link
Contributor Author

@potiuk potiuk Jan 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I run gofmt with the project and it formatted it this way. I hope it's OK for you in one PR.

@potiuk potiuk force-pushed the add-missing-pagination branch 3 times, most recently from bcce8e1 to 377ef0f Compare January 4, 2020 13:58
Fixes paulfantom#3.

Unfortunately most of the Github APIs returning collections returns maximum 30
responses. This is documented here https://developer.github.com/v3/#pagination
but unfortunately it is not mentioned in the particular APIs that are affected.
So when reading https://developer.github.com/v3/pulls/#list-pull-requests it's
not at all obvious that pagination is in operation and that you should add
another loop through pages.

Periodic labeler does not have pagination implemented, therefore the result is
that in big repositories with > 30 PRs only the OLDEST prs will be checked
(that's what is the sorting order). If all those 30 PRs are properly labelled,
then the plugin will stop working and will not pick any new PRs. There is an
interesting behaviour of that - if there are unlabelled old PRs, they will get
updated, so - in fact - they will disappear from the "oldest" list and possibly
some new PRs that will be checked in the new pass. However once you have 30
oldest, properly labelled PRs, the plugin stops working. That's really annoying
and difficult to detect problem - especially if your repo does not have a lot
of PRs.

Pagination is easy. It is described here:
https://github.com/google/go-github/#pagination
@potiuk potiuk force-pushed the add-missing-pagination branch from 377ef0f to e556bb8 Compare January 4, 2020 14:02
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 4, 2020
The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 4, 2020
The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.
potiuk added a commit to apache/airflow that referenced this pull request Jan 4, 2020
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.
@paulfantom
Copy link
Owner

Awesome! Thanks!

@paulfantom paulfantom merged commit 330d41b into paulfantom:master Jan 6, 2020
galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
…pache#7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 15, 2021
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 17, 2021
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 23, 2021
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Nov 25, 2021
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Mar 9, 2022
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jun 3, 2022
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jun 6, 2022
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jul 8, 2022
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Aug 26, 2022
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 3, 2022
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
aglipska pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 7, 2022
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Dec 7, 2022
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jan 27, 2023
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
kosteev pushed a commit to kosteev/composer-airflow-test-copybara that referenced this pull request Sep 11, 2024
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
kosteev pushed a commit to kosteev/composer-airflow-test-copybara that referenced this pull request Sep 12, 2024
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 16, 2024
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
kosteev pushed a commit to kosteev/composer-airflow-test-copybara that referenced this pull request Sep 17, 2024
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Nov 6, 2024
…7047)

The periodic labeler had a bug that it was not using pagination.
In effect the labeler was checking only 30 oldest request and
if they all contained proper labels, it was not checking any more
PRs.

Details are available in
paulfantom/periodic-labeler#4

But until it is merged, we switch to our own fixed version.

GitOrigin-RevId: 592284a7e41adc4ff0ee5a084385e596abf270a6
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.

Pagination is not implemented
3 participants