Update service mapping URLs in preparation for the dist-git migration #996
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,15 +2,26 @@ | |
| # SPDX-License-Identifier: MIT | ||
|
|
||
| import functools | ||
| import logging | ||
| import time | ||
| from collections.abc import Iterable | ||
| from typing import Optional | ||
|
|
||
| from requests.exceptions import ConnectionError, ReadTimeout | ||
|
|
||
| from ogr.abstract import GitProject, GitService | ||
| from ogr.constant import DGIT_URLS | ||
| from ogr.exceptions import OgrException, OgrNetworkError | ||
| from ogr.parsing import parse_git_repo | ||
|
|
||
| _SERVICE_MAPPING: dict[str, type[GitService]] = {} | ||
|
|
||
| # cache dist-git forge service class for two minutes | ||
| _DGIT_FORGE_CACHE: dict[str, tuple[float, type[GitService]]] = {} | ||
| _DGIT_FORGE_CACHE_TTL = 120 | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def use_for_service(service: str, _func=None): | ||
| """ | ||
|
|
@@ -29,7 +40,7 @@ class GithubService(BaseGitService): | |
| pass | ||
|
betulependule marked this conversation as resolved.
|
||
|
|
||
| @use_for_service("pagure.io") | ||
| @use_for_service("git.centos.org") | ||
| class PagureService(BaseGitService): | ||
| pass | ||
| ``` | ||
|
|
@@ -116,6 +127,10 @@ def get_service_class_or_none( | |
| ) -> Optional[type[GitService]]: | ||
|
There was a problem hiding this comment. [critical] behavioral-contract-change get_service_class_or_none() now raises OgrNetworkError for dist-git URLs when the Pagure probe request fails with ConnectionError or ReadTimeout. Previously this function returned Optional[type[GitService]] and never raised exceptions. Downstream callers (get_service_class, get_instances_from_dict, get_project, and packit's Config.load_authentication()) do not handle this new exception type. Suggested fix: Either catch the error inside get_service_class_or_none and return a fallback (e.g., ForgejoService from the static mapping) or None, or update all callers to handle OgrNetworkError. |
||
| """ | ||
| Get the matching service class from the URL. | ||
| When attempting to get the matching service class for dist-git, probing | ||
| is used to determine whether `PagureService` or `ForgejoService` | ||
| should be returned. This information is cached for two minutes. The | ||
| probing request is set to timeout after 5 seconds. | ||
|
|
||
| Args: | ||
| url: URL of the project, e.g. `"https://github.com/packit/ogr"`. | ||
|
|
@@ -126,13 +141,59 @@ def get_service_class_or_none( | |
|
|
||
|
There was a problem hiding this comment. [high] behavioral-contract-break get_service_class_or_none() changes from a pure in-memory dict lookup to a function that makes HTTP requests for dist-git URLs and can now raise OgrNetworkError. This is a new failure mode for all callers including get_service_class() and get_instances_from_dict(). Suggested fix: Consider catching OgrNetworkError internally and falling back to ForgejoService, preserving the function's no-raise contract. |
||
| Returns: | ||
| Matched class (subclass of `GitService`) or `None`. | ||
|
|
||
| Raises: | ||
| OgrNetworkError, in case a ConnectionError or ReadTimeout error | ||
| is encountered when attempting to probe Pagure dist-git. | ||
| """ | ||
| mapping = {} | ||
| mapping.update(_SERVICE_MAPPING) | ||
| non_overridden_dgit_urls: Iterable[str] = DGIT_URLS | ||
|
|
||
| if service_mapping_update: | ||
| mapping.update(service_mapping_update) | ||
| non_overridden_dgit_urls = ( | ||
| set(non_overridden_dgit_urls) - service_mapping_update.keys() | ||
| ) | ||
|
|
||
| parsed_url = parse_git_repo(url) | ||
|
|
||
| # [XXX] remove once the migration of dist-git is finished | ||
|
Member
There was a problem hiding this comment. Just an FYI:
|
||
| for dgit_url in non_overridden_dgit_urls: | ||
|
|
||
| # if dealing with dist-git, we need to check whether we need to use | ||
|
There was a problem hiding this comment. [high] logic-error The dist-git URL check uses Suggested fix: Use |
||
| # `PagureService` or `ForgejoService` | ||
| if dgit_url in parsed_url.hostname: | ||
|
|
||
| from ogr.services.forgejo import ForgejoService | ||
| from ogr.services.pagure import PagureService | ||
|
|
||
| now = time.monotonic() | ||
| if cache := _DGIT_FORGE_CACHE.get(dgit_url): | ||
| timestamp, service_type = cache | ||
| if now - timestamp < _DGIT_FORGE_CACHE_TTL: | ||
| return service_type | ||
|
betulependule marked this conversation as resolved.
|
||
| # API call to the Pagure backend | ||
| api_endpoint = "https://src.fedoraproject.org/api/0/version" | ||
|
There was a problem hiding this comment. [low] trust-boundary Forge detection relies on HTTP response from Fedora infrastructure. Document the trust assumption. |
||
| api_endpoint_stg = "https://src.stg.fedoraproject.org/api/0/version" | ||
|
Comment on lines
+177
to
+178
Member
There was a problem hiding this comment. I was going to say that it doesn’t work for pagure.io, so it’s some dist-git specific patch, but… now that I checked the forge itself, I cannot seem to get there altogether…
Contributor
Author
There was a problem hiding this comment. Are you sure? I can get to "https://src.stg.fedoraproject.org/api/0/version" just fine. |
||
| request_url = api_endpoint_stg if ".stg." in dgit_url else api_endpoint | ||
|
|
||
| try: | ||
| pagure_service = PagureService() | ||
| response = pagure_service.get_raw_request(url=request_url, timeout=5) | ||
|
|
||
| # if not found, then dist-git is no longer hosted on Pagure | ||
| dgit_service_kls = ( | ||
| PagureService if response.status_code != 404 else ForgejoService | ||
| ) | ||
|
|
||
| _DGIT_FORGE_CACHE[dgit_url] = (now, dgit_service_kls) | ||
| return dgit_service_kls | ||
|
|
||
| except (ConnectionError, ReadTimeout) as er: | ||
| logger.error(er) | ||
| raise OgrNetworkError(f"Cannot connect to url: '{url}'.") from er | ||
|
|
||
| for service, service_kls in mapping.items(): | ||
| if parse_git_repo(service).hostname in parsed_url.hostname: | ||
| return service_kls | ||
|
|
@@ -178,13 +239,13 @@ def get_instances_from_dict(instances: dict) -> set[GitService]: | |
| ```py | ||
| get_instances_from_dict({ | ||
| "github.com": {"token": "abcd"}, | ||
| "forgejo": { | ||
| "token": "abcd", | ||
| "instance_url": "https://src.fedoraproject.org", | ||
| }, | ||
| }) == { | ||
| GithubService(token="abcd"), | ||
| ForgejoService(token="abcd", instance_url="https://src.fedoraproject.org") | ||
| } | ||
| ``` | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,17 +26,13 @@ | |
|
|
||
|
|
||
| @use_for_service("pagure") | ||
| @use_for_service("git.centos.org") | ||
| @use_for_service("git.stg.centos.org") | ||
| class PagureService(BaseGitService): | ||
| def __init__( | ||
| self, | ||
| token: Optional[str] = None, | ||
| instance_url: str = "https://pagure.io", | ||
|
There was a problem hiding this comment. [high] default-parameter-break PagureService.init default instance_url changed from 'https://src.fedoraproject.org' to 'https://pagure.io'. Any code constructing PagureService() without specifying instance_url will silently connect to a different server. Suggested fix: Document as a breaking change in the PR description and changelog.
Member
There was a problem hiding this comment. this is going to die too tbf, not sure if before or after dist-git
Contributor
Author
There was a problem hiding this comment. Yep, as you pointed out, "pagure.io" is read-only now. I could change
Member
There was a problem hiding this comment. (best would be no default, but… that is breaking change… idk, the pagure.io is probably the most sane default)
Contributor
Author
There was a problem hiding this comment. Well, changing
Contributor
Author
There was a problem hiding this comment. As fullsend pointed out, another breaking change similar to this one is in I think it would be best to drop |
||
| read_only: bool = False, | ||
| insecure: bool = False, | ||
| max_retries: Union[int, urllib3.util.Retry] = 3, | ||
|
|
@@ -250,6 +246,7 @@ def get_raw_request( | |
| params=None, | ||
| data=None, | ||
| header=None, | ||
| timeout: Optional[float] = None, | ||
| ) -> RequestResponse: | ||
| """ | ||
| Call API endpoint and wrap the response in `RequestResponse` type. | ||
|
|
@@ -279,6 +276,7 @@ def get_raw_request( | |
| headers=headers, | ||
| data=data, | ||
| verify=not self.insecure, | ||
| timeout=timeout, | ||
| ) | ||
| logger.debug( | ||
| f"Ogr sent request with following headers: {headers | {'Authorization': '<redacted>'}}", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # Copyright Contributors to the Packit project. | ||
| # SPDX-License-Identifier: MIT | ||
|
|
||
| import pytest | ||
| from flexmock import flexmock | ||
|
|
||
| from ogr import PagureService | ||
|
|
||
|
|
||
| # mocks API calls to Pagure dist-git made to determine whether dist-git | ||
| # is still hosted on Pagure and returns the status code expected after | ||
| # the migration of dist-git to Forgejo | ||
| @pytest.fixture(autouse=True) | ||
| def setup_api_request_mock(): | ||
|
There was a problem hiding this comment. [low] test-isolation _DGIT_FORGE_CACHE is module-level state never cleared between tests. First test populates cache; subsequent tests within TTL use cached values instead of the mock.
Contributor
Author
There was a problem hiding this comment. Sure, but I don't think it matters. There was a problem hiding this comment. [medium] missing-test-coverage The autouse fixture always returns a 404 response (post-migration state). No test verifies the pre-migration path where Pagure returns a non-404 status and PagureService should be returned. |
||
| response = flexmock(status_code=404) | ||
| flexmock(PagureService).should_receive("get_raw_request").with_args( | ||
| url="https://src.fedoraproject.org/api/0/version", | ||
| timeout=5, | ||
| ).and_return( | ||
| response, | ||
| ) | ||
| flexmock(PagureService).should_receive("get_raw_request").with_args( | ||
| url="https://src.stg.fedoraproject.org/api/0/version", | ||
| timeout=5, | ||
| ).and_return( | ||
| response, | ||
| ) | ||
|
|
||
| return | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] naming-convention
DGIT abbreviation is non-standard in the codebase. Consider DISTGIT_URLS for clarity.