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

drop(integration): Always fetch max number of pages #83984

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

armenzg
Copy link
Member

@armenzg armenzg commented Jan 24, 2025

It is very easy to write code and not pass the parameter, thus, we won't get the whole set of repositories.

This will cause the calls to be slower for customers with thousands of repositories, however, it will be more accurate.

We still have self.page_number_limit to stop the requests after 50-page fetches (5,000 repositories).

It is very easy to write code and not pass the parameter, thus, we won't get the whole set of repositories.

This will cause the calls to be slower for customers with thousands of repos, however, it will be more accurate.

We still have `self.page_number_limit` to stop the requests after 50 page fetches (5,000 repositories).
@armenzg armenzg self-assigned this Jan 24, 2025
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 24, 2025
Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ntry/integrations/github_enterprise/integration.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #83984       +/-   ##
===========================================
+ Coverage   33.07%   87.56%   +54.49%     
===========================================
  Files        8079     9545     +1466     
  Lines      450123   541175    +91052     
  Branches    21290    21274       -16     
===========================================
+ Hits       148861   473870   +325009     
+ Misses     300907    66950   -233957     
  Partials      355      355               

repos = self.get_with_pagination(
"/installation/repositories",
response_key="repositories",
page_number_limit=self.page_number_limit if fetch_max_pages else 1,
Copy link
Member Author

Choose a reason for hiding this comment

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

self.page_number_limit is the default, thus, we don't need to pass it anymore.

response_key="repositories",
page_number_limit=self.page_number_limit if fetch_max_pages else 1,
)
return [repo for repo in repos if not repo.get("archived")]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now handled by the callers:

for i in [repo for repo in repos if not repo.get("archived")]

for i in [repo for repo in repos if not repo.get("archived")]

@armenzg armenzg marked this pull request as ready for review January 24, 2025 14:56
@armenzg armenzg requested a review from a team as a code owner January 24, 2025 14:56
@armenzg armenzg requested a review from a team as a code owner January 24, 2025 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant