Skip to content

Conversation

@raulcd
Copy link
Member

@raulcd raulcd commented Jul 12, 2023

Rationale for this change

We currently were missing maintenance branches due to pagination on GH API.

What changes are included in this PR?

Check whether the API is returning a paginated view and extend the list returned.

Are these changes tested?

I have tested locally:

(Pdb) pr.maintenance_branches
['maint-0.11.x', 'maint-0.12.x', 'maint-0.14.x', 'maint-0.15.x', 'maint-0.17.x', 'maint-1.0.x', 'maint-3.0.x', 'maint-4.0.x', 'maint-6.0.x', 'maint-7.0.x', 'maint-7.0.1', 'maint-8.0.x', 'maint-9.0.0', 'maint-10.0.x', 'maint-10.0.0', 'maint-10.0.1', 'maint-11.0.0', 'maint-12.0.x', 'maint-12.0.0', 'maint-12.0.1', 'maint-13.0.0']
(Pdb) c
Enter fix version [14.0.0]: 

Are there any user-facing changes?

No

@raulcd raulcd requested review from assignUser and kou as code owners July 12, 2023 12:51
@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Jul 12, 2023
@raulcd
Copy link
Member Author

raulcd commented Jul 12, 2023

There are currently no tests on the merge_pr script around neither GH issues or any interaction with GH. We should add them but this is tracked on this PR: #14802

@raulcd
Copy link
Member Author

raulcd commented Jul 12, 2023

Just for the record, other libraries like pygithub use the link header for the pagination and do a similar parsing: https://github.com/PyGithub/PyGithub/blob/de80ff4b914b9ffd47347d60b14cbf270e07f346/github/PaginatedList.py#L241

Copy link
Member

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

Ah good catch! Is this something we need to add to archery somewhere too? Or not as that is using an api wrapper that handles it?

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 13, 2023
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Jul 13, 2023
@raulcd
Copy link
Member Author

raulcd commented Jul 13, 2023

Is this something we need to add to archery somewhere too? Or not as that is using an api wrapper that handles it?

The pygithub and github3.py libraries used on archery handle pagination. I am not sure if there's any place where we should explicitly call to get next pages, etcetera but I don't think so as from what I've seen they just return iterators and handle that for us. Would be good though to try and refactor archery to use a single library so we are sure around those: #33241

@raulcd raulcd added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Jul 13, 2023
@kou kou merged commit a4384d9 into apache:main Jul 14, 2023
@kou kou removed the awaiting merge Awaiting merge label Jul 14, 2023
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit a4384d9.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

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.

[Dev] Merge PR script defaults to the wrong milestone when merging

3 participants