Skip to content

Raise GithubAPIException when github instance is None for rate limit check #972

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.

Sign up for GitHub

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

Merged
majamassarini merged 1 commit into from
Jan 30, 2026
Merged

Raise GithubAPIException when github instance is None for rate limit check #972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions ogr/abstract/git_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,18 @@ def get_group(self, group_name: str):
"""
raise NotImplementedError

def get_rate_limit_remaining(self) -> Optional[int]:
def get_rate_limit_remaining(
self,
namespace: Optional[str] = None,
repo: Optional[str] = None,
) -> Optional[int]:
"""
Get the remaining rate limit.
Get the remaining rate limit. If namespace and repo are set,
returns repository-specific rate limit.

Args:
namespace: Namespace of the project.
repo: Repository name.

Returns:
Number of remaining API requests, or None if rate limit information
Expand Down
6 changes: 5 additions & 1 deletion ogr/services/forgejo/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,11 @@ def get_project_from_url(self, url: str) -> "ForgejoProject":

return self.get_project(repo=repo, namespace=namespace)

def get_rate_limit_remaining(self) -> Optional[int]:
def get_rate_limit_remaining(
self,
namespace: Optional[str] = None,
repo: Optional[str] = None,
) -> Optional[int]:
"""
There is no way to check rate limit status from Forgejo API.
"""
Expand Down
39 changes: 35 additions & 4 deletions ogr/services/github/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from urllib3.util import Retry

from ogr.abstract import AuthMethod, GitUser
from ogr.exceptions import GithubAPIException
from ogr.exceptions import GithubAPIException, OgrException
from ogr.factory import use_for_service
from ogr.services.base import BaseGitService, GitProject
from ogr.services.github.auth_providers import (
Expand Down Expand Up @@ -240,8 +240,7 @@ def list_projects(

return projects

def get_rate_limit_remaining(self) -> Optional[int]:
rate_limit = self.github.get_rate_limit()
def _get_rate_limit_value(self, rate_limit) -> int:
# Handle both old and new PyGithub API versions
# Old API in f42 and f43: rate_limit.resources.core.remaining
# New API in rawhide (f44): rate_limit.core.remaining (or rate_limit.remaining)
Expand All @@ -250,4 +249,36 @@ def get_rate_limit_remaining(self) -> Optional[int]:
return rate_limit.resources.core.remaining
except AttributeError:
return rate_limit.core.remaining
return None

def get_rate_limit_remaining(
self,
namespace: Optional[str] = None,
repo: Optional[str] = None,
) -> Optional[int]:
# If namespace and repo are provided, use repository-specific instance
# (needed for GithubApp/Tokman auth where service.github is None)
if namespace and repo:
try:
github_instance = self.get_pygithub_instance(namespace, repo)
rate_limit = github_instance.get_rate_limit()
return self._get_rate_limit_value(rate_limit)
except (OgrException, github.GithubException) as ex:
logger.error(
f"Failed to get rate limit from repository-specific instance "
f"for {namespace}/{repo}: {ex}",
)
return None

# Try using generic github instance
if not self.github:
auth_type = (
type(self.authentication).__name__ if self.authentication else "unknown"
)
raise GithubAPIException(
f"Github instance is not available for rate limit check. "
f"This typically occurs when using {auth_type} authentication, "
f"which requires repository-specific instances.",
)

rate_limit = self.github.get_rate_limit()
Copy link
Copy Markdown
Member

@nforro nforro Jan 28, 2026

Choose a reason for hiding this comment

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

Is the fallback really necessary? Does get_pygithub_instance() not work with token authentication?

Copy link
Copy Markdown
Member Author

@majamassarini majamassarini Jan 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

get_pygithub_instance requires a namespace and a repo. Which I might not have. However, these are optional in this method. In packit service, I call this method without namespace and repo. If that fails I fall back to using them instead. packit/packit-service#2970

Copy link
Copy Markdown
Member

@nforro nforro Jan 29, 2026

Choose a reason for hiding this comment

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

get_pygithub_instance requires a namespace and a repo. Which I may not have. They are optional in this method. In packit service, I call this method without namespace and repo. If it fails I fall back trying to use use them. packit/packit-service#2970

I don't see anything that would indicate namespace and repo can be None. But even if they can, aren't two calls of get_rate_limit_remaining() unnecessary? (This comment probably belongs to the other PR, but they are interconnected).

Copy link
Copy Markdown
Member Author

@majamassarini majamassarini Jan 29, 2026

Choose a reason for hiding this comment

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

My get_rate_limit_remaining method has those arguments as optional, and I think that's the right approach for Ogr. If the library caller doesn't use App ID authentication, there's no reason to tie rate limiting retrieval to a specific namespace and repo. For Packit Service, I'm not sure, when using a token instead of the App ID, I don't know if we always have namespace and repo available. This approach feels safer to me, but I can try always passing these arguments and see what happens. Because setting up a proper test for this use case would take too much time.

Copy link
Copy Markdown
Member

@nforro nforro Jan 29, 2026

Choose a reason for hiding this comment

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

My get_rate_limit_remaining method has those arguments as optional, and I think that's the right approach for Ogr.

Sure, I agree.

This approach feels safer to me, but I can try always passing these arguments and see what happens.

It makes more sense to me to call get_rate_limit_remaining() once and handle this in ogr (which you already do, I'm just a bit unsure about the logic).

return self._get_rate_limit_value(rate_limit)
10 changes: 7 additions & 3 deletions ogr/services/gitlab/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,11 @@ def list_projects(
for project in projects_to_convert
]

def get_rate_limit_remaining(self) -> Optional[int]:
def get_rate_limit_remaining(
self,
namespace: Optional[str] = None,
repo: Optional[str] = None,
) -> Optional[int]:
# python-gitlab doesn't have get_rate_limit(), so we make a lightweight
# HEAD request to get rate limit headers from GitLab API
# GitLab returns rate limit in headers: ratelimit-remaining
Expand All @@ -191,10 +195,10 @@ def get_rate_limit_remaining(self) -> Optional[int]:
)
return 0
logger.error(
f"Could not get rate limit from GitLab: {e}",
f"Could not get rate limit from GitLab instance {self.gitlab_instance.url}: {e}",
)
except Exception as e:
logger.error(
f"Could not get rate limit from GitLab: {e}",
f"Could not get rate limit from GitLab instance {self.gitlab_instance.url}: {e}",
)
return None
6 changes: 5 additions & 1 deletion ogr/services/pagure/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,11 @@ def get_group(self, group_name: str) -> PagureGroup:
url = self.get_api_url("group", group_name)
return PagureGroup(group_name, self.call_api(url))

def get_rate_limit_remaining(self) -> Optional[int]:
def get_rate_limit_remaining(
self,
namespace: Optional[str] = None,
repo: Optional[str] = None,
) -> Optional[int]:
"""
There is no way to check rate limit status from Pagure API.
"""
Expand Down
Loading